Migration to Lit Element #886
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request performs a comprehensive architectural migration from a Symbiote-based component system to a Lit-based (LitElement) implementation. The refactoring includes migrating Block, ActivityBlock, and UploaderBlock classes to their Lit equivalents; replacing TypedSchema-based collections with UID-keyed PubSub-backed state; updating all component rendering from static templates to Lit's render() methods; and modernizing dependencies (adding Lit, nanostores, updating Vitest to v4). Large portions of legacy abstraction layers are removed, with new Lit-compatible mixin infrastructure added to support reactive state, light DOM adoption, CSS data binding, and element registration. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as App / Demo
participant LitBlock as LitBlock
participant PubSub as PubSub Context
participant Managers as Managers (Locale/Modal/A11y/Telemetry)
participant Component as Component (render)
Loader->>LitBlock: instantiate & connect
LitBlock->>PubSub: registerCtx(initial state)
PubSub-->>LitBlock: PubSub instance
LitBlock->>Managers: addSharedContextInstance(key, resolver)
Managers-->>LitBlock: manager ready
LitBlock->>Component: initCallback() / requestUpdate()
Component->>PubSub: sub/read state keys
PubSub-->>Component: current state
Component->>Loader: render TemplateResult
Loader->>Component: user event (`@click`)
Component->>PubSub: pub(key, newValue)
PubSub->>Component: notify subscribers
Component->>Component: requestUpdate() / re-render
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (6)
🧰 Additional context used🧬 Code graph analysis (1)src/blocks/CloudImageEditor/src/CloudImageEditorBlock.ts (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (20)
Comment |
nd0ut
force-pushed
the
feat/lit-element
branch
6 times, most recently
from
bb4bcc6 to
35d0ec5
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
nd0ut
force-pushed
the
feat/lit-element
branch
from
546d2b2 to
7f3a67c
Compare
nd0ut
force-pushed
the
feat/lit-element
branch
3 times, most recently
from
222e1be to
f1012c4
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates the codebase from Symbiote.js to Lit Element, a major architectural change that modernizes the component framework while maintaining existing functionality.
Key Changes:
- Replaced Symbiote.js reactive system with Lit Element's property/state decorators and rendering model
- Introduced new Lit-based base classes (
LitBlock,LitUploaderBlock,LitActivityBlock,LitSolutionBlock) to replace Symbiote counterparts - Separated Vitest configuration from Vite configuration into dedicated files
- Updated TypeScript configurations to support Lit decorators and removed JavaScript compilation
Reviewed changes
Copilot reviewed 172 out of 210 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
vitest.config.ts |
New test configuration file extracting test setup from vite.config |
vite.config.ts |
Converted to TypeScript, removed test configuration |
tsconfig.*.json |
Added decorator support and updated type references for Lit migration |
src/lit/*.ts |
New Lit compatibility layer and base classes |
src/blocks/**/*.ts |
Migrated all blocks to use Lit decorators and rendering |
src/solutions/**/*.ts |
Updated solution blocks to extend Lit base classes |
src/utils/*.ts |
New utility functions for Lit compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| iframe.addEventListener('load', this._handleIframeLoad.bind(this)); | ||
|
|
There was a problem hiding this comment.
Duplicate event listener registration. The 'load' event listener is being added twice on lines 243 and 245. Remove one of these duplicate lines.
| iframe.addEventListener('load', this._handleIframeLoad.bind(this)); |
| // @ts-expect-error | ||
| iframe.allowTransparency = true; |
There was a problem hiding this comment.
These deprecated iframe attributes should be set using setAttribute() instead of direct property assignment. For example: iframe.setAttribute('marginheight', '0'). The ts-expect-error comments indicate these properties don't exist in modern TypeScript types because they're deprecated.
| // @ts-expect-error | |
| iframe.allowTransparency = true; | |
| // Set deprecated attribute using setAttribute (see CodeQL recommendation) | |
| iframe.setAttribute('allowTransparency', 'true'); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
specs/npm/npm.test.ts (1)
35-63: Do not swallow unexpected dynamic import errors in testsThe new try/catch around the web bundle imports will mask real failures:
- For any error that isn’t the specific CSS
ERR_UNKNOWN_FILE_EXTENSION,mremainsundefined, but the test still proceeds to snapshotstickyPackageVersion(m).- That can allow regressions (e.g. missing
.jsbundles, wrong paths) to slip by as changed snapshots instead of failing the test.You probably only want to special‑case CSS bundles. Consider something like:
- for (const bundle of webBundles) { - let m: unknown; - try { - m = await import(`@uploadcare/file-uploader/web/${bundle}`); - } catch (error) { - // Vite@4 can't dynamically import css files - // Here we're just want to ensure that the css file exists and is importable - // In that case we receive specific error which we can safely ignore - if (error instanceof Error && 'code' in error && error.code === 'ERR_UNKNOWN_FILE_EXTENSION') { - m = { default: '' }; - } - } - expect(stickyPackageVersion(m)).toMatchSnapshot(`web/${bundle}`); - } + for (const bundle of webBundles) { + let m: unknown; + try { + m = await import(`@uploadcare/file-uploader/web/${bundle}`); + } catch (error) { + const isCssBundle = bundle.endsWith('.css'); + + // Vite@4 can't dynamically import css files + // Here we just want to ensure that the css file exists and is importable. + if ( + isCssBundle && + error instanceof Error && + 'code' in error && + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (error as any).code === 'ERR_UNKNOWN_FILE_EXTENSION' + ) { + m = { default: '' }; + } else { + throw error; + } + } + + expect(stickyPackageVersion(m)).toMatchSnapshot(`web/${bundle}`); + }This preserves the intended CSS workaround while keeping other import problems visible.
src/abstract/managers/LocaleManager.ts (2)
22-52: Missing cleanup for subscriptions could cause memory leaks.The
subConfigValuecalls return unsubscribe functions that are not stored or called. When theLocaleManageris destroyed or the block disconnects, these subscriptions will remain active, potentially causing memory leaks and stale updates.Store the unsubscribe functions and call them in a
destroy()method:export class LocaleManager { private _blockInstance: LitBlock | null = null; private _localeName = ''; + private _unsubscribers: Array<() => void> = []; public constructor(blockInstance: LitBlock) { // ... existing code ... setTimeout(() => { - blockInstance.subConfigValue('localeName', async (localeName) => { + const unsubLocaleName = blockInstance.subConfigValue('localeName', async (localeName) => { // ... callback ... }); + this._unsubscribers.push(unsubLocaleName); - blockInstance.subConfigValue('localeDefinitionOverride', (localeDefinitionOverride) => { + const unsubOverride = blockInstance.subConfigValue('localeDefinitionOverride', (localeDefinitionOverride) => { // ... callback ... }); + this._unsubscribers.push(unsubOverride); }); } + + public destroy(): void { + this._unsubscribers.forEach((unsub) => unsub()); + this._unsubscribers = []; + this._blockInstance = null; + } }
27-31: Condition logic appears inverted - may skip valid locale changes.The condition on line 29
localeName !== DEFAULT_LOCALE && this._localeName !== localeNamereturns early when processing a new non-default locale (where_localeNamehasn't been updated yet). This seems backwards.If the intent is to skip stale callbacks (when the locale changed again before this async callback completed), the check should verify
_localeName !== localeNameafter it was set on line 27:if (!this._blockInstance || !localeName) { return; } - this._localeName = localeName; const definition = await resolveLocaleDefinition(localeName); - if (localeName !== DEFAULT_LOCALE && this._localeName !== localeName) { + if (this._localeName !== localeName) { + // Locale changed while we were loading - skip stale update return; } + this._localeName = localeName;Move
_localeNameassignment after the async operation and check if it still matches.src/blocks/Config/Config.ts (1)
192-211: Remove the redundant assignment on lines 208–210.Lines 204–206 correctly assign the config key via
attrKeyMapping[name]. The subsequent check on lines 208–210 assigns tothis[name]using the kebab-case attribute name, which doesn't align with the config system's design and serves no purpose. The state update is already handled by setting the config property on line 205, which triggers the setter defined ininitCallback(lines 170–177) or is stored for later initialization. The second assignment to the attribute name should be removed.src/blocks/CameraSource/CameraSource.ts (2)
345-358: Missing error handling for MediaRecorder initialization.If
MediaRecorderconstructor throws (e.g., unsupported codec), the error is caught but_mediaRecordermay remainnullwhile state changes have already occurred. Consider validating stream tracks before instantiation.if (this._stream) { + if (this._stream.getVideoTracks().length === 0) { + console.error('No video tracks available'); + return; + } this._mediaRecorder = new MediaRecorder(this._stream, this._options);
1089-1098: Duplicateclassattribute on button element.The button has
classspecified twice - once as a static string and once bound to_mutableClassButton. The secondclass=will override the first in Lit.<button type="button" - class="uc-shot-btn uc-camera-action" data-testid="shot" @click=${this._handleStartCamera} class=${this._mutableClassButton} ?hidden=${this._cameraHidden} >src/abstract/managers/ModalManager.ts (2)
113-120:closeAllnotifies with empty object butModalCbexpects{ id, modal }.The
closeAllmethod calls_notify(ModalEvents.CLOSE_ALL, {})with an empty object, but subscribers typed asModalCbexpect{ id: ModalId; modal: ModalNode }. This could cause runtime errors in subscribers that accessidormodal.Consider either:
- Notifying for each closed modal individually
- Creating a separate callback type for CLOSE_ALL events
public closeAll(): number { const count = this._activeModals.size; + // Notify for each modal being closed + for (const id of this._activeModals) { + const modal = this._modals.get(id); + if (modal) { + this._notify(ModalEvents.CLOSE, { id, modal }); + } + } this._activeModals.clear(); - this._notify(ModalEvents.CLOSE_ALL, {}); + this._notify(ModalEvents.CLOSE_ALL, {} as { id: ModalId; modal: ModalNode }); return count; }
165-170:destroynotifies after clearing subscribers.The
_notify(ModalEvents.DESTROY, {})call occurs after_subscribers.clear(), so no DESTROY event will ever be delivered to subscribers.public destroy(): void { this.closeAll(); this._modals.clear(); + this._notify(ModalEvents.DESTROY, {} as { id: ModalId; modal: ModalNode }); this._subscribers.clear(); - this._notify(ModalEvents.DESTROY, {}); }
♻️ Duplicate comments (3)
tests/bundles.e2e.test.tsx (1)
2-2: Vitest import path update (already flagged).This import path change from
@vitest/browser/contexttovitest/browseris consistent with the update intests/utils/test-renderer.tsx, where verification was already requested.src/blocks/SourceList/SourceList.ts (1)
42-46: Missing space in string concatenation.The error message is missing a space after "here:" which would result in malformed output like
"here:https://...".This issue was already flagged by static analysis (CodeQL).
console.error( "Instagram source was removed because the Instagram Basic Display API hasn't been available since December 4, 2024. " + - 'Official statement, see here:' + + 'Official statement, see here: ' + 'https://developers.facebook.com/blog/post/2024/09/04/update-on-instagram-basic-display-api/?locale=en_US', );src/abstract/CTX.ts (1)
9-21: LitBlock-based contexts look good; align*uploadTrigger/*secureUploadsManagertypes withSharedStateThe switch from
BlocktoLitBlockforactivityBlockCtx,uploaderBlockCtx, andsolutionBlockCtx, plus usingfnCtx.pub('*currentActivity', null)in*closeModal, matches the new Lit-based context model.As noted in
src/lit/SharedState.ts, consider:
- Initializing
*uploadTriggerasnew Set<Uid>()(instead ofSet<string>) to match theUid-typed shared state.- Making
*secureUploadsManagereither always non-null here or marking it nullable inSharedStateso both sides agree.Also applies to: 23-33, 35-38
🟡 Minor comments (15)
types/test/uc-cloud-image-editor.test-d.tsx-7-8 (1)
7-8: Error message is misleading and should clarify the component's actual requirements.The error message "no cdn-url" on line 7 is incomplete. The
CloudImageEditorBlockcomponent requiresctx-namepaired with eitheruuidORcdn-url(lines 33-36 ofsrc/blocks/CloudImageEditor/src/CloudImageEditorBlock.ts). The error message should indicate that eitheruuidorcdn-urlis required, not just mentioncdn-url. Consider updating the message to "missing uuid or cdn-url" or similar to reflect the actual type constraint shown in the valid examples on lines 13 and 15.demo/features/custom-icons.html-1-2 (1)
1-2: Missing<html>wrapper element.The document structure is incomplete—
<head>should be wrapped in an<html>element for valid HTML5.<!doctype html> +<html lang="en"> <head>Add the closing
</html>tag at the end of the file.src/blocks/CloudImageEditor/src/elements/button/BtnUi.ts-57-61 (1)
57-61: OverwritingclassNamemay remove existing classes.Setting
this.classNamedirectly replaces all existing classes on the element. If the element has other classes (e.g., from parent styling orstyleAttrs), they will be lost.Consider using
classListto add/remove the theme class instead:private _applyThemeClass(): void { + // Remove previous theme class if any + const existingTheme = [...this.classList].find(c => c.startsWith('uc-') && c !== 'uc-icon'); + if (existingTheme) { + this.classList.remove(existingTheme); + } if (this.theme && this.theme !== 'custom') { - this.className = `uc-${this.theme}`; + this.classList.add(`uc-${this.theme}`); } }Alternatively, if this is intentional and the component should only have the theme class, consider documenting this behavior.
src/blocks/DropArea/DropArea.ts-51-51 (1)
51-51: Incorrect ref type for content wrapper element.The ref is typed as
Ref<HTMLInputElement>but it's bound to a<div>element in the render method. This should beRef<HTMLDivElement>for type correctness.- private _contentWrapperRef: Ref<HTMLInputElement> = createRef(); + private _contentWrapperRef: Ref<HTMLDivElement> = createRef();demo/solutions/cloud-image-editor.html-34-34 (1)
34-34: Remove duplicate crop preset.The crop-preset attribute contains a duplicate "4:3" entry. This appears to be a copy-paste error.
Apply this diff:
- crop-preset="1:1, 16:9, 4:3, 4:3, 3:4, 9:16" + crop-preset="1:1, 16:9, 4:3, 3:4, 9:16"demo/external-sources/social-sources.html-1-3 (1)
1-3: Missing<html>opening tag.The document is missing the
<html>opening tag, which is required for valid HTML5 structure.<!doctype html> +<html lang="en"> <head>Also add the closing
</html>tag at the end of the document.Committable suggestion skipped: line range outside the PR's diff.
demo/bundles/cloud-image-editor.html-37-37 (1)
37-37: Duplicate crop preset value.The
crop-presetattribute contains"4:3"twice:"1:1, 16:9, 4:3, 4:3, 3:4, 9:16". This appears to be a typo.- crop-preset="1:1, 16:9, 4:3, 4:3, 3:4, 9:16" + crop-preset="1:1, 16:9, 4:3, 3:4, 9:16"demo/external-sources/secure-uploads.html-1-4 (1)
1-4: Malformed HTML structure.The document is missing the
<html>tag and<body>wrapper. While browsers are forgiving, a well-formed demo should include proper structure.<!doctype html> +<html lang="en"> <head> <meta name="viewport" content="width=device-width, initial-scale=1">And wrap the components in a
<body>:</head> <body> <uc-file-uploader-regular ctx-name="my-uploader"></uc-file-uploader-regular> ... </body> </html>demo/external-sources/secure-uploads.html-20-35 (1)
20-35: Remove ineffectiveexportand fix potential timestamp mismatch.
The
exportkeyword on an inline<script type="module">has no effect—module exports only work when loading from an external file.
getSecureExpire(options)is called twice (lines 22 and 34), which could return different timestamps if the calls span a second boundary, causing signature verification failures.- export const generateSecureSignature = async (secret, options) => { + const generateSecureSignature = async (secret, options) => { const encoder = new TextEncoder(); - const data = encoder.encode(getSecureExpire(options)); + const secureExpire = getSecureExpire(options); + const data = encoder.encode(secureExpire); const key = await window.crypto.subtle.importKey( 'raw', encoder.encode(secret), { name: 'HMAC', hash: { name: 'SHA-256' } }, false, ['sign'], ); const signature = await window.crypto.subtle.sign('HMAC', key, data); const secureSignature = Array.from(new Uint8Array(signature)) .map((byte) => byte.toString(16).padStart(2, '0')) .join(''); - return { secureSignature, secureExpire: getSecureExpire(options) }; + return { secureSignature, secureExpire }; };src/blocks/ProgressBar/ProgressBar.ts-8-95 (1)
8-95: Fake progress bar won't replay on subsequent upload cyclesThe progress handling is well-structured overall, but there's a behavioral issue when the component is reused across multiple cycles:
Once the fake progress animation fires its
animationiterationevent, theuc-fake-progress--hiddenclass is added (either becausevisiblebecomes false or_progressValue > 0). This class setsanimation: nonein CSS, which persists on the element for the component's lifetime.When reusing the same
<uc-progress-bar>instance for a second upload cycle—resettingvalueto 0 and togglingvisibleback to true—the fake progress animation won't replay because the class is still present. The element needs to have the class removed to re-enable the animation.Add this to the
visiblechange handler:if (changedProperties.has('visible')) { this.classList.toggle('uc-progress-bar--hidden', !this.visible); if (this.visible) { + if (this._progressValue === 0) { + this._fakeProgressLineRef.value?.classList.remove('uc-fake-progress--hidden'); + } this._updateProgressValueStyle(); } else { this._progressValue = this._normalizeProgressValue(this.value); } }This ensures the fake progress bar can restart its animation on each new upload cycle.
src/lit/LightDomMixin.ts-58-75 (1)
58-75: Consecutive comments before elements may not resolve correctly.The recursion at line 62-64 only advances when
nextSibling instanceof Element. If there are multiple consecutive comments before an element (e.g., Angular binding comments), the recursion stops at the second comment and returns''instead of finding the actual element's slot.private _getSlotNameForChild(child: AdoptedNode): string { // Both Angular and AngularJS will decorate nodes with comments when they // compile their template expressions. When we see a comment directly before // an element look ahead to find the slot. - if (child instanceof Comment && child.nextSibling instanceof Element) { + if (child instanceof Comment && child.nextSibling) { return this._getSlotNameForChild(child.nextSibling); }src/blocks/CloudImageEditor/src/elements/presence-toggle/PresenceToggle.ts-25-33 (1)
25-33: AddrequestUpdate()call in thevisiblesetter for Lit to detect changes.When using a custom setter with
@property, Lit requires an explicitrequestUpdate()call to schedule a re-render. The current setter stores to_visibleand calls_handleVisible(), but ifrequestUpdate()is not invoked, Lit's reactive system won't detect the property change. Either addthis.requestUpdate('visible', oldValue)in the setter before or after updating_visible, or simplify the pattern using@state()for internal reactive state if external property binding is not required.src/blocks/CloudImageEditor/src/elements/line-loader/LineLoaderUi.ts-25-42 (1)
25-42: Potential double_start()call on initial render whenactiveis true.When
activeis initiallytrue, bothfirstUpdated(line 27-29) andupdated(line 35-37) will call_start(). Inupdated,changedProperties.has('active')will be true on the first render ifactivewas set. Consider guarding against this or relying on only one lifecycle method.protected override firstUpdated(changedProperties: PropertyValues<this>): void { super.firstUpdated(changedProperties); - if (this.active) { - this._start(); - } + // Let updated() handle initial activation to avoid duplicate calls }Or alternatively, guard in
_start():private _start(): void { const lineEl = this._lineRef.value; - if (!lineEl) { + if (!lineEl || this._isAnimating) { return; }src/blocks/CloudImageEditor/src/EditorButtonControl.ts-51-61 (1)
51-61: Potentialthisbinding issue withonClickhandler.Assigning
this.onClicktoclickHandler(line 52) and then using it in the template loses thethisbinding. If subclasses overrideonClickand access instance properties viathis, it will fail.Use the method reference directly in the template to preserve binding:
public override render() { - const clickHandler = this.onClick; const title = this.title; return html` <button role="option" type="button" aria-label=${ifDefined(this.titleProp)} title=${ifDefined(this.titleProp)} - @click=${clickHandler} + @click=${this.onClick} >Alternatively, convert
onClickto an arrow function property or bind it in the constructor.src/abstract/TypedCollection.ts-193-203 (1)
193-203: Pending timeouts not cleared indestroy().The
_observeTimeoutand_notifyTimeoutmay still fire afterdestroy()is called, potentially causing errors or unexpected behavior if observers have been cleared.public destroy(): void { + if (this._observeTimeout) { + window.clearTimeout(this._observeTimeout); + } + if (this._notifyTimeout) { + window.clearTimeout(this._notifyTimeout); + } PubSub.deleteCtx(this._ctxId); this._propertyObservers = new Set(); this._collectionObservers = new Set();
| <script type="module"> | ||
| import '@/solutions/file-uploader/regular/index.css'; | ||
| import * as UC from '@/index.js'; | ||
|
|
||
| UC.defineComponents(UC); | ||
|
|
||
| const config = document.querySelector('uc-config'); | ||
| config.secureDeliveryProxyUrlResolver = (previewUrl) => { | ||
| return `http://localhost:3000/preview?url=${encodeURIComponent(previewUrl)}`; | ||
| }; |
There was a problem hiding this comment.
Avoid null access when wiring secureDeliveryProxyUrlResolver
The script in <head> queries for uc-config before the body is parsed:
const config = document.querySelector('uc-config');
config.secureDeliveryProxyUrlResolver = (previewUrl) => { ... };At that point config can be null, leading to a runtime error and a broken demo.
A minimal, robust fix is to wait for DOM readiness and guard against a missing element:
- const config = document.querySelector('uc-config');
- config.secureDeliveryProxyUrlResolver = (previewUrl) => {
- return `http://localhost:3000/preview?url=${encodeURIComponent(previewUrl)}`;
- };
+ window.addEventListener('DOMContentLoaded', () => {
+ const config = document.querySelector('uc-config');
+ if (!config) return;
+
+ config.secureDeliveryProxyUrlResolver = (previewUrl) => {
+ return `http://localhost:3000/preview?url=${encodeURIComponent(previewUrl)}`;
+ };
+ });Alternatively, you could move the script tag to after the <uc-config> element and still keep a simple null check.
Also applies to: 17-19
🤖 Prompt for AI Agents
In demo/external-sources/preview-proxy/secure-delivery-proxy-url-resolver.html
around lines 4 to 13 (and similarly lines 17 to 19), the script queries
document.querySelector('uc-config') before the body is parsed which can return
null and cause a runtime error; fix it by ensuring the DOM is ready and guarding
against a missing element: either wrap the wiring code in a DOMContentLoaded
handler (or move the script to after the <uc-config> element) and check that
config is non-null before assigning secureDeliveryProxyUrlResolver, failing
gracefully or logging if absent.
| "@biomejs/biome": "2.3.8", | ||
| "@biomejs/js-api": "^4.0.0", | ||
| "@biomejs/wasm-nodejs": "^2.3.8", | ||
| "@csstools/postcss-cascade-layers": "^5.0.2", | ||
| "@happy-dom/global-registrator": "^16.8.1", | ||
| "@happy-dom/global-registrator": "^20.0.10", | ||
| "@size-limit/esbuild-why": "^11.2.0", | ||
| "@size-limit/preset-small-lib": "^11.2.0", | ||
| "@total-typescript/ts-reset": "^0.6.1", | ||
| "@types/node": "^22", | ||
| "@types/react": "^18.2.34", | ||
| "@types/node": "^24", | ||
| "@types/react": "^18.0.0", | ||
| "@types/svg-sprite": "^0.0.39", | ||
| "@vitest/browser": "^3.2.4", | ||
| "@vitest/coverage-v8": "^3.2.4", | ||
| "esbuild": "^0.25.10", | ||
| "husky": "^8.0.3", | ||
| "lint-staged": "^13.2.1", | ||
| "node-watch": "^0.7.3", | ||
| "@vitest/browser": "^4.0.15", | ||
| "@vitest/browser-playwright": "^4.0.15", | ||
| "@vitest/coverage-v8": "^4.0.15", | ||
| "esbuild": "^0.25.12", | ||
| "esbuild-minify-templates": "^0.13.1", | ||
| "husky": "^9.1.7", | ||
| "lint-staged": "^16.2.6", | ||
| "node-watch": "^0.7.4", | ||
| "npm-run-all": "^4.1.5", | ||
| "playwright": "^1.55.0", | ||
| "playwright": "^1.56.1", | ||
| "postcss": "^8.5.6", | ||
| "publint": "^0.3.14", | ||
| "publint": "^0.3.15", | ||
| "render-jsx": "^0.2.4", | ||
| "rimraf": "^5.0.0", | ||
| "rimraf": "^6.1.0", | ||
| "shipjs": "^0.27.0", | ||
| "size-limit": "^11.2.0", | ||
| "stylelint": "^15.4.0", | ||
| "stylelint-config-standard": "^32.0.0", | ||
| "stylelint-declaration-block-no-ignored-properties": "^2.7.0", | ||
| "stylelint-order": "^6.0.3", | ||
| "stylelint": "^16.25.0", | ||
| "stylelint-config-standard": "^39.0.1", | ||
| "stylelint-declaration-block-no-ignored-properties": "^2.8.0", | ||
| "stylelint-order": "^7.0.0", | ||
| "svg-sprite": "^2.0.4", | ||
| "tsd": "^0.32.0", | ||
| "tsd": "^0.33.0", | ||
| "tsup": "^8.5.0", | ||
| "tsx": "^4.20.6", | ||
| "typescript": "^5.9.3", | ||
| "vite": "npm:rolldown-vite@latest", | ||
| "vitest": "^3.2.4" | ||
| "vite": "8.0.0-beta.0", | ||
| "vitest": "^4.0.15", | ||
| "vitest-browser-lit": "^1.0.1" | ||
| }, | ||
| "author": "Uploadcare Inc.", | ||
| "license": "MIT", | ||
| "dependencies": { | ||
| "@symbiotejs/symbiote": "^1.11.7", | ||
| "@lit/context": "^1.1.6", | ||
| "@uploadcare/cname-prefix": "^6.18.2", | ||
| "@uploadcare/image-shrink": "^6.18.2", | ||
| "@uploadcare/quality-insights": "^6.18.2", | ||
| "@uploadcare/upload-client": "^6.18.2", | ||
| "keyux": "^0.7.1" | ||
| "keyux": "^0.7.1", | ||
| "lit": "^3.3.1", | ||
| "nanostores": "^1.1.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check latest versions of key dependencies from npm registry
echo "=== Checking lit package ==="
curl -s https://registry.npmjs.org/lit | jq '.["dist-tags"].latest'
echo "=== Checking nanostores package ==="
curl -s https://registry.npmjs.org/nanostores | jq '.["dist-tags"].latest'
echo "=== Checking @lit/context package ==="
curl -s https://registry.npmjs.org/@lit/context | jq '.["dist-tags"].latest'
echo "=== Checking keyux package ==="
curl -s https://registry.npmjs.org/keyux | jq '.["dist-tags"].latest'
echo "=== Checking vitest package ==="
curl -s https://registry.npmjs.org/vitest | jq '.["dist-tags"].latest'
echo "=== Checking vite package ==="
curl -s https://registry.npmjs.org/vite | jq '.["dist-tags"].latest'
echo "=== Checking esbuild package ==="
curl -s https://registry.npmjs.org/esbuild | jq '.["dist-tags"].latest'Repository: uploadcare/file-uploader
Length of output: 355
🌐 Web query:
keyux npm security vulnerabilities 0.7.1
💡 Result:
I found no published security advisories or vulnerability reports for keyux v0.7.1. Sources: Socket's package scan shows the package and versions without alerts [1], and jsDelivr/Evil Martians documentation lists the package versions (no security advisories shown) [2][3].
If you want, I can:
- run a quick Snyk/GitHub/OSV search for that exact version and return any indirect-dependency issues, or
- check your project for indicators of compromise (lockfile scan).
🌐 Web query:
vite 8.0.0-beta.0 security vulnerabilities
💡 Result:
Short answer: there are no Vite-8-specific public CVEs I could find, but known Vite dev-server vulnerabilities (notably CVE-2025-30208 / arbitrary file read via crafted ?raw?? or ?import&raw?? queries) and earlier esbuild-related advisories can still affect projects using unpatched/dev-server behavior — so treat any 8.0.0-beta.0 dev-server as potentially exposed until you verify it includes the patches. [1][2][3]
What to check (quick checklist)
- Confirm the Vite 8.0.0-beta.0 package includes the CVE fixes for the dev server (CVE-2025-30208 was fixed in the 4.5.11 / 5.4.16 / 6.0.13 / 6.1.3 / 6.2.4 families). If 8.0.0-beta.0 is built on a version that did not include those fixes, it may be vulnerable. [1][6]
- Watch for esbuild advisories flagged by npm audit (GHSA-67mh-4wv8-2f99) — Vite historically pointed to esbuild versions that triggered audits; the project discussed this on GitHub. If your install pulls a vulnerable esbuild, address or upgrade accordingly. [3]
- Never expose the Vite dev server to untrusted networks (avoid --host / server.host or configure server.allowedHosts) — most of these issues require an exposed dev server. [1][5]
References
- CVE-2025-30208 / file-disclosure (details and affected versions). [1][4]
- Vite security page / advisories and project guidance. [5]
- GitHub issue re: esbuild / npm-audit reporting (GHSA advisory discussion). [3]
If you want, I can:
- check the published changelog/commit for v8.0.0-beta.0 and confirm whether those patches are included (I can look up the v8 beta release notes or compare commits).
🌐 Web query:
esbuild 0.25.12 security vulnerabilities
💡 Result:
Short answer: esbuild 0.25.12 is not affected by the GHSA-67mh-4wv8-2f99 CORS/serve vulnerability (that was fixed in 0.25.0) and public scanners (Snyk) show no direct vulnerabilities for esbuild itself in that release. [1][2]
Caveat: some esbuild npm packages include prebuilt native binaries compiled with a specific Go stdlib version. There are separate advisories (CVE-2025-22871) about a Go net/http issue and active esbuild GitHub issues to update the bundled Go stdlib — if your environment uses a binary built with an affected Go stdlib you may be exposed to that Go CVE until esbuild/binaries are rebuilt with patched Go. Check esbuild's repo/issue tracker and your installed binary build metadata. [3][2]
References
- GHSA-67mh-4wv8-2f99 (esbuild serve CORS) — affected <=0.24.2, fixed in 0.25.0. [1]
- esbuild v0.25.12 release notes. [2]
- esbuild issue about updating Go stdlib for CVE-2025-22871. [3]
Address vite beta version and outdated keyux dependency.
The main runtime dependencies (lit, nanostores, @lit/context) are current and secure. However, two issues require attention:
-
vite 8.0.0-beta.0: Using a beta version instead of stable 7.2.7. Beta versions may not include security patches; notably CVE-2025-30208 (file-disclosure via crafted dev-server queries) was fixed in stable release families. Verify the beta includes those patches or upgrade to stable 7.2.7.
-
keyux 0.7.1: Outdated (latest is 0.11.3), though no known security vulnerabilities are published for this version. Consider updating if compatible.
esbuild 0.25.12 is outdated but has no direct npm vulnerabilities; verify the installed binary does not use an affected Go stdlib version (CVE-2025-22871).
| import postcss from 'postcss'; | ||
| import { build as tsupBuild } from 'tsup'; | ||
| import { dependencies } from '../package.json'; | ||
| import pkgJson from '../package.json'; |
There was a problem hiding this comment.
Make banner robust to different repository shapes in package.json
banner() currently assumes pkgJson.repository.url exists; if repository is configured as a string (valid package.json form), this will be undefined and new URL('blob/main/LICENSE', repositoryUrl) will throw at build time.
Consider guarding for both string and object forms and for missing repository, e.g.:
- const repositoryUrl = pkgJson.repository.url;
- const licenseUrl = new URL('blob/main/LICENSE', repositoryUrl).toString();
+ const repository = pkgJson.repository as { url?: string } | string | undefined;
+ const repositoryUrl = typeof repository === 'string' ? repository : repository?.url;
+ const licenseUrl = repositoryUrl
+ ? new URL('blob/main/LICENSE', repositoryUrl).toString()
+ : null;
@@
- return [
+ return [
@@
- ` * License: ${licenseUrl}`,
+ licenseUrl && ` * License: ${licenseUrl}`,
@@
- ].join('\n');
+ ]
+ .filter(Boolean)
+ .join('\n');Also applies to: 16-31
🤖 Prompt for AI Agents
In scripts/build.ts around lines 8 and 16-31, the banner() helper assumes
pkgJson.repository.url exists which breaks when repository is a string or
missing; update the code to robustly derive repositoryUrl by handling three
cases: if pkgJson.repository is a string use that string; if it's an object
prefer pkgJson.repository.url || pkgJson.repository; otherwise treat it as
undefined. Normalize the extracted URL by stripping common prefixes like "git+"
and trailing ".git" before using it as the base for new URL('blob/main/LICENSE',
repositoryUrl), and only construct that URL (or wrap it in try/catch) when
repositoryUrl is truthy; fall back to a safe default (e.g., no link or relative
path) when repository is absent or invalid.
| public override disconnectedCallback(): void { | ||
| super.disconnectedCallback(); | ||
|
|
||
| const svg = this._svgElement; | ||
| svg?.removeEventListener('pointermove', this._handleSvgPointerMove, true); | ||
| document.removeEventListener('pointermove', this._handlePointerMove, true); | ||
| document.removeEventListener('pointerup', this._handlePointerUp, true); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "addEventListener" src/blocks/CloudImageEditor/src/CropFrame.tsRepository: uploadcare/file-uploader
Length of output: 408
🏁 Script executed:
sed -n '260,270p' src/blocks/CloudImageEditor/src/CropFrame.tsRepository: uploadcare/file-uploader
Length of output: 415
🏁 Script executed:
sed -n '570,595p' src/blocks/CloudImageEditor/src/CropFrame.tsRepository: uploadcare/file-uploader
Length of output: 829
Add cleanup for pointerdown event listener on interactionNode.
The disconnectedCallback removes three event listeners but misses the pointerdown listener added on line 262 to interactionNode. This listener is attached with .bind(this, direction) during _createThumbs(), creating a memory leak since the bound function reference is never stored or removed. Store the bound handler reference and remove it in disconnectedCallback to prevent listener accumulation.
🤖 Prompt for AI Agents
In src/blocks/CloudImageEditor/src/CropFrame.ts around lines ~262 and 600-607,
the pointerdown listener added to interactionNode in _createThumbs is attached
with .bind(this, direction) and the bound function is not stored or removed,
causing a memory leak; change _createThumbs to save the bound handler (e.g.
store per-direction handlers on this, or in a Map keyed by
interactionNode/direction) instead of using an anonymous bound call, use that
stored reference when calling addEventListener, and then in disconnectedCallback
remove the pointerdown listener by calling
interactionNode.removeEventListener('pointerdown', storedHandler, true) and
clear the stored reference so the handler can be GC'd.
| protected override updated(changedProperties: PropertyValues<this>): void { | ||
| super.updated(changedProperties); | ||
|
|
||
| this.sub('visible', (visible: boolean) => { | ||
| if (visible) { | ||
| if (changedProperties.has('visible' as keyof ProgressBarCommon)) { | ||
| if (this._visible) { | ||
| this.setAttribute('active', ''); | ||
| } else { | ||
| this.removeAttribute('active'); | ||
| } | ||
| }); | ||
|
|
||
| this.sub('*commonProgress', (progress: number) => { | ||
| this.$.value = progress; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Property name mismatch prevents attribute toggle.
The changedProperties.has('visible') check will never match because the property is named _visible, not visible. This means the active attribute will never be set or removed, breaking the visibility behavior.
protected override updated(changedProperties: PropertyValues<this>): void {
super.updated(changedProperties);
- if (changedProperties.has('visible' as keyof ProgressBarCommon)) {
+ if (changedProperties.has('_visible' as keyof ProgressBarCommon)) {
if (this._visible) {
this.setAttribute('active', '');
} else {
this.removeAttribute('active');
}
}
}🤖 Prompt for AI Agents
In src/blocks/ProgressBarCommon/ProgressBarCommon.ts around lines 45 to 55, the
updated() hook checks changedProperties.has('visible') but the actual property
is named _visible so the condition never fires; change the check to
changedProperties.has('_visible' as keyof ProgressBarCommon) (or reference the
public property if one exists) and keep using this._visible to set or remove the
'active' attribute so the attribute toggles correctly when the visibility state
changes.
src/lit/LitUploaderBlock.ts
Outdated
| private _flushCommonUploadProgress = (): void => { | ||
| let commonProgress = 0; | ||
| const uploadTrigger: Set<string> = this.$['*uploadTrigger']; | ||
| const uploadTrigger: Set<Uid> = this.$['*uploadTrigger']; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check uploadTrigger type definition in CTX and SharedState files
rg -n "uploadTrigger" --type ts src/abstract/CTX.ts src/lit/SharedState.tsRepository: uploadcare/file-uploader
Length of output: 185
Type mismatch between CTX initialization and SharedState definition for uploadTrigger.
The type assertion on line 340 is declared as Set<Uid> in SharedState.ts, but CTX.ts initializes '*uploadTrigger' as new Set<string>(). This type discrepancy between the initialization and type definition will cause runtime type mismatches. Ensure the initialization in CTX.ts uses the correct type.
🤖 Prompt for AI Agents
In src/lit/LitUploaderBlock.ts around line 340, the code casts
this.$['*uploadTrigger'] to Set<Uid> but CTX.ts initializes '*uploadTrigger' as
new Set<string>(); update the CTX.ts initialization to use new Set<Uid>() (and
import/alias the Uid type there if needed) so the runtime type matches
SharedState.ts; alternatively, if changing CTX.ts is not desired, change the
SharedState/type declaration to Set<string> consistently across CTX.ts and
SharedState.ts, then run typechecks to confirm no other mismatches remain.
| this.sub('*history', () => { | ||
| this.$.couldCancel = this.couldHistoryBack || this.couldShowList; | ||
| this._couldCancel = this._couldHistoryBack || this._couldShowList; | ||
| }); |
There was a problem hiding this comment.
_couldCancel may not update when *uploadList changes.
The subscription listens to *history, but _couldCancel depends on both _couldHistoryBack (which uses history) and _couldShowList (which uses *uploadList and cfg.showEmptyList). Changes to *uploadList won't trigger this subscription, potentially leaving _couldCancel stale.
Consider subscribing to both keys or adding a separate subscription:
this.sub('*history', () => {
this._couldCancel = this._couldHistoryBack || this._couldShowList;
});
+
+ this.sub('*uploadList', () => {
+ this._couldCancel = this._couldHistoryBack || this._couldShowList;
+ });📝 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.
| this.sub('*history', () => { | |
| this.$.couldCancel = this.couldHistoryBack || this.couldShowList; | |
| this._couldCancel = this._couldHistoryBack || this._couldShowList; | |
| }); | |
| this.sub('*history', () => { | |
| this._couldCancel = this._couldHistoryBack || this._couldShowList; | |
| }); | |
| this.sub('*uploadList', () => { | |
| this._couldCancel = this._couldHistoryBack || this._couldShowList; | |
| }); |
🤖 Prompt for AI Agents
In src/solutions/file-uploader/inline/FileUploaderInline.ts around lines 83-85,
the subscription only listens to '*history' so _couldCancel isn't recomputed
when '*uploadList' (or cfg.showEmptyList) changes; add a subscription for
'*uploadList' (and if config can change, also for the config key that controls
showEmptyList) that runs the same handler or refactor to subscribe to both keys
in one call, recomputing this._couldCancel = this._couldHistoryBack ||
this._couldShowList whenever either source changes.
| import { dirname, resolve } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { defineConfig } from 'vite'; | ||
|
|
||
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
|
|
||
| const alias = { | ||
| '@': resolve(__dirname, 'src'), | ||
| '~': __dirname, | ||
| }; | ||
|
|
||
| export default defineConfig(({ command }) => { | ||
| if (command === 'serve') { | ||
| return { | ||
| build: { | ||
| target: 'esnext', | ||
| }, | ||
| resolve: { | ||
| alias, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| throw new Error('Not implemented'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat vite.config.tsRepository: uploadcare/file-uploader
Length of output: 559
🏁 Script executed:
cat package.jsonRepository: uploadcare/file-uploader
Length of output: 6200
Avoid throwing for non‑serve commands in Vite config
The config currently throws an error for any command other than serve, which would break vite build, vite preview, and other tooling that relies on non-serve Vite commands. Since the returned config doesn't depend on the command parameter, simplify it to work for all commands:
-export default defineConfig(({ command }) => {
- if (command === 'serve') {
- return {
- build: {
- target: 'esnext',
- },
- resolve: {
- alias,
- },
- };
- }
-
- throw new Error('Not implemented');
-});
+export default defineConfig({
+ build: {
+ target: 'esnext',
+ },
+ resolve: {
+ alias,
+ },
+});Note: The project's build is currently orchestrated through a custom tsx script (build:lib) rather than vite build directly, so this issue doesn't immediately break the build pipeline. However, the fix removes a footgun that could block future adoption of direct Vite build invocations.
📝 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 { dirname, resolve } from 'node:path'; | |
| import { fileURLToPath } from 'node:url'; | |
| import { defineConfig } from 'vite'; | |
| const __dirname = dirname(fileURLToPath(import.meta.url)); | |
| const alias = { | |
| '@': resolve(__dirname, 'src'), | |
| '~': __dirname, | |
| }; | |
| export default defineConfig(({ command }) => { | |
| if (command === 'serve') { | |
| return { | |
| build: { | |
| target: 'esnext', | |
| }, | |
| resolve: { | |
| alias, | |
| }, | |
| }; | |
| } | |
| throw new Error('Not implemented'); | |
| }); | |
| import { dirname, resolve } from 'node:path'; | |
| import { fileURLToPath } from 'node:url'; | |
| import { defineConfig } from 'vite'; | |
| const __dirname = dirname(fileURLToPath(import.meta.url)); | |
| const alias = { | |
| '@': resolve(__dirname, 'src'), | |
| '~': __dirname, | |
| }; | |
| export default defineConfig({ | |
| build: { | |
| target: 'esnext', | |
| }, | |
| resolve: { | |
| alias, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In vite.config.ts around lines 1 to 25, the config throws an error for any
command other than "serve", which breaks "vite build", "vite preview", etc.;
remove the command-specific throw and return the config unconditionally (or
return the same config for all commands) so both serve and non-serve commands
work — update the default export to always provide the build.target and
resolve.alias settings and delete the throw and conditional branching.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/blocks/CloudImageEditor/src/EditorAspectRatioButtonControl.ts (2)
72-107: Unify equality logic between active state and click no‑opThe subscription marks a button active when either the
idmatches or thewidth/heightpair matches, butonClick()only short‑circuits whenids are equal. That means two different aspect-ratio objects with the same dimensions but different/missingids can be treated as “active” yet still trigger a write on click (or vice versa, depending on how you construct them).Consider extracting a small
isSameAspectRatio(a, b)helper and using it both in the subscription and inonClick()so the “is this already selected?” predicate is consistent.
118-196: Icon rendering now avoids nested<uc-icon>; consider tightening update callsThe combination of
_renderIcon()andrender()now yields exactly one<uc-icon>instance in all cases, so the earlier nested‑icon concern is resolved. The fixed‑ratio SVG rect is nicely centered viagetAdjustResolutions, and the freeform/null case defers to the configurediconname, which keeps things flexible.One small refinement:
_updateAspectRatioPresentation()callsrequestUpdate()for non‑freeform ratios, while theaspectRatiosetter already callsrequestUpdate('aspectRatio', previous). When_updateAspectRatioPresentation()is invoked via the setter this can cause a second, redundant update cycle; when it’s invoked frominitCallbackyou still want to ensure at least one update. If you decide to keep both call sites, you might either rely solely on the setter’srequestUpdateor add a brief comment explaining why the extra call is intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/blocks/CloudImageEditor/src/EditorAspectRatioButtonControl.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/blocks/CloudImageEditor/src/EditorAspectRatioButtonControl.ts (1)
src/blocks/CloudImageEditor/src/EditorButtonControl.ts (1)
EditorButtonControl(7-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
src/blocks/CloudImageEditor/src/EditorAspectRatioButtonControl.ts (1)
28-69: Freeform button wiring and rendering look solid
initCallback,_computeTitle, andrender()cleanly hook into*currentAspectRatio, set both visible title and tooltip, and reuse the sharedbuttonClasses/ARIA pattern. The custom layout (label before dropdown icon) is clear and consistent with the base control; no issues from my side here.
| @property({ type: Boolean, reflect: true }) | ||
| public active = false; |
There was a problem hiding this comment.
It seems that we don't need this. Because we don't calculate anything based on this property and don't apply any styles.
| return; | ||
| } | ||
| this._setCurrentValue(value); | ||
| this._emitSliderEvent('slider-change', value); |
There was a problem hiding this comment.
Why do we need this event if we're not going to use it anywhere after?
There was a problem hiding this comment.
Looks like some legacy event. Removed.
| } | ||
|
|
||
| export class TelemetryManager implements ITelemetryManager { | ||
| private readonly _sessionId: string = crypto.randomUUID(); |
There was a problem hiding this comment.
We could add a fallback UID.generate(); for _sessionId. Agree?
| @property({ type: String, reflect: true }) | ||
| public uuid: string | null = null; |
There was a problem hiding this comment.
Hooray! Reactivity for uuid is now available.
| fill: 'var(--color-image-background)', | ||
| 'fill-opacity': 0.85, | ||
| mask: `url(#${this._backdropMaskId})`, | ||
| mask: 'url(#backdrop-mask)', |
There was a problem hiding this comment.
If we don't calculate a unique ID for each backdrop mask, we'll have a bug.
There was a problem hiding this comment.
Oh, that's a merge issue. Fixed.
| const slider = this.$['*sliderEl'] as { setOperation: (op: string, filter: string) => void }; | ||
| slider.setOperation(this._operation, this._filter); | ||
| this.$['*showSlider'] = true; | ||
| if ('IntersectionObserver' in window) { |
There was a problem hiding this comment.
I'm not sure we need this condition.
src/lit/LitBlock.ts
Outdated
| persist: true, | ||
| }); | ||
| this.addSharedContextInstance('*telemetryManager', () => { | ||
| if (this.cfg.qualityInsights) { |
There was a problem hiding this comment.
It's rather strange, but at this level, all fields from Config are not reactive, meaning that here we get everything we get from initialConfig. This means we need to move the logic for enabling/disabling qualityInsights somewhere higher up.
| if (!this._editorConfig) { | ||
| return; | ||
| } | ||
| const normalized = cropPreset || undefined; |
There was a problem hiding this comment.
It seems, or there is no point in this
| public handleChange(event: CustomEvent<ChangeResult>): void { | ||
| this.debugPrint(`editor event "change"`, event.detail); | ||
| } |
There was a problem hiding this comment.
Why do we need this method?
There was a problem hiding this comment.
To print debug messages :)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/blocks/CloudImageEditor/src/EditorFilterControl.ts (1)
43-43: Setter type signature still incomplete (unresolved from previous review).The setter parameter is typed as
stringbut Lit's@property({ type: String })can passnullorundefinedwhen attributes are removed or unset. The runtime code handles this correctly withvalue ?? ''on line 44, but the TypeScript signature creates a type-checking gap.Apply this diff:
- public set filter(value: string) { + public set filter(value: string | null | undefined) {
🧹 Nitpick comments (7)
src/blocks/CloudImageEditor/src/css/common.css (1)
1-1: Biome-ignore directive documents technical debt.The file-level
noImportantStylesignore acknowledges the extensive use of!importantrules (e.g., lines 60-63, 68-70, 75-77). The accompanying TODO signals this should be refactored in a future iteration. This is a reasonable pragmatic choice during a large migration, but consider scheduling a follow-up refactor to reduce reliance on!important.src/lit/LitBlock.ts (1)
319-325: Type cast bypasses type checking insubConfigValue.Line 324 uses
as anyto castsharedKey, which suppresses TypeScript's type checking. This could mask type mismatches between the shared state key and the expected callback parameter type.Consider improving the type definition to avoid the cast, or add a comment explaining why the cast is necessary.
src/lit/LitUploaderBlock.ts (2)
227-242: RedundantisConnectedcheck.The
_handleCollectionPropertiesUpdatemethod checks!this.isConnectedtwice (lines 228 and 242). The second check at line 242 is inside asetTimeout, so it's actually necessary to prevent stale callbacks.Consider adding a comment explaining why the second check is needed to clarify the intent.
449-450: Silent error handling for secure token.Line 450 silently catches all errors from
getSecureToken()without logging. This could make it difficult to debug secure upload configuration issues.Consider logging the error to help with troubleshooting:
- const secureToken = await this.secureUploadsManager.getSecureToken().catch(() => null); + const secureToken = await this.secureUploadsManager.getSecureToken().catch((err) => { + this.debugPrint('Failed to get secure token:', err); + return null; + });src/blocks/FileItem/FileItem.ts (1)
39-40: Empty string cast to brandedUidtype.The
uidproperty is initialized with an empty string cast toUidtype. SinceUidis a branded type, this cast undermines the type safety that the brand provides.Consider either:
- Making the property optional:
public uid?: Uid;- Or initializing with a proper Uid value if one is always available
src/blocks/CloudImageEditor/src/EditorFilterControl.ts (2)
191-243: Complex but correct preview loading logic.The
_loadPreviewmethod implements sophisticated request tracking, cancellation, and error handling. The request ID pattern (line 205), stale-request guards (lines 227, 239), and proper cleanup are all correct. However, the method's cognitive complexity is high.Consider extracting sub-methods to improve readability:
- Extract URL resolution (lines 207-213) into
_resolvePreviewUrl()- Extract preload setup (lines 215-223) into
_setupPreload(src, requestId)- Extract image loading (lines 225-243) into
_awaitPreload(promise, requestId, observer)This would make the control flow easier to follow while preserving the correctness.
245-280: Fallback visibility checking is sound but complex.The manual visibility check logic provides a robust fallback when IntersectionObserver is unavailable or not firing. The RAF + setTimeout pattern with 500ms retry is appropriate. However, the method's nesting and multiple early returns make it harder to follow.
Consider extracting:
- Lines 265-270: manual intersection check into a
_isElementVisible()helper- The RAF callback (lines 259-279) into a separate
_checkVisibility()methodThis would flatten the nesting and improve readability without changing behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/blocks/CloudImageEditor/src/EditorFilterControl.ts(3 hunks)src/blocks/CloudImageEditor/src/css/common.css(7 hunks)src/blocks/FileItem/FileItem.ts(10 hunks)src/lit/LitBlock.ts(1 hunks)src/lit/LitUploaderBlock.ts(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/blocks/CloudImageEditor/src/EditorFilterControl.ts (2)
src/blocks/CloudImageEditor/src/EditorSlider.ts (2)
FAKE_ORIGINAL_FILTER(12-12)cancel(96-99)src/blocks/CloudImageEditor/src/utils/parseFilterValue.ts (1)
parseFilterValue(4-13)
src/blocks/FileItem/FileItem.ts (4)
src/lit/Uid.ts (1)
Uid(1-3)src/index.ts (1)
LitActivityBlock(50-50)src/lit/LitActivityBlock.ts (1)
LitActivityBlock(16-196)src/abstract/uploadEntrySchema.ts (1)
UploadEntryTypedData(63-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
src/blocks/CloudImageEditor/src/css/common.css (6)
58-64: Focus pseudo-class selector fixed correctly.The change from
[focus-visible]attribute selector to:focus-visiblepseudo-class is a critical bug fix. The old selector would never match, breaking keyboard focus states for interactive elements. This fix ensures proper visual feedback for keyboard navigation and accessibility compliance.
11-11: Verify oklch() color syntax change doesn't affect rendering.The change from
oklch(0 0 0 / 0.1)tooklch(0% 0 0 / 0.1)properly formalizes the lightness parameter as a percentage. While both should render identically in modern browsers, please test the modal backdrop color to confirm no visual differences appeared.
234-242: Verify network splash z-index doesn't cause unintended stacking issues.The z-index increase from 4 to 999 ensures the network error overlay blocks interaction with the editor, which is correct behavior. However, test edge cases where other overlays (modals, popovers) might appear to ensure no unintended layering issues occur.
724-731: Deprecated CSS property updated correctly.The transition from
grid-gaptogapis a modern best practice that works with both grid and flexbox layouts. This change is safe and aligns with current CSS standards.
886-898: Tooltip z-index adjusted for proper visual hierarchy.The increase from z-index 3 to 10 ensures tooltips properly layer above toolbar controls while remaining below critical overlays. This creates a sensible stacking order: controls → tooltips (10) → network splash (999) → line loader (9999).
507-513: Filter control opacity rules appear correctly positioned.The opacity rules for active/inactive filter preview states are properly placed after the base
.uc-previewstyles (line 492). Verify there are no duplicate rules for these selectors elsewhere in the file or in related stylesheets.src/lit/LitUploaderBlock.ts (1)
245-298: Good defensive coding with context null checks.The consistent pattern of calling
PubSub.getCtx()followed by null checks (lines 245-246, 255-256, 267-268, 277-278, 296-297) is excellent defensive programming. This prevents null reference errors when entries are removed during iteration.src/blocks/FileItem/FileItem.ts (5)
273-279: Correct usage of Lit'swillUpdatelifecycle.The override properly detects
uidproperty changes and triggers entry rebinding. This is the correct Lit pattern for responding to property updates.
370-377: Good defensive handling of image shrink settings.The code properly validates the parsed shrink settings before attempting to shrink the image, logging a helpful warning if the settings are invalid. This prevents runtime errors from malformed configuration.
420-422: Excellent defensive check for stale entry references.The checks
if (entry === this.entry)after async operations prevent state updates for entries that are no longer current. This is crucial since theuidproperty could change while an upload is in progress.Also applies to: 448-450
456-461: Good performance optimization with lazy rendering.The
shouldUpdateoverride combined with the intersection observer (lines 320-323) implements lazy rendering for file items that aren't yet visible. This is an excellent performance optimization for large file lists.
84-98: No action required —withEntry()method is properly implemented in the base class.The
withEntry()method is defined inFileItemConfig(lines 11-21) and correctly handles the case wherethis.entrymight be undefined by returning a wrapper function that checks entry existence, logs a warning if missing, and returnsundefinedrather than proceeding with invalid data.src/blocks/CloudImageEditor/src/EditorFilterControl.ts (3)
117-121: IntersectionObserver feature detection is appropriate (responding to past comment).The condition
'IntersectionObserver' in windowis standard feature detection and serves as defensive programming for older browsers or environments where IntersectionObserver may not be available. The code includes a fallback mechanism (_schedulePreviewVisibilityCheck) that uses manual visibility checks with RAF and setTimeout. Removing this check would cause runtime errors in unsupported environments.
159-164: Thorough resource cleanup.The cleanup logic properly handles all lifecycle resources: IntersectionObserver disconnection, preload cancellation, and visibility check timers. This prevents memory leaks and ensures proper component teardown.
297-327: Clean Lit render implementation.The render method correctly uses Lit's template syntax and directives (
classMap,ifDefined,styleMap). The opacity-based transitions between preview and icon, accessibility attributes, and proper event binding all follow Lit best practices.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/abstract/managers/TelemetryManager.ts (1)
47-60: Fix logic error: Config updates are skipped when telemetry is disabled.The early return on lines 49-51 prevents the config from being updated via
_setConfig(line 58) when telemetry is disabled. This causes the manager to maintain stale config state, which could lead to incorrect telemetry data if telemetry is re-enabled later.The
_isEnabledguard should only prevent sending the telemetry event, not updating the internal config state.Apply this diff to fix the logic:
for (const key of Object.keys(this._config) as (keyof ConfigType)[]) { this._block.subConfigValue(key, (value) => { - if (!this._isEnabled) { - return; - } - if (this._initialized && this._config[key] !== value) { + if (this._initialized && this._config[key] !== value && this._isEnabled) { this.sendEvent({ eventType: InternalEventType.CHANGE_CONFIG, }); } this._setConfig(key, value); }); }
♻️ Duplicate comments (1)
src/lit/LitBlock.ts (1)
113-142: TelemetryManager initialization is not reactive to config changes.The
TelemetryManageris instantiated once duringinitCallback(line 125). Ifcfg.qualityInsightschanges at runtime, telemetry behavior won't update accordingly. Consider making the telemetry manager lazily created or reactive to config changes.Based on past review comment from egordidenko.
🧹 Nitpick comments (5)
demo/features/config-playground.html (3)
506-515: Replace legacyescape/unescapein base64 helpers
escape/unescapeare legacy APIs; useTextEncoder/TextDecoder(or at minimumencodeURIComponentwithoutescape/unescape).@@ - const encodeBase64Url = (value) => - btoa(unescape(encodeURIComponent(value))) + const encodeBase64Url = (value) => { + const bytes = new TextEncoder().encode(value); + let binary = ''; + for (const b of bytes) binary += String.fromCharCode(b); + return btoa(binary) .replace(/\+/g, '-') .replace(/\//g, '_') .replace(/=+$/g, ''); + }; const decodeBase64Url = (value) => { const padded = value.replace(/-/g, '+').replace(/_/g, '/').padEnd(Math.ceil(value.length / 4) * 4, '='); - return decodeURIComponent(escape(atob(padded))); + const binary = atob(padded); + const bytes = Uint8Array.from(binary, (c) => c.charCodeAt(0)); + return new TextDecoder().decode(bytes); };
592-621: URL persistence frequency could become noisy / exceed URL limits
Persisting on every_cfgchange (and also on_filter) can spamreplaceStateand create very large URLs (especially if config grows). Consider debouncing_persistStateToUrl()and/or skipping persistence while typing filter.Also applies to: 565-573
882-885: Button: settype="button"to avoid accidental submit behavior if embedded
Minor, but helps if this demo markup ever ends up inside a<form>.- <button class="pill" @click=${() => this._resetConfig()}>Reset to defaults</button> + <button type="button" class="pill" @click=${() => this._resetConfig()}>Reset to defaults</button>src/lit/LitBlock.ts (1)
310-316: Type assertions in subConfigValue.The
as anycasts at line 315 bypass type checking. While this works, consider if the types can be improved to avoid these casts.Consider using a more precise generic signature if possible, though this may require changes to the
submethod signature:// If sub() signature allows, something like: return this.sub(sharedKey, callback as (value: SharedState[typeof sharedKey]) => void);src/blocks/Thumb/Thumb.ts (1)
22-30:uiddefault + branded cast hides “unset” state; consider explicit empty handling
public uid: Uid = '' as Uid;relies on a cast for the empty/unset state. You already handle empty in_bindToEntry(), so it’s functionally OK, but consider making the “unset” value explicit (e.g.uid?: Uid) to avoid accidental downstream “valid uid” assumptions.Also applies to: 41-41
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
demo/features/config-playground.html(1 hunks)src/abstract/CTX.ts(3 hunks)src/abstract/managers/TelemetryManager.ts(5 hunks)src/blocks/Thumb/Thumb.ts(7 hunks)src/blocks/Thumb/thumb.css(1 hunks)src/lit/LitBlock.ts(1 hunks)src/lit/SharedState.ts(1 hunks)src/solutions/file-uploader/minimal/index.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lit/SharedState.ts
- src/blocks/Thumb/thumb.css
- src/solutions/file-uploader/minimal/index.css
🧰 Additional context used
🧬 Code graph analysis (3)
src/abstract/managers/TelemetryManager.ts (2)
src/lit/LitBlock.ts (1)
LitBlock(30-329)src/types/exported.ts (1)
ConfigType(40-334)
src/blocks/Thumb/Thumb.ts (3)
src/index.ts (1)
Thumb(28-28)src/blocks/FileItem/FileItemConfig.ts (1)
FileItemConfig(6-48)src/lit/Uid.ts (1)
Uid(1-3)
src/abstract/CTX.ts (1)
src/lit/LitBlock.ts (1)
LitBlock(30-329)
🪛 GitHub Check: CodeQL
demo/features/config-playground.html
[failure] 652-652: Remote property injection
A property name to write to depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (18)
src/abstract/CTX.ts (4)
1-6: LGTM on imports and base context.The imports are correctly updated to use
LitBlockfrom the new Lit-based architecture, andblockCtx()remains a clean empty object factory.
8-20: Clean migration to LitBlock-based activity context.The function signature change from
BlocktoLitBlockis correct. UsingfnCtx.pub('*currentActivity', null)aligns with the LitBlock's pub/sub API shown inLitBlock.ts. The optional chaining onfnCtx.modalManager?.close()is appropriate defensive coding.
22-31: Uploader context migration looks correct.The context properly extends
activityBlockCtxand initializes all required state fields. TheQueue(1)initialization for*uploadQueuemaintains the existing behavior for sequential upload processing.
33-36: Solution context correctly extends uploader context.Simple and clean extension pattern.
src/lit/LitBlock.ts (9)
1-24: Well-organized imports and interface definition.The imports are comprehensive and the
SharedContextInstanceinterface is appropriately minimal, requiring only an optionaldestroymethod for lifecycle management.
25-49: Class foundation looks solid.The mixin composition chain
RegisterableElementMixin(SymbioteMixin<SharedState>()(CssDataMixin(LightDomMixin(LitElement))))properly layers functionality. The private_sharedContextInstancesmap with typed structure for tracking persist flag and instance is well-designed.
51-71: Localization methods are well-implemented.The
l10nmethod handles template variables and plural forms correctly. The_pluralizemethod properly falls back to 'en' locale when no locale-id is found.
73-100: Event emission with telemetry integration.The
emitmethod correctly checks for the presence of*eventEmitterbefore attempting to emit. The lazy payload resolution (typeof payload === 'function' ? payload() : payload) is a nice pattern for deferred computation.
207-221: Shared context instance registration has a subtle early-return path.The method returns early at line 213 if
_sharedContextInstances.has(key), but it also returns (implicitly) at line 220 after registering. This is correct behavior but could be clearer. The check at line 215!this.has(key) || !this.$[key]ensures we only create if the key doesn't exist or is falsy in the shared context.
235-248: Type assertion in getSharedContextInstance return.The conditional return type
TRequired extends true ? NonNullable<SharedState[TKey]> : SharedState[TKey]is well-designed for type safety. The runtime behavior matches: throwing when required and not found, returning potentially-null value otherwise.
250-280: proxyUrl method handles edge cases well.Good warning when both
secureDeliveryProxyandsecureDeliveryProxyUrlResolverare set. The try-catch with fallback to original URL and telemetry error reporting is defensive and appropriate.
282-308: Config proxy provides lazy initialization.The Proxy-based approach for
cfgprovides on-demand registration of config keys in the shared context. Thesettrap returningfalsefor invalid keys is correct Proxy semantics (will throw in strict mode, which is desired for invalid config access).
318-328: Debug print with lazy evaluation.The pattern of accepting either direct args or a resolver function (
typeof args?.[0] === 'function') is efficient for avoiding computation when debug is disabled.src/abstract/managers/TelemetryManager.ts (3)
22-26: LGTM!The interface is well-defined and provides a clear contract for telemetry operations.
128-131: LGTM!The guard correctly prevents telemetry events from being sent when disabled.
156-167: LGTM!The public methods correctly delegate to
sendEvent, which handles the_isEnabledcheck, ensuring these operations respect the telemetry enablement state.Also applies to: 172-184
src/blocks/Thumb/Thumb.ts (2)
236-247: Entry binding lifecycle looks consistent (firstUpdated + uid change)
Calling_bindToEntry()infirstUpdated()and again whenuidchanges is the right shape, and thereset()before re-subscribing avoids subscription leaks.Also applies to: 275-309
266-273: Cleanup on reset/disconnect is solid (debounce + abort + observer)
Canceling the debounced generator, aborting pending decode/RAF work, and disconnecting the observer covers the key lifecycle hazards.Also applies to: 335-341
| _applyConfig() { | ||
| if (!this._configEl) return; | ||
| for (const [key, value] of Object.entries(this._cfg)) { | ||
| this._configEl[key] = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix URL-driven property injection (CodeQL) by allowlisting keys + blocking proto keys
parsed.cfg comes from the URL and its keys are later applied to uc-config via this._configEl[key] = value, which is exactly the “remote property injection” CodeQL is flagging. Please restrict keys to the known config surface (e.g., initialConfig + complex keys) and explicitly block __proto__/constructor/prototype.
@@
- _applyConfig() {
+ _applyConfig() {
if (!this._configEl) return;
- for (const [key, value] of Object.entries(this._cfg)) {
- this._configEl[key] = value;
- }
+ const allowed = new Set([...Object.keys(initialConfig), ...COMPLEX_KEYS]);
+ for (const [key, value] of Object.entries(this._cfg)) {
+ if (!allowed.has(key)) continue;
+ if (key === '__proto__' || key === 'constructor' || key === 'prototype') continue;
+ this._configEl[key] = value;
+ }
}
@@
- if (parsed.cfg && typeof parsed.cfg === 'object') {
- for (const [key, value] of Object.entries(parsed.cfg)) {
- cfg[key] = value;
- }
- }
+ if (parsed.cfg && typeof parsed.cfg === 'object') {
+ const allowed = new Set([...Object.keys(initialConfig), ...COMPLEX_KEYS]);
+ for (const [key, value] of Object.entries(parsed.cfg)) {
+ if (!allowed.has(key)) continue;
+ if (key === '__proto__' || key === 'constructor' || key === 'prototype') continue;
+ cfg[key] = value;
+ }
+ }Also applies to: 649-654, 656-665
🤖 Prompt for AI Agents
In demo/features/config-playground.html around lines 575-580 (and also update
the similar blocks at 649-654 and 656-665), the method currently assigns
URL-parsed keys directly to this._configEl which allows remote property
injection; restrict assignments by checking keys against a whitelist of known
safe config properties (e.g., "initialConfig" and any other legitimate top-level
config keys) and explicitly skip dangerous keys "__proto__", "constructor", and
"prototype"; implement the guard so only allowlisted keys are copied and
return/continue for any disallowed or prototype-related names.
| if (kind === 'number') { | ||
| return html` | ||
| <label class="control"> | ||
| <span class="label-text"> | ||
| <strong>${label}</strong> | ||
| <small>${value}</small> | ||
| </span> | ||
| <input | ||
| type="number" | ||
| .value=${String(value)} | ||
| @input=${(e) => this._updateValue(key, Number(e.target.value))} | ||
| /> | ||
| </label> | ||
| `; | ||
| } |
There was a problem hiding this comment.
Prevent NaN writes for number inputs
Number(e.target.value) will produce NaN for empty strings (and some intermediate states). Consider treating empty as null/undefined or skipping updates.
@@
- @input=${(e) => this._updateValue(key, Number(e.target.value))}
+ @input=${(e) => {
+ const raw = e.target.value;
+ if (raw === '') return;
+ const n = Number(raw);
+ if (Number.isNaN(n)) return;
+ this._updateValue(key, n);
+ }}📝 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.
| if (kind === 'number') { | |
| return html` | |
| <label class="control"> | |
| <span class="label-text"> | |
| <strong>${label}</strong> | |
| <small>${value}</small> | |
| </span> | |
| <input | |
| type="number" | |
| .value=${String(value)} | |
| @input=${(e) => this._updateValue(key, Number(e.target.value))} | |
| /> | |
| </label> | |
| `; | |
| } | |
| if (kind === 'number') { | |
| return html` | |
| <label class="control"> | |
| <span class="label-text"> | |
| <strong>${label}</strong> | |
| <small>${value}</small> | |
| </span> | |
| <input | |
| type="number" | |
| .value=${String(value)} | |
| @input=${(e) => { | |
| const raw = e.target.value; | |
| if (raw === '') return; | |
| const n = Number(raw); | |
| if (Number.isNaN(n)) return; | |
| this._updateValue(key, n); | |
| }} | |
| /> | |
| </label> | |
| `; | |
| } |
🤖 Prompt for AI Agents
In demo/features/config-playground.html around lines 695 to 709, the number
input handler uses Number(e.target.value) which can produce NaN for empty or
transient input states; change the handler to detect empty string (or
non-numeric) and either send null/undefined or skip calling _updateValue, e.g.
read e.target.value into a variable, if value === '' then call _updateValue(key,
null) or return early, otherwise convert to Number and only call _updateValue
when !isNaN(parsedNumber).
| } | ||
|
|
||
| export class TelemetryManager implements ITelemetryManager { | ||
| private readonly _sessionId: string = crypto.randomUUID(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the TelemetryManager file
find . -name "TelemetryManager.ts" -type fRepository: uploadcare/file-uploader
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Search for UID utility/class in the codebase
rg "class Uid|export.*Uid|UID\.generate" -t ts -t js --max-count 20Repository: uploadcare/file-uploader
Length of output: 417
🏁 Script executed:
#!/bin/bash
# Check the TelemetryManager file content around line 29
fd "TelemetryManager.ts" -x head -40 {}Repository: uploadcare/file-uploader
Length of output: 1755
🏁 Script executed:
#!/bin/bash
# Check imports in TelemetryManager and find UID.generate() implementation
rg "import.*UID|export.*generate" src/lit/Uid.ts -A 5 -B 2Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Get full UID implementation
cat src/lit/Uid.tsRepository: uploadcare/file-uploader
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Check if TelemetryManager imports UID
head -30 src/abstract/managers/TelemetryManager.ts | grep -i "import"Repository: uploadcare/file-uploader
Length of output: 674
🏁 Script executed:
#!/bin/bash
# Search for UID.generate() function definition
rg "generate\s*\(" --type ts -B 3 | grep -A 3 "UID\|generate" | head -50Repository: uploadcare/file-uploader
Length of output: 1092
🏁 Script executed:
#!/bin/bash
# Search more specifically for generate function
rg "export.*function.*generate|export const generate" --type tsRepository: uploadcare/file-uploader
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Look at files that use UID.generate()
fd "TypedData.ts" -x cat {}Repository: uploadcare/file-uploader
Length of output: 1410
🏁 Script executed:
#!/bin/bash
# Get the full UID.ts implementation
cat src/utils/UID.tsRepository: uploadcare/file-uploader
Length of output: 383
🏁 Script executed:
#!/bin/bash
# Check what crypto.randomUUID() actually returns and if there's a fallback pattern elsewhere
rg "crypto\.randomUUID|window\.crypto" --type ts -B 2 -A 2Repository: uploadcare/file-uploader
Length of output: 497
🏁 Script executed:
#!/bin/bash
# Check the package.json or similar to understand runtime environment/browser support
fd "package.json|tsconfig|\.browserslistrc" -x cat {} \; | head -100Repository: uploadcare/file-uploader
Length of output: 2556
🏁 Script executed:
#!/bin/bash
# Check if there are any try-catch patterns for crypto usage in the codebase
rg "try|catch.*crypto" --type ts -B 2 -A 2Repository: uploadcare/file-uploader
Length of output: 50381
🏁 Script executed:
#!/bin/bash
# Check if there are any other usages of crypto in the codebase
rg "crypto\." --type tsRepository: uploadcare/file-uploader
Length of output: 170
🏁 Script executed:
/#Check deployment/browser support documentation
fd "README|BROWSER|SUPPORT|\.md" -type f | head -10 | xargs cat | head -100Repository: uploadcare/file-uploader
Length of output: 292
Use UID.generate() instead of crypto.randomUUID() for broader environment support.
crypto.randomUUID() throws an error in non-secure contexts (non-HTTPS environments). The codebase already provides UID.generate() which generates UUIDs without relying on the crypto API and works in all environments. Import UID from '../utils/UID' and use it directly:
import { UID } from '../utils/UID';
private readonly _sessionId: string = UID.generate();This approach is consistent with how other parts of the codebase (TypedData, TypedCollection) generate context IDs.
🤖 Prompt for AI Agents
In src/abstract/managers/TelemetryManager.ts around line 29, replace the direct
use of crypto.randomUUID() with the project utility UID.generate() to avoid
failures in non-secure contexts: add an import for UID from '../utils/UID' at
the top of the file and set private readonly _sessionId to UID.generate();
ensuring consistency with other modules that create context IDs and removing
reliance on the Web Crypto API.
| public override connectedCallback(): void { | ||
| super.connectedCallback(); | ||
|
|
||
| this._observer?.disconnect(); | ||
| this._observer = new window.IntersectionObserver(this._observerCallback.bind(this), { threshold: 0.1 }); | ||
|
|
||
| this._observer.observe(this); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/blocks/Thumb/Thumb.ts | sed -n '320,340p'Repository: uploadcare/file-uploader
Length of output: 734
🏁 Script executed:
rg "_requestThumbGeneration" src/blocks/Thumb/Thumb.ts -A 5 -B 2Repository: uploadcare/file-uploader
Length of output: 1016
🏁 Script executed:
rg "IntersectionObserver" --type ts --type jsRepository: uploadcare/file-uploader
Length of output: 1695
🏁 Script executed:
head -100 src/blocks/Thumb/Thumb.ts | tail -50Repository: uploadcare/file-uploader
Length of output: 1916
🏁 Script executed:
fd "test|spec" --type f -e ts -e js | head -20Repository: uploadcare/file-uploader
Length of output: 827
🏁 Script executed:
fd "setup|setup.ts|test-setup|vitest|jest" --type f | head -10Repository: uploadcare/file-uploader
Length of output: 84
🏁 Script executed:
rg "IntersectionObserver|mock|stub" src --type ts --type js -l | grep -i testRepository: uploadcare/file-uploader
Length of output: 158
🏁 Script executed:
cat package.json | head -50Repository: uploadcare/file-uploader
Length of output: 1135
🏁 Script executed:
cat vitest.config.tsRepository: uploadcare/file-uploader
Length of output: 1653
🏁 Script executed:
rg "IntersectionObserver" src/blocks/CloudImageEditor/src/EditorFilterControl.ts -B 5 -A 10Repository: uploadcare/file-uploader
Length of output: 1959
Guard IntersectionObserver for non-browser/test environments
new window.IntersectionObserver(...) will throw in test environments using happy-dom, which lacks this API. Add a feature check and fall back to eager generation:
public override connectedCallback(): void {
super.connectedCallback();
this._observer?.disconnect();
- this._observer = new window.IntersectionObserver(this._observerCallback.bind(this), { threshold: 0.1 });
- this._observer.observe(this);
+ if ('IntersectionObserver' in window) {
+ this._observer = new window.IntersectionObserver(this._observerCallback.bind(this), { threshold: 0.1 });
+ this._observer.observe(this);
+ } else {
+ this._requestThumbGeneration(true);
+ }
}This pattern is already used successfully in EditorFilterControl.ts.
🤖 Prompt for AI Agents
In src/blocks/Thumb/Thumb.ts around lines 326 to 333, constructing new
window.IntersectionObserver directly will throw in non-browser/test environments
(happy-dom); add a feature check for window and IntersectionObserver before
creating and using the observer, and if unavailable fall back to the eager
thumbnail-generation path used in EditorFilterControl.ts (i.e. skip observer
setup and call the existing generation method immediately). Ensure you still
disconnect any existing observer when present.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/blocks/Modal/Modal.ts (2)
43-69:show()/hide()need null-safety (can throw if called before first ref assignment)
Right nowthis.dialogEl.value as ...will crash if the modal is opened before initial render or after disconnect.public show(): void { - const dialog = this.dialogEl.value as HTMLDialogElement & { + const dialog = this.dialogEl.value as (HTMLDialogElement & { showModal?: () => void; - }; + }) | null; + if (!dialog) return; @@ public hide(): void { - const dialog = this.dialogEl.value as HTMLDialogElement & { + const dialog = this.dialogEl.value as (HTMLDialogElement & { close?: () => void; - }; + }) | null; + if (!dialog) return;
104-120: Unregister modal indisconnectedCallback()to prevent memory leaksThe modal is registered in
initCallback()but never unregistered. Callthis.modalManager?.deleteModal(this.id)indisconnectedCallback()alongside the event unsubscribes. Otherwise, modals persist in ModalManager's_modalsMap indefinitely, causing memory leaks and preventing re-mounting with the same id.
♻️ Duplicate comments (1)
src/blocks/Modal/Modal.ts (1)
122-130: Dialog DOM listeners also need cleanup indisconnectedCallback()
Even if you fix ref-change cleanup, you should also detach listeners on disconnect and clear the ref to prevent leaks and accidental retention. (This is the same issue noted in the past review.)public override disconnectedCallback(): void { super.disconnectedCallback(); @@ + const dialog = this.dialogEl.value; + dialog?.removeEventListener('close', this._handleDialogClose); + dialog?.removeEventListener('mousedown', this._handleDialogMouseDown); + dialog?.removeEventListener('mouseup', this._handleDialogMouseUp); + this._dialogElWithListeners = null; + this.dialogEl.value = null; @@ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/blocks/Modal/Modal.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/blocks/Modal/Modal.ts (3)
src/abstract/managers/ModalManager.ts (2)
ModalId(14-14)ModalEvents(5-12)src/lit/LitBlock.ts (1)
LitBlock(30-329)src/blocks/UploadCtxProvider/EventEmitter.ts (1)
EventType(15-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
src/blocks/Modal/Modal.ts (1)
16-27: KeepdialogElref stable and remove listeners properly when disconnecting from the DOMReassigning
this.dialogElto a new object breaks any external code holding the original ref object (you mention it's "used in dashboard"). Instead, only update the.valueproperty. Additionally,_handleDialogRefadds event listeners without removing them from the previous element—this creates memory leaks and duplicate handlers if the ref callback fires multiple times during the component lifecycle. Add proper cleanup indisconnectedCallbackto remove all listeners attached in_handleDialogRef.@@ protected dialogEl = createRef<HTMLDialogElement>(); + private _previousDialogEl: HTMLDialogElement | null = null; @@ private _handleDialogRef(dialog: Element | undefined): void { - this.dialogEl = { value: dialog } as typeof this.dialogEl; - - this.dialogEl.value?.addEventListener('close', this._handleDialogClose); - this.dialogEl.value?.addEventListener('mousedown', this._handleDialogMouseDown); - this.dialogEl.value?.addEventListener('mouseup', this._handleDialogMouseUp); + // Detach listeners from previous element before switching + if (this._previousDialogEl) { + this._previousDialogEl.removeEventListener('close', this._handleDialogClose); + this._previousDialogEl.removeEventListener('mousedown', this._handleDialogMouseDown); + this._previousDialogEl.removeEventListener('mouseup', this._handleDialogMouseUp); + } + + this.dialogEl.value = (dialog as HTMLDialogElement | undefined) ?? null; + this._previousDialogEl = this.dialogEl.value; + + this.dialogEl.value?.addEventListener('close', this._handleDialogClose); + this.dialogEl.value?.addEventListener('mousedown', this._handleDialogMouseDown); + this.dialogEl.value?.addEventListener('mouseup', this._handleDialogMouseUp); } @@ disconnectedCallback(): void { super.disconnectedCallback(); + // Cleanup listeners attached in _handleDialogRef + if (this._previousDialogEl) { + this._previousDialogEl.removeEventListener('close', this._handleDialogClose); + this._previousDialogEl.removeEventListener('mousedown', this._handleDialogMouseDown); + this._previousDialogEl.removeEventListener('mouseup', this._handleDialogMouseUp); + } this.modalManager?.unregisterModal(this.id); }
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/abstract/managers/TelemetryManager.ts (2)
49-62: Don’t skip_setConfigupdates when telemetry is disabled (stale config after re-enable).
The early return at Lines 51-53 prevents_configfrom tracking changes while disabled; when telemetry is later enabled, INIT/CHANGE_CONFIG payloads may carry outdated config values. Prefer: always_setConfig, but only emitCHANGE_CONFIGwhen enabled.this._block.subConfigValue(key, (value) => { - if (!this._isEnabled) { - return; - } if (this._initialized && this._config[key] !== value) { - this.sendEvent({ - eventType: InternalEventType.CHANGE_CONFIG, - }); + if (this._isEnabled) { + this.sendEvent({ eventType: InternalEventType.CHANGE_CONFIG }); + } } this._setConfig(key, value); });
158-169: HardensendEventErrorfor non-Errorthrows.
(error as Error).messagewill often beundefined(or misleading) forstring,DOMException, custom objects, etc. Safer to derive a message defensively.public sendEventError(error: unknown, context = 'unknown'): void { + const message = + error instanceof Error ? error.message : typeof error === 'string' ? error : JSON.stringify(error); this.sendEvent({ eventType: InternalEventType.ERROR_EVENT, payload: { metadata: { event: 'error', text: `Error in ${context}`, - error: (error as Error).message, + error: message, }, }, }); }
♻️ Duplicate comments (4)
src/lit/LitBlock.ts (1)
182-191: Guard condition contains potentially confusing logic.The condition
if (this.isConnected || blocksRegistry?.size > 0)at line 184 includesthis.isConnectedwhich will typically befalsein this callback. While the check serves as a safety net for edge cases where the block is reconnected during thesetTimeout, it may be clearer to rely solely on the registry size check or add a comment explaining the intent.Consider simplifying or documenting:
setTimeout(() => { - if (this.isConnected || blocksRegistry?.size > 0) { + // Check if another block connected during the timeout + if (blocksRegistry?.size > 0) { return; }src/blocks/Thumb/Thumb.ts (1)
344-351: GuardIntersectionObserverfor non-browser/test environments.
new window.IntersectionObserver(...)will throw in test environments using happy-dom, which lacks this API. Add a feature check and fall back to eager generation. This pattern is already used inEditorFilterControl.ts.Apply this diff:
public override connectedCallback(): void { super.connectedCallback(); this._observer?.disconnect(); - this._observer = new window.IntersectionObserver(this._observerCallback.bind(this), { threshold: 0.1 }); - this._observer.observe(this); + if ('IntersectionObserver' in window) { + this._observer = new window.IntersectionObserver(this._observerCallback.bind(this), { threshold: 0.1 }); + this._observer.observe(this); + } else { + this._requestThumbGeneration(true); + } }src/blocks/Modal/Modal.ts (2)
20-26: Body scroll-lock is clobbered (overflow = '') even when this modal didn't set it.
closeDialog()anddisconnectedCallback()can override app-definedbody.style.overflowand/or conflict with other overlays. Consider tracking the prior value and only restoring when this modal applied the lock and no active modals remain.
132-138: Event listeners added but never removed from the dialog element.The
_handleDialogRefcallback adds event listeners to the dialog element but there's no corresponding cleanup. When the component is disconnected and reconnected, or if the dialog element changes, this could lead to duplicate listeners or memory leaks.Consider removing listeners in
disconnectedCallbackor tracking and removing old listeners when the ref changes:+ public override disconnectedCallback(): void { + super.disconnectedCallback(); + document.body.style.overflow = ''; + this._mouseDownTarget = undefined; + + // Remove dialog listeners + this.dialogEl.value?.removeEventListener('close', this._handleDialogClose); + this.dialogEl.value?.removeEventListener('mousedown', this._handleDialogMouseDown); + this.dialogEl.value?.removeEventListener('mouseup', this._handleDialogMouseUp); + + this.modalManager?.unsubscribe(ModalEvents.OPEN, this._handleModalOpen); + this.modalManager?.unsubscribe(ModalEvents.CLOSE, this._handleModalClose); + this.modalManager?.unsubscribe(ModalEvents.CLOSE_ALL, this._handleModalCloseAll); + }
🧹 Nitpick comments (1)
src/lit/LitBlock.ts (1)
310-316: Minor:anycasts insubConfigValue.The casts to
anyat line 315 bypass type safety. Consider improving the type definitions to avoid these casts.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.coderabbit.yaml(1 hunks)src/abstract/managers/TelemetryManager.ts(5 hunks)src/blocks/Modal/Modal.ts(6 hunks)src/blocks/Thumb/Thumb.ts(7 hunks)src/lit/LitBlock.ts(1 hunks)src/utils/UID.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .coderabbit.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/UID.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/blocks/Modal/Modal.ts (3)
src/abstract/managers/ModalManager.ts (3)
ModalId(14-14)ModalCb(15-15)ModalEvents(5-12)src/lit/LitBlock.ts (1)
LitBlock(30-329)src/blocks/UploadCtxProvider/EventEmitter.ts (1)
EventType(15-37)
src/lit/LitBlock.ts (7)
src/lit/SharedState.ts (1)
SharedState(108-115)src/abstract/managers/LocaleManager.ts (2)
localeStateKey(5-5)LocaleManager(8-54)src/blocks/UploadCtxProvider/EventEmitter.ts (1)
EventEmitter(69-125)src/utils/WindowHeightTracker.ts (1)
WindowHeightTracker(7-37)src/abstract/managers/ModalManager.ts (1)
ModalManager(18-171)src/abstract/managers/TelemetryManager.ts (2)
TelemetryManager(29-228)ITelemetryManager(23-27)src/lit/PubSubCompat.ts (1)
PubSub(7-91)
src/blocks/Thumb/Thumb.ts (2)
src/blocks/FileItem/FileItemConfig.ts (1)
FileItemConfig(6-48)src/lit/Uid.ts (1)
Uid(1-3)
🪛 GitHub Check: CodeQL
src/abstract/managers/TelemetryManager.ts
[failure] 30-30: Insecure randomness
This uses a cryptographically insecure random number generated at Math.random() in a security context.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (20)
src/abstract/managers/TelemetryManager.ts (5)
23-27:ITelemetryManageraddition looks good (clear contract).
No concerns with the shape; it should help the Lit migration keep call sites consistent.
37-47: Enablement initialization is now safe vs “send before subscription” concern.
Settingthis._isEnabled = Boolean(this._block.cfg.qualityInsights)before registering the subscription avoids the initial “leak” window noted previously.
130-156:sendEventgating is fine (cheap early exit).
The disabled check before formatting payload/queueing avoids unnecessary work and reduces accidental telemetry.
174-186: Cloud Image Editor telemetry shape looks consistent with the interface.
Defaultingoptionsto{}is a nice ergonomics improvement and keeps payload assembly straightforward.
8-10: No changes required;UID.generateRandomUUID()already implements crypto-first randomness with fallback.Line 30 uses
UID.generateRandomUUID(), which checks forglobalThis.crypto.randomUUID()(cryptographically secure) and falls back toMath.random()only when unavailable. This is the correct pattern. The CodeQL flag is likely a false positive due to static analysis not recognizing the crypto-first guard. The original suggestion to switch toUID.generate()would be a step backward, asgenerate()uses onlyMath.random()without crypto fallback.No code changes needed. If CodeQL requires suppression, document that telemetry session IDs are non-security-sensitive and use the already-crypto-first
generateRandomUUID()for additional robustness.src/blocks/Thumb/Thumb.ts (6)
15-19: Well-structured pending update type.The
PendingThumbUpdatetype cleanly encapsulates the abort controller, RAF ID, and cancel function needed for managing async thumbnail updates.
22-29: LGTM on the Lit decorators.Using
@propertyfor public reactive props (badgeIcon,uid) and@statefor internal reactive state (_thumbUrl) follows Lit best practices.
128-180: Solid image decoding implementation with proper cleanup.The
_decodeImagemethod correctly handles:
- AbortSignal for cancellation
- Cleanup that sets
image.srctoTRANSPARENT_PIXEL_SRC(avoiding the document URL fetch issue)- Nullifying event handlers and image reference
- Both modern
decode()API and fallbackonload/onerrorpaths
214-235: Decode failures now correctly prevent_thumbUrlupdate.The promise chain properly gates the
_thumbUrlupdate to only occur on successful decode. Failures are caught and logged without triggering the success path.
284-327: Clean entry binding implementation.The
_bindToEntrymethod properly:
- Handles empty/invalid
uidby callingreset()- Avoids re-binding to the same entry
- Sets up subscriptions for
fileInfo,thumbUrl, andcdnUrlModifiers- Triggers initial thumb generation with
force: true
361-370: Good use of fallback for empty_thumbUrl.Using
TRANSPARENT_PIXEL_SRCas a fallback and hiding the<img>when no thumbnail is available prevents broken image indicators and unnecessary network requests.src/blocks/Modal/Modal.ts (4)
11-12: LGTM on the Lit migration.Extending
LitBlockand using the staticstyleAttrspattern aligns with the broader migration strategy.
16-17: Comment typo fixed.The warning comment now correctly reads "Do not rename/change this".
71-102: Well-structured modal event handlers.The arrow function handlers (
_handleModalOpen,_handleModalClose,_handleModalCloseAll) properly maintainthisbinding and handle the modal lifecycle events correctly.
140-146: Clean render implementation.The render method properly uses Lit's
refdirective to bind the dialog element andyield('')for slot content projection.src/lit/LitBlock.ts (5)
29-30: Well-structured mixin composition.The mixin chain (
RegisterableElementMixin(SymbioteMixin<SharedState>()(CssDataMixin(LightDomMixin(LitElement))))) provides a clean separation of concerns for the base block functionality.
113-142: Solid shared context initialization.The
initCallbackproperly initializes all shared context instances with appropriate persistence flags. The subscription tolocale-idfor direction handling andtestModefor test ID attributes follows good patterns.
207-221: Good lazy initialization pattern for shared instances.The
addSharedContextInstancemethod correctly avoids creating duplicate instances and supports both persistent and non-persistent context instances.
282-308: Effective config proxy implementation.The Proxy-based config accessor provides clean reactive access to configuration values while ensuring defaults are properly initialized from
initialConfig.
46-49: TheTestModeControllerproperly self-registers with the host element via Lit's controller lifecycle pattern. In its constructor, it callsthis._host.addController(this), which registers itself with the host. This is the standard Lit controller pattern, and the reference does not need to be stored. Lit automatically manages the controller's lifecycle, including cleanup via thehostDisconnected()method when the host is disconnected. The current implementation is correct.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/abstract/TypedCollection.ts (1)
1-70: Clear timeouts on teardown to avoid post-destroy callbacks.
_observeTimeout/_notifyTimeoutare scheduled onwindowbut never cleared indestroy(). Ifdestroy()runs before the timeout fires, queued callbacks can run against partially-torn-down state.Suggested addition inside
destroy():+ if (this._observeTimeout) window.clearTimeout(this._observeTimeout); + if (this._notifyTimeout) window.clearTimeout(this._notifyTimeout);
♻️ Duplicate comments (3)
src/blocks/CloudImageEditor/src/CropFrame.ts (1)
239-265: Memory leak: pointerdown listeners not cleaned up.The past review correctly identified that line 264 adds a
pointerdownlistener with.bind(this, direction), creating an anonymous bound function that cannot be removed indisconnectedCallback(lines 602-609). This creates a memory leak when the component is destroyed and recreated.As noted in the previous review, store the bound handler reference (e.g., in a Map keyed by direction or as properties on the thumb object) during
_createThumb, then remove these stored references indisconnectedCallback.src/blocks/CloudImageEditor/src/EditorFilterControl.ts (1)
38-56:filtersetter param type should acceptnull | undefined(Lit can pass those)Setter uses
value ?? '', but the signature isstring, so TS doesn’t match runtime. This was raised before; looks still unresolved.- public set filter(value: string) { + public set filter(value: string | null | undefined) { const nextFilter = value ?? ''; if (this._filter === nextFilter) { return; }src/blocks/CloudImageEditor/src/elements/button/BtnUi.ts (1)
32-48:reverse/themeupdates: likely no-op for layout +classNameclobbers classes and doesn’t clear old theme.
this.style.flexDirectionaffects the host, not the inner<button>; unless your CSS intentionally relies on host flexing the button contents,reversewon’t actually flip icon/text. (This seems related to the earlier note that the prop wasn’t impacting styles.)this.className = ...overwrites any existing classes and leaves stale theme classes behind whenthemechanges to'custom'/null/''.Proposed fix (track and toggle only what you own, and target the right element for
reverse):export class BtnUi extends LitBlock { + private _appliedThemeClass?: string; private _applyReverse(): void { - this.style.flexDirection = this.reverse ? 'row-reverse' : ''; + const btn = this.renderRoot?.querySelector('button'); + if (!btn) return; + btn.style.flexDirection = this.reverse ? 'row-reverse' : ''; } private _applyThemeClass(): void { - if (this.theme && this.theme !== 'custom') { - this.className = `uc-${this.theme}`; - } + if (this._appliedThemeClass) { + this.classList.remove(this._appliedThemeClass); + this._appliedThemeClass = undefined; + } + const next = this.theme && this.theme !== 'custom' ? `uc-${this.theme}` : undefined; + if (next) { + this.classList.add(next); + this._appliedThemeClass = next; + } }Also applies to: 50-58
🧹 Nitpick comments (12)
src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts (3)
14-18: AlignEditorTemplateConfigtypes withifDefined()semantics (optional vs empty-string).
Right nowcropPreset/tabsare typed asstring, but rendered viaifDefined(), which is typically used to omit attributes when values areundefined. If these cfg values can be absent/empty, consider reflecting that in the type and normalization.type EditorTemplateConfig = { cdnUrl: string; - cropPreset: string; - tabs: string; + cropPreset?: string; + tabs?: string; }; private _createEditorConfig(cdnUrl: string): EditorTemplateConfig { const config: EditorTemplateConfig = { cdnUrl, - cropPreset: this.cfg.cropPreset, - tabs: this.cfg.cloudImageEditorTabs, + cropPreset: this.cfg.cropPreset || undefined, + tabs: this.cfg.cloudImageEditorTabs || undefined, }; return config; }Also applies to: 121-128, 135-142
37-70: Nice: state-driven config updates trigger re-render; consider deduping the update pattern.
ThesubConfigValue()handlers are basically identical (guard → equality check → spread update). A tiny helper would reduce repetition and future drift.+ private _patchEditorConfig(patch: Partial<EditorTemplateConfig>): void { + if (!this._editorConfig) return; + this._editorConfig = { ...this._editorConfig, ...patch }; + } this.subConfigValue('cropPreset', (cropPreset) => { - if (!this._editorConfig) { - return; - } - if (this._editorConfig.cropPreset === cropPreset) { - return; - } - this._editorConfig = { - ...this._editorConfig, - cropPreset, - }; + if (this._editorConfig?.cropPreset === cropPreset) return; + this._patchEditorConfig({ cropPreset }); }); this.subConfigValue('cloudImageEditorTabs', (tabs) => { - if (!this._editorConfig) { - return; - } - if (this._editorConfig.tabs === tabs) { - return; - } - this._editorConfig = { - ...this._editorConfig, - tabs, - }; + if (this._editorConfig?.tabs === tabs) return; + this._patchEditorConfig({ tabs }); });
72-95: Consider makinghandleChangeprivate unless it’s part of a public API contract.
It’s currently only used as an internal event handler and logs debug output.(If you keep it public intentionally, no change needed.)
src/utils/UID.ts (2)
5-7: Consider collision risk in high-frequency scenarios.The
generateFastUid()implementation usesDate.now()(millisecond precision) andMath.random()(non-cryptographic). Multiple UID generations within the same millisecond could theoretically collide, though the 9-character random suffix significantly reduces this risk. This is acceptable for the "fast" UID use case, but for high-volume scenarios with many UIDs generated per millisecond, consider adding a counter or usinggenerateRandomUUID()instead.
8-14: Note return type and format inconsistency.The
generateRandomUUID()method returns:
- A standard UUID (8-4-4-4-12 format) when crypto.randomUUID is available
- A custom
uid-{timestamp}-{random}format when falling back togenerateFastUid()Additionally, the return type is
stringwhilegenerateFastUid()returnsUid(branded string). If callers only need a unique identifier string, this is acceptable. However, if format consistency or the brandedUidtype matters, consider either:
- Renaming to clarify it returns varying formats (e.g.,
generateUniqueId)- Always returning the custom format
- Returning
Uidtype for consistency withgenerateFastUid()src/blocks/CloudImageEditor/src/elements/slider/SliderUi.ts (1)
204-229: Consider optional enhancements for value handling.Two minor observations:
_setCurrentValue (lines 204-213): The method doesn't clamp values to
min/maxbounds. While the HTMLinput[type=range]automatically clamps user input, adding explicit bounds checking would make the method more defensive if called programmatically._extractEventValue (line 227): Using
parseInttruncates decimal values. The current implementation is consistent with integer-only sliders (nostepattribute in the render), but if decimal precision is needed in the future, this would need to change toparseFloat.If you want to add bounds clamping:
private _setCurrentValue(value: number): void { if (!Number.isFinite(value)) { return; } + const clampedValue = Math.max(this.min, Math.min(this.max, value)); - this._currentValue = value; + this._currentValue = clampedValue; if (this.hasUpdated) { - this._syncInputValue(value); - this._updateValue(value); + this._syncInputValue(clampedValue); + this._updateValue(clampedValue); } }src/abstract/TypedData.ts (1)
20-47:getValue()warns but still returns “missing key” reads; consider a safer contract.Right now a missing key results in warnings and
read()returningundefinedanyway. Either (a) throw, (b) returnundefinedearly, or (c) make it explicit in the return type for missing keys.Example (early-return
undefined):- public getValue<K extends keyof T>(prop: K): T[K] { + public getValue<K extends keyof T>(prop: K): T[K] | undefined { if (!this._data.has(prop)) { console.warn(`${MSG_NAME}${String(prop)}`); + return undefined; } return this._data.read(prop); }src/abstract/TypedCollection.ts (1)
156-160:clearAll()currently triggers N notifies; consider batching (optional).Given
_notify()is debounced, this is probably fine, but you can avoid repeatedpub(id, undefined)+ repeated_notify()calls by doing a single batch clear.src/blocks/CloudImageEditor/src/EditorFilterControl.ts (3)
76-103: Guard against empty_operation/_filtergenerating invalid transformationsIf
_loadPreview()runs beforefilteris set,_previewSrc()will writetransformations[''] = { name: '', amount: 100 }, producing a potentially invalid CDN URL. Cheap safety: bail out (or treat as “original”) when_operation/_filteris unset.private _previewSrc(): string { + if (!this._operation || !this._filter) { + return createCdnUrl( + this._originalUrl, + createCdnUrlModifiers( + COMMON_OPERATIONS, + transformationsToOperations(this.$['*editorTransformations'] as Transformations), + `@clib/${PACKAGE_NAME}/${PACKAGE_VERSION}/uc-cloud-image-editor/`, + ), + ); + } const cssSize = parseInt(window.getComputedStyle(this).getPropertyValue('--l-base-min-width'), 10);
243-289: Visibility polling every 500ms can get expensive with many controlsThe RAF +
setTimeout(500)loop per control can add up. If IntersectionObserver is reliable enough, consider removing polling; otherwise consider exponential backoff and/or stop after N tries until something changes (e.g.,*originalUrl,isOriginal,*networkProblems).
308-324: A11y:role="option"likely needsaria-selectedIf this button is intended to behave like an option in a listbox, expose selection state (
this.active) viaaria-selected=${String(this.active)}to match ARIA patterns.src/blocks/CloudImageEditor/src/elements/button/BtnUi.ts (1)
11-31:aria-roleattribute is non-standard; considerrolemapping (or document why it’s needed).
Today you acceptaria-roleand then write it to the realroleattribute inrender(). That’s workable, but surprising API-wise; if this isn’t required for backward compatibility, prefer@property({ attribute: 'role' }) public role = ''(or similar) and bind from that.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
src/abstract/TypedCollection.ts(3 hunks)src/abstract/TypedData.ts(1 hunks)src/blocks/CloudImageEditor/src/CropFrame.ts(15 hunks)src/blocks/CloudImageEditor/src/EditorFilterControl.ts(3 hunks)src/blocks/CloudImageEditor/src/elements/button/BtnUi.ts(1 hunks)src/blocks/CloudImageEditor/src/elements/slider/SliderUi.ts(3 hunks)src/blocks/CloudImageEditor/src/lib/parseCropPreset.test.ts(1 hunks)src/blocks/CloudImageEditor/src/lib/parseCropPreset.ts(2 hunks)src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts(2 hunks)src/utils/UID.test.ts(1 hunks)src/utils/UID.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/blocks/CloudImageEditor/src/lib/parseCropPreset.test.ts
🧰 Additional context used
🧬 Code graph analysis (7)
src/utils/UID.test.ts (1)
src/utils/UID.ts (1)
UID(4-15)
src/blocks/CloudImageEditor/src/lib/parseCropPreset.ts (2)
src/utils/UID.ts (1)
UID(4-15)src/index.ts (1)
UID(7-7)
src/blocks/CloudImageEditor/src/elements/slider/SliderUi.ts (3)
src/lit/LitBlock.ts (1)
LitBlock(30-329)src/index.ts (1)
LitBlock(47-47)src/abstract/managers/a11y.ts (1)
CustomEvent(41-43)
src/blocks/CloudImageEditor/src/EditorFilterControl.ts (3)
src/blocks/CloudImageEditor/src/EditorSlider.ts (2)
FAKE_ORIGINAL_FILTER(12-12)cancel(96-99)src/blocks/CloudImageEditor/src/utils/parseFilterValue.ts (1)
parseFilterValue(4-13)src/abstract/managers/a11y.ts (1)
document(45-47)
src/utils/UID.ts (2)
src/index.ts (1)
UID(7-7)src/lit/Uid.ts (1)
Uid(1-3)
src/abstract/TypedData.ts (4)
src/lit/Uid.ts (1)
Uid(1-3)src/lit/PubSubCompat.ts (1)
PubSub(7-91)src/index.ts (2)
PubSub(5-5)UID(7-7)src/utils/UID.ts (1)
UID(4-15)
src/blocks/CloudImageEditor/src/CropFrame.ts (2)
src/lit/LitBlock.ts (1)
LitBlock(30-329)src/utils/UID.ts (1)
UID(4-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (31)
src/blocks/CloudImageEditorActivity/CloudImageEditorActivity.ts (3)
1-10: Imports look consistent with Lit render/state migration.
97-109: AvoidinternalId as Uidif a runtime conversion/validator exists.
IfUidis a branded type, the cast is fine for TS but doesn’t protect runtime; a proper converter would make this safer and self-documenting.- const entry = this.uploadCollection.read(internalId as Uid); + // Prefer a canonical Uid parser/validator if available in the codebase. + const entry = this.uploadCollection.read(internalId as Uid);If there’s a helper like
toUid(internalId)/Uid.parse(internalId), I’d recommend switching to it here.
111-133: Mount/unmount + render gating via_editorConfiglooks clean and Lit-idiomatic.
Resetting_entryand_editorConfigon deactivate is a good, explicit teardown.src/blocks/CloudImageEditor/src/CropFrame.ts (7)
1-57: LGTM! Clean Lit migration setup.The Lit imports, LitBlock extension, UID-based mask ID generation, ref-based SVG access, and deferred initialization pattern are all well-structured for the migration.
59-83: LGTM! Well-encapsulated dragging state.The getter/setter pattern with
_applyGuidesDragState()ensures guides visibility classes are consistently updated whenever the dragging state changes.
100-143: LGTM! Comprehensive SVG availability guards.The backdrop and frame creation methods properly guard all SVG operations with availability checks, and the
_applyGuidesDragState()call at line 349 ensures the initial guides visibility is correct.Also applies to: 171-237, 331-350
352-411: LGTM! Arrow function handlers enable proper cleanup.The arrow function declarations for
_handlePointerUp,_handlePointerMove, and_handleSvgPointerMovemaintain stable references that are correctly removed indisconnectedCallback. SVG availability guards prevent operations when the element isn't ready.Also applies to: 436-469
471-496: LGTM! Deferred mask creation handles SVG readiness.The
_pendingMaskHrefpattern (lines 477-481) defers mask creation until the SVG is ready, preventing errors when the mask is requested via config subscription beforefirstUpdated. Lines 593-597 properly apply the pending mask once the SVG initializes.
515-522: LGTM! Comprehensive SVG readiness guards.The
_svgReadyguards at lines 516-518, 546-548, and 560-562 prevent rendering operations before SVG initialization completes infirstUpdated. The defensive double-check pattern is safe.Also applies to: 544-566
541-576: LGTM! Lifecycle hooks properly structured for Lit.The lifecycle follows correct Lit patterns:
initCallback()sets up subscriptions and document-level listenersfirstUpdated()initializes SVG structure one-timedisconnectedCallback()cleans up arrow function listenersrender()returns the SVG template with refThe only cleanup gap is the
pointerdownlisteners, already flagged separately.Also applies to: 578-600, 602-614
src/blocks/CloudImageEditor/src/lib/parseCropPreset.ts (2)
3-3: LGTM! Clean migration to local UID utility.The import migration from the external Symbiote package to the local UID utility aligns with the PR's goal of migrating to Lit Element.
32-32: Migration toUID.generateFastUid()is complete.The change from
UID.generate()toUID.generateFastUid()is confirmed complete across the codebase with no remaining references to the old API. The new method name is more explicit about the generation strategy.src/utils/UID.test.ts (3)
4-17: Good test coverage for generateFastUid.The regex pattern correctly validates the UID format, and the tests verify both format compliance and uniqueness across consecutive calls.
19-33: Well-structured test for crypto.randomUUID usage.The test properly handles environments with and without crypto.randomUUID, uses spies to verify invocation, and cleans up mocks appropriately.
35-58: Thorough fallback test with proper cleanup.The test correctly validates the fallback behavior when crypto.randomUUID is unavailable. The global state manipulation is handled safely with proper restoration in the finally block and graceful handling of non-configurable crypto objects.
src/utils/UID.ts (1)
3-4: Justified use of static-only class pattern.The biome-ignore is appropriate here since the static-only class maintains API compatibility with Symbiote's UID during the migration.
src/blocks/CloudImageEditor/src/elements/slider/SliderUi.ts (8)
1-6: LGTM! Clean Lit migration foundation.The imports and class declaration properly establish the Lit-based component structure, extending LitBlock and importing necessary Lit utilities.
11-31: LGTM! Proper Lit property declarations.The refs and property decorators follow Lit best practices, with appropriate reflection settings and sensible defaults.
38-65: Verify: Missing 'slider-change' event emission.The
_handleSliderChangehandler extracts and updates the value but doesn't emit a corresponding 'slider-change' event, unlike_handleSliderInputwhich emits 'slider-input'. HTML range inputs fire both 'input' (during interaction) and 'change' (on release) events, and consumers often differentiate between intermediate values and final committed values.Is the absence of 'slider-change' emission intentional, or should the handler emit a change event?
If 'slider-change' emission is needed, consider:
- private _emitSliderEvent(type: 'slider-input', value: number): void { + private _emitSliderEvent(type: 'slider-input' | 'slider-change', value: number): void { this.dispatchEvent( new CustomEvent(type, { detail: { value }, bubbles: true, composed: true, }), ); } private readonly _handleSliderChange = (event: Event): void => { event.stopPropagation(); const value = this._extractEventValue(event); if (value === null) { return; } this._setCurrentValue(value); + this._emitSliderEvent('slider-change', value); };
67-73: LGTM! Focus styling handlers.The focus/blur handlers correctly manage visual feedback via CSS custom properties.
75-113: LGTM! Proper Lit lifecycle integration.The
firstUpdatedandwillUpdatehooks correctly initialize the component and respond to property changes, with appropriate guards (hasUpdated) to prevent premature updates.
115-193: LGTM! Robust update methods with proper guards.The update methods include division-by-zero checks, null-safe ref access, and use
requestAnimationFramefor DOM updates. The step rendering efficiently uses document fragments.
195-202: LGTM! Proper resource cleanup.The
disconnectedCallbackcorrectly removes event listeners and disconnects the ResizeObserver, preventing memory leaks.
231-247: LGTM! Clean Lit template rendering.The render method correctly uses Lit's html template syntax, ref directives, property bindings, and event bindings. All patterns follow Lit best practices.
src/abstract/TypedData.ts (1)
20-36:setValue()“only publish on change” behavior is good.Avoiding redundant publishes will help keep subscription churn down.
src/abstract/TypedCollection.ts (3)
71-101: Debounced notify logic looks reasonable.The
setTimeoutdebouncing and copying ofadded/removedsets before clearing is a solid pattern.
129-143: Throwing on missing items inreadProp/publishPropis good.The error messages are specific and should help diagnose invalid ids quickly.
162-183:findItems()is fine; consider returning ids only (as you do) to avoid exposing internals.src/blocks/CloudImageEditor/src/EditorFilterControl.ts (2)
114-155: No action required. Subscriptions created viasub()are automatically tracked and cleaned up by the SymbioteMixin framework. The mixin stores all unsubscriber functions in_symbioteSubscriptionsand automatically invokes them duringdisconnectedCallback(). SinceEditorFilterControlproperly callssuper.disconnectedCallback(), all subscriptions registered ininitCallback()are automatically cleaned up when the element disconnects—there is no subscription leak.
58-74: Use declarative event binding to preservethiscontextStoring an unbound method reference outside the template loses the
thiscontext when the handler is invoked. TheonClickmethod accessesthis.$[...]andthis.telemetryManager, which requires the correct binding.Change from:
public override render() { - const clickHandler = this.onClick; ... @click=${clickHandler}To:
public override render() { ... @click=${this.onClick}Or bind once using a class field:
+ private readonly _onClick = (e: MouseEvent) => this.onClick(e);This affects
EditorButtonControl.ts,EditorFilterControl.ts, andEditorAspectRatioButtonControl.ts.Likely an incorrect or invalid review comment.
src/blocks/CloudImageEditor/src/elements/button/BtnUi.ts (1)
60-77: Icon class/visibility derivation looks consistent.
The_iconClassMap+_computedIconHidden/_computedIconSinglesplit is clear and keepsrender()tidy.
| public read(id: Uid): TypedData<T> | null { | ||
| return this._data.read(id) as TypedData<T> | null; | ||
| } |
There was a problem hiding this comment.
read() return type is misleading (null vs undefined).
PubSub.read() will return undefined when a key is missing. Returning TypedData<T> | null invites incorrect checks.
- public read(id: Uid): TypedData<T> | null {
- return this._data.read(id) as TypedData<T> | null;
+ public read(id: Uid): TypedData<T> | undefined {
+ return this._data.read(id) as TypedData<T> | undefined;
}🤖 Prompt for AI Agents
In src/abstract/TypedCollection.ts around lines 125 to 127, the read() method
currently declares a return type of TypedData<T> | null while underlying
PubSub.read() returns undefined for missing keys; update the method signature to
return TypedData<T> | undefined and change the cast/return accordingly so it
passes through the undefined value from this._data.read(id) rather than coercing
to null.
| import { PubSub } from '../lit/PubSubCompat'; | ||
| import type { Uid } from '../lit/Uid'; | ||
| import { UID } from '../utils/UID'; | ||
|
|
||
| const MSG_NAME = '[Typed State] Wrong property name: '; | ||
| const MSG_TYPE = '[Typed State] Wrong property type: '; | ||
|
|
||
| export type TypedSchema = Record<string, { type: unknown; value: unknown; nullable?: boolean }>; | ||
| export class TypedData<T extends Record<string, unknown>> { | ||
| private _ctxId: Uid; | ||
| private _data: PubSub<T>; | ||
|
|
||
| export type Constructor<T = any> = new (...args: any[]) => T; | ||
|
|
||
| export type ExtractType<T, V> = T extends StringConstructor | ||
| ? string | ||
| : T extends BooleanConstructor | ||
| ? boolean | ||
| : T extends NumberConstructor | ||
| ? number | ||
| : T extends ArrayConstructor | ||
| ? V | ||
| : T extends Constructor | ||
| ? InstanceType<T> | ||
| : T; | ||
|
|
||
| export type ExtractDataFromSchema<T extends TypedSchema> = { | ||
| [K in keyof T]: ExtractType<T[K]['type'], T[K]['value']> | (T[K]['nullable'] extends true ? null : never); | ||
| }; | ||
|
|
||
| export type ExtractKeysFromSchema<T extends TypedSchema> = Extract<keyof T, string>; | ||
|
|
||
| export class TypedData<T extends TypedSchema> { | ||
| private __typedSchema: T; | ||
| private __ctxId: string; | ||
| private __schema: ExtractDataFromSchema<T>; | ||
| private __data: Data; | ||
|
|
||
| constructor(typedSchema: T, ctxName?: string) { | ||
| this.__typedSchema = typedSchema; | ||
| this.__ctxId = ctxName || UID.generate(); | ||
|
|
||
| this.__schema = Object.keys(typedSchema).reduce( | ||
| (acc, key) => { | ||
| if (typedSchema[key]) { | ||
| (acc as Record<string, unknown>)[key] = typedSchema[key].value; | ||
| } | ||
| return acc; | ||
| }, | ||
| {} as ExtractDataFromSchema<T>, | ||
| ); | ||
| this.__data = Data.registerCtx(this.__schema, this.__ctxId); | ||
| public constructor(initialValue: T) { | ||
| this._ctxId = UID.generateFastUid(); | ||
| this._data = PubSub.registerCtx(initialValue, this._ctxId); | ||
| } | ||
|
|
||
| get uid(): string { | ||
| return this.__ctxId; | ||
| public get uid(): Uid { | ||
| return this._ctxId; | ||
| } |
There was a problem hiding this comment.
Consider a stronger ctx id generator (collision → runtime throw).
PubSub.registerCtx() throws if a ctx id already exists; UID.generateFastUid() is time/random based and can collide under high churn. Consider switching _ctxId generation to a UUID-based source when available.
- this._ctxId = UID.generateFastUid();
+ this._ctxId = UID.generateRandomUUID() as Uid;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/abstract/TypedData.ts lines 1-18, the current ctx id uses
UID.generateFastUid() which can collide and cause PubSub.registerCtx() to throw;
replace the single generate call with a robust approach: either switch to a
UUIDv4 (or other cryptographically strong) generator from your utilities and use
that for _ctxId, or implement a short retry loop that generates a new id and
re-attempts PubSub.registerCtx(initialValue, ctxId) until it succeeds (with a
bounded retry count and a clear runtime error if exhausted). Ensure the new code
sets this._ctxId to the successful id and preserves existing behavior if
registerCtx() succeeds immediately.
| private async _loadPreview(observer?: IntersectionObserver): Promise<void> { | ||
| if (!this.isConnected) { | ||
| observer?.unobserve(this); | ||
| this._cancelPreload?.(); | ||
| this._cancelPreload = undefined; | ||
| return; | ||
| } | ||
|
|
||
| if (!this._originalUrl) { | ||
| if (!this._previewVisibilityCheckTimeout && !this._previewVisibilityCheckRaf) { | ||
| this._schedulePreviewVisibilityCheck(); | ||
| } | ||
| return; | ||
| } | ||
| const requestId = ++this._lastPreviewRequestId; | ||
| let src = ''; | ||
| try { | ||
| src = await this.proxyUrl(this._previewSrc()); | ||
| } catch (err) { | ||
| this.$['*networkProblems'] = true; | ||
| console.error('Failed to resolve preview URL', { error: err }); | ||
| return; | ||
| } | ||
|
|
||
| this._previewLoaded = false; | ||
| this._cancelPreload?.(); | ||
| const { promise, cancel } = preloadImage(src); | ||
| this._cancelPreload = () => { | ||
| cancel(); | ||
| if (this._lastPreviewRequestId === requestId) { | ||
| this._cancelPreload = undefined; | ||
| } | ||
| }; | ||
|
|
||
| this.sub('active', (active: boolean) => { | ||
| if (this.$.isOriginal) { | ||
| try { | ||
| await promise; | ||
| if (this._lastPreviewRequestId !== requestId || !this.isConnected) { | ||
| return; | ||
| } | ||
| const iconEl = this.ref['icon-el'] as HTMLElement; | ||
| iconEl.style.opacity = active ? '1' : '0'; | ||
|
|
||
| const previewEl = this.ref['preview-el'] as HTMLElement; | ||
| if (active) { | ||
| previewEl.style.opacity = '0'; | ||
| } else if (previewEl.style.backgroundImage) { | ||
| previewEl.style.opacity = '1'; | ||
| this._previewImage = src; | ||
| this._previewLoaded = true; | ||
| this._clearPreviewVisibilityChecks(); | ||
| (observer ?? this._observer)?.unobserve(this); | ||
| } catch (err) { | ||
| this.$['*networkProblems'] = true; | ||
| console.error('Failed to load image', { error: err }); | ||
| this._schedulePreviewVisibilityCheck(); | ||
| } finally { | ||
| if (this._lastPreviewRequestId === requestId) { | ||
| this._cancelPreload = undefined; | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Race/perf: stale proxyUrl() resolutions can still start preloads
You increment requestId before await this.proxyUrl(...), but if a newer request starts while awaiting, this older continuation can still kick off a preload (wasted network). Add a requestId check immediately after proxyUrl() resolves (and also before mutating _previewLoaded).
const requestId = ++this._lastPreviewRequestId;
let src = '';
try {
src = await this.proxyUrl(this._previewSrc());
+ if (this._lastPreviewRequestId !== requestId || !this.isConnected) {
+ return;
+ }
} catch (err) {
this.$['*networkProblems'] = true;
console.error('Failed to resolve preview URL', { error: err });
return;
}
- this._previewLoaded = false;
+ this._previewLoaded = false;
this._cancelPreload?.();🤖 Prompt for AI Agents
In src/blocks/CloudImageEditor/src/EditorFilterControl.ts around lines 189 to
241, the code increments requestId before awaiting proxyUrl, allowing a stale
resolution to continue and start a preload; add a guard that compares the
captured requestId to this._lastPreviewRequestId immediately after await
this.proxyUrl(...) and return early if they differ (so no preload or state
changes occur), and also perform the same requestId check right before mutating
_previewLoaded/_previewImage; ensure any cancel/cleanup logic only runs when the
requestId still matches to avoid leaking preloads.
| public override render() { | ||
| return html` | ||
| <button | ||
| type="button" | ||
| role=${ifDefined(this.ariaRole || undefined)} | ||
| aria-controls=${ifDefined(this.ariaControls || undefined)} | ||
| aria-label=${ifDefined(this.l10n(this.titleProp))} | ||
| title=${ifDefined(this.l10n(this.titleProp))} | ||
| > | ||
| <uc-icon | ||
| class=${classMap(this._iconClassMap)} | ||
| name=${ifDefined(this.icon || undefined)} | ||
| ?hidden=${this._computedIconHidden} | ||
| ></uc-icon> | ||
| <div class="uc-text">${this.text}</div> | ||
| </button> | ||
| `; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full BtnUi.ts file
cat -n src/blocks/CloudImageEditor/src/elements/button/BtnUi.tsRepository: uploadcare/file-uploader
Length of output: 3290
🏁 Script executed:
# Find the LitBlock class
fd -e ts -e js -p LitBlock | head -20Repository: uploadcare/file-uploader
Length of output: 87
🏁 Script executed:
# Read the LitBlock file
cat -n src/lit/LitBlock.tsRepository: uploadcare/file-uploader
Length of output: 13722
🏁 Script executed:
# Check for tests related to BtnUi accessibility
find . -name "*.test.*" -o -name "*.spec.*" | xargs grep -l "BtnUi\|aria-label" 2>/dev/null | head -10Repository: uploadcare/file-uploader
Length of output: 110
🏁 Script executed:
# Let's also check what ifDefined does - it's a lit-html directive
# First, let's search for how aria-label is tested or used in this codebase
rg "aria-label" -A 2 -B 2 --max-count=10Repository: uploadcare/file-uploader
Length of output: 6507
🏁 Script executed:
# Let's check the exact behavior of the proposed fix
# Trace the logic: when titleProp is empty, what should ariaLabel be?
cat -n src/blocks/CloudImageEditor/src/elements/button/BtnUi.ts | sed -n '70,95p'Repository: uploadcare/file-uploader
Length of output: 1007
🏁 Script executed:
# Also check if there are any tests for BtnUi
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) -exec grep -l "BtnUi" {} \;Repository: uploadcare/file-uploader
Length of output: 50
🏁 Script executed:
# Check how ifDefined is used in the codebase to confirm behavior
rg "ifDefined\(" -B 1 -A 1 src/blocks/CloudImageEditor/src/elements/button/BtnUi.tsRepository: uploadcare/file-uploader
Length of output: 480
🏁 Script executed:
# Let's also understand the render logic - when would titleProp be empty?
# Check how BtnUi is used in the codebase
rg "<uc-btn" -A 5 | head -30Repository: uploadcare/file-uploader
Length of output: 2535
🏁 Script executed:
# Verify ifDefined behavior by checking lit-html documentation or usage patterns
# Search for how ifDefined is implemented or documented in dependencies
rg "ifDefined" src/ --type ts -B 2 -A 2 | grep -E "(from|import|ifDefined)" | head -20Repository: uploadcare/file-uploader
Length of output: 2213
Empty aria-label attributes harm accessibility; use a fallback and prevent empty string values.
When titleProp is empty, l10n('') returns an empty string, and ifDefined('') still renders aria-label="" in the DOM rather than omitting the attribute. This is particularly problematic for icon-only buttons (where text is also empty) which end up without any label for screen readers.
Add a fallback to this.text and ensure empty aria-label values are not rendered:
public override render() {
+ const title = this.titleProp ? this.l10n(this.titleProp) : '';
+ const ariaLabel = title || this.text || undefined;
return html`
<button
type="button"
role=${ifDefined(this.ariaRole || undefined)}
aria-controls=${ifDefined(this.ariaControls || undefined)}
- aria-label=${ifDefined(this.l10n(this.titleProp))}
- title=${ifDefined(this.l10n(this.titleProp))}
+ aria-label=${ifDefined(ariaLabel)}
+ title=${ifDefined(title || undefined)}
>📝 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 override render() { | |
| return html` | |
| <button | |
| type="button" | |
| role=${ifDefined(this.ariaRole || undefined)} | |
| aria-controls=${ifDefined(this.ariaControls || undefined)} | |
| aria-label=${ifDefined(this.l10n(this.titleProp))} | |
| title=${ifDefined(this.l10n(this.titleProp))} | |
| > | |
| <uc-icon | |
| class=${classMap(this._iconClassMap)} | |
| name=${ifDefined(this.icon || undefined)} | |
| ?hidden=${this._computedIconHidden} | |
| ></uc-icon> | |
| <div class="uc-text">${this.text}</div> | |
| </button> | |
| `; | |
| public override render() { | |
| const title = this.titleProp ? this.l10n(this.titleProp) : ''; | |
| const ariaLabel = title || this.text || undefined; | |
| return html` | |
| <button | |
| type="button" | |
| role=${ifDefined(this.ariaRole || undefined)} | |
| aria-controls=${ifDefined(this.ariaControls || undefined)} | |
| aria-label=${ifDefined(ariaLabel)} | |
| title=${ifDefined(title || undefined)} | |
| > | |
| <uc-icon | |
| class=${classMap(this._iconClassMap)} | |
| name=${ifDefined(this.icon || undefined)} | |
| ?hidden=${this._computedIconHidden} | |
| ></uc-icon> | |
| <div class="uc-text">${this.text}</div> | |
| </button> | |
| `; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/blocks/SourceList/SourceList.ts (3)
27-35: Avoid mutatingstyleon every Lit update; gate by relevant change or extract helper.
Right now this runs even when the update is unrelated to wrapping, which can add churn (especially if parent causes frequent updates).protected override updated(changedProperties: PropertyValues<this>): void { super.updated(changedProperties); + // If LitBlock exposes a reactive config property, prefer gating on it here. if (this.cfg.sourceListWrap) { this.style.removeProperty('display'); } else { this.style.display = 'contents'; } }
37-63: SanitizecameraModesto avoid emitting invalid source names likemobile--camera.
IfdeserializeCsv()can return empty entries (e.g., trailing commas), this generates bogusuc-source-btntypes.if (srcName === 'camera' && browserFeatures.htmlMediaCapture) { - const cameraSources = this._cameraModes.length - ? this._cameraModes.map((mode) => `mobile-${mode}-camera`) + const modes = this._cameraModes.map((m) => m.trim()).filter(Boolean); + const cameraSources = modes.length + ? modes.map((mode) => `mobile-${mode}-camera`) : ['mobile-photo-camera'];
68-70: Verify<uc-source-btn>expectstypeas a property (use.type=${...}) and considerrepeat()for stable list rendering.
For many web components, setting the property is the reliable way to trigger updates; andrepeat()helps preserve element identity when the list changes.+import { repeat } from 'lit/directives/repeat.js'; public override render() { - return html`${this._sources.map((type) => html`<uc-source-btn role="listitem" type=${type}></uc-source-btn>`)}`; + return html`${repeat( + this._sources, + (type) => type, + (type) => html`<uc-source-btn role="listitem" .type=${type}></uc-source-btn>`, + )}`; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
demo/features/config-playground.htmlis excluded by!demo/**
📒 Files selected for processing (1)
src/blocks/SourceList/SourceList.ts(1 hunks)
🔇 Additional comments (1)
src/blocks/SourceList/SourceList.ts (1)
13-25: No changes needed. The code is correctly typed and safe.Both
sourceListandcameraModesare guaranteed to be non-null strings by the config system:
subConfigValueinitializes missing keys frominitialConfig, which provides concrete string defaults for both- The
asStringvalidator converts all inputs to strings, never returning undefined- The local type annotations
(val: string)and(cameraModesValue: string)accurately reflect the actual contractstringToArray()is safe to call without guards, anddeserializeCsv()already has internal null-handling
…llection, not it destroy items after 10s of inactivity
nd0ut
force-pushed
the
feat/lit-element
branch
from
3435abf to
d473f8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/adaptive-image.e2e.test.tsx (1)
16-18: Incomplete test implementation.The test contains a commented-out assertion. Either implement the test properly or mark it with
it.todo()to signal it's intentionally incomplete.Consider one of these approaches:
Option 1: Implement the test
it('should be rendered', async () => { - // await expect.element(page.getByTestId('uc-img')).toBeVisible(); + await expect.element(page.getByTestId('uc-img')).toBeVisible(); });Option 2: Mark as TODO
-it('should be rendered', async () => { - // await expect.element(page.getByTestId('uc-img')).toBeVisible(); -}); +it.todo('should be rendered');package.json (1)
79-79: Duplicatetest:specsin test script.The test script runs
npm run test:specstwice consecutively, which appears to be a typo.- "test": "npm run test:types && npm run test:specs && npm run test:specs && npm run test:locales && npm run test:e2e", + "test": "npm run test:types && npm run test:specs && npm run test:locales && npm run test:e2e",src/blocks/Config/Config.ts (1)
192-211: Potential redundant assignment inattributeChangedCallback.The method sets the value twice when both conditions match: first via
anyThis[key] = newVal(line 205) for the config key, then again via(this as any)[name] = newVal(line 209) for the attribute state mapping. Sincekeyis derived fromattrKeyMapping[name]and the setter at line 205 already handles flushing to state, the second assignment at lines 208-210 may be redundant or could cause unexpected behavior.Consider verifying if the second assignment block (lines 208-210) is necessary, or if it's intended for a specific edge case:
if (key) { anyThis[key] = newVal; } - - if (attrStateMapping[name]) { - (this as any)[name] = newVal; - }src/abstract/managers/ModalManager.ts (1)
165-170: DESTROY notification is emitted after subscribers are cleared.The
_notify(ModalEvents.DESTROY, {})call on line 169 occurs afterthis._subscribers.clear()on line 168, so no subscribers will receive the DESTROY event. If this notification is intended to reach subscribers, move it before clearing.public destroy(): void { this.closeAll(); this._modals.clear(); + this._notify(ModalEvents.DESTROY, {}); this._subscribers.clear(); - this._notify(ModalEvents.DESTROY, {}); }src/lit/LitUploaderBlock.ts (1)
424-437: Same inconsistency in _setInitialCrop.Lines 432-433 have the same pattern where activity state is set before the optional modal open. Apply the same guard here for consistency.
♻️ Duplicate comments (30)
src/blocks/CloudImageEditor/src/elements/button/BtnUi.ts (1)
85-86: Accessibility issue from past review remains unaddressed.The past review comment correctly identified that empty
aria-labelattributes harm accessibility. WhentitlePropis empty,l10n('')returns an empty string, andifDefined('')still rendersaria-label=""instead of omitting the attribute. This is particularly problematic for icon-only buttons.Apply the suggested fix from the previous review:
public override render() { + const title = this.titleProp ? this.l10n(this.titleProp) : ''; + const ariaLabel = title || this.text || undefined; return html` <button type="button" role=${ifDefined(this.ariaRole || undefined)} aria-controls=${ifDefined(this.ariaControls || undefined)} - aria-label=${ifDefined(this.l10n(this.titleProp))} - title=${ifDefined(this.l10n(this.titleProp))} + aria-label=${ifDefined(ariaLabel)} + title=${ifDefined(title || undefined)} >package.json (1)
149-151: Vite beta version and outdated keyux already flagged.The concerns about vite 8.0.0-beta.0 and keyux 0.7.1 were addressed in a previous review.
scripts/build.ts (1)
16-31: Repository URL handling already flagged.The concern about
pkgJson.repository.urlpotentially being undefined was addressed in a previous review.src/lit/SymbioteCompatMixin.ts (4)
97-102: Previous review identified blocking all updates before initialization.This concern was already raised in a past review. The
shouldUpdateimplementation blocks all Lit updates until_isInitializedis true, which may interfere with Lit's reactive property system.
104-123: Previous review identified manual attribute reading and error handling issues.Past review noted that line 108's manual
getAttribute('ctx-name')duplicates Lit's declarative attribute handling, and line 115's error handling doesn't prevent continued execution in a broken state.
195-214: Previous review identified buggy ctxOwner behavior.The past review documented that
ctxOwnerenables multiple components to overwriteinit$values unpredictably, and is marked for removal in the changelog.
220-242: Previous review identified Proxy allocation on every access.Past review noted that the
$getter creates a new Proxy and calls_initSharedContext()on every access, causing unnecessary allocations. Caching the Proxy was recommended.src/blocks/CloudImageEditor/src/CropFrame.ts (1)
264-264: Previous review identified missing cleanup for pointerdown listeners.The past review noted that line 264's
addEventListener('pointerdown', this._handlePointerDown.bind(this, direction))creates a bound function that is never stored or removed, causing a memory leak. ThedisconnectedCallbackremoves three other listeners but misses this one.Also applies to: 602-609
src/blocks/CloudImageEditor/src/EditorFilterControl.ts (2)
38-56: Previous review identified missing null/undefined in setter signature.The past review noted that Lit can pass
nullorundefinedto thefiltersetter when attributes change, but the current signature only acceptsstring. The setter should acceptstring | null | undefined.
189-241: Previous review identified race condition in preview loading.The past review noted that the code increments
requestIdbefore awaitingproxyUrl(), allowing stale resolutions to continue. A guard comparingrequestIdto_lastPreviewRequestIdimmediately after theawaitwas recommended.src/abstract/TypedCollection.test.ts (1)
6-10: Tests should clean up collections and timers to prevent flakiness.The
afterEachhook should includevi.clearAllTimers()to clean pending fake timers, and tests should callcollection.destroy()to properly clean up PubSub contexts. This was flagged in a previous review and still applies.afterEach(() => { vi.useRealTimers(); + vi.clearAllTimers(); vi.restoreAllMocks(); });Additionally, add
collection.destroy()calls at the end of each test to prevent context leakage.src/blocks/FileItem/FileItemConfig.ts (1)
44-47: Memory leak: subscriptions not removed indisconnectedCallback.This replaces
_entrySubswith a new Set but doesn't callsub.remove()on existing subscriptions. Consider callingreset()instead.public override disconnectedCallback(): void { super.disconnectedCallback(); - this._entrySubs = new Set<EntrySubscription>(); + this.reset(); }src/blocks/Img/ImgBase.ts (2)
123-125:cdnCnamemay beundefined, producing malformed URLs.When
uuidexists butcdnCnameisundefined, castingcdnCname as stringat line 124 results in the literal string'undefined'being passed tocreateOriginalUrl, generating invalid CDN URLs.Apply this diff to fix:
- if (uuid) { - return this._proxyUrl(createCdnUrl(createOriginalUrl(cdnCname as string, uuid), cdnModifiers)); + if (uuid && cdnCname) { + return this._proxyUrl(createCdnUrl(createOriginalUrl(cdnCname, uuid), cdnModifiers)); }Alternatively, if there's a default CDN host to use when
cdnCnameis missing, use that instead.
214-216: Format check misses 'jpg' alias, causing incorrect width limits.The
_hasFormatJPGcheck only matches'jpeg'. Users specifyingformat: 'jpg'(a common alias) will have their images incorrectly limited toMAX_WIDTH(3000px) instead ofMAX_WIDTH_JPG(5000px).Apply this diff to handle both formats:
private get _hasFormatJPG(): boolean { - return (this.$$('format') as string).toLowerCase() === 'jpeg'; + const format = (this.$$('format') as string).toLowerCase(); + return format === 'jpeg' || format === 'jpg'; }src/blocks/Thumb/Thumb.ts (1)
344-351: GuardIntersectionObserverfor non-browser/test environments.This concern from a previous review appears unresolved.
new window.IntersectionObserver(...)will throw in test environments (e.g., happy-dom) that lack this API. Add a feature check with a fallback to eager generation, as done inEditorFilterControl.ts:public override connectedCallback(): void { super.connectedCallback(); this._observer?.disconnect(); - this._observer = new window.IntersectionObserver(this._observerCallback.bind(this), { threshold: 0.1 }); - this._observer.observe(this); + if ('IntersectionObserver' in window) { + this._observer = new window.IntersectionObserver(this._observerCallback.bind(this), { threshold: 0.1 }); + this._observer.observe(this); + } else { + this._requestThumbGeneration(true); + } }src/blocks/ProgressBarCommon/ProgressBarCommon.ts (1)
45-55: Bug: Property name mismatch prevents attribute toggle.The
changedProperties.has('visible')check will never match because the property is named_visible, notvisible. This means theactiveattribute will never be set or removed, breaking the visibility behavior.Apply this diff to fix the property name:
protected override updated(changedProperties: PropertyValues<this>): void { super.updated(changedProperties); - if (changedProperties.has('visible' as keyof ProgressBarCommon)) { + if (changedProperties.has('_visible' as keyof ProgressBarCommon)) { if (this._visible) { this.setAttribute('active', ''); } else { this.removeAttribute('active'); } } }src/solutions/file-uploader/inline/FileUploaderInline.ts (1)
83-85:_couldCancelmay not update when*uploadListchanges.The subscription only listens to
*history, but_couldCanceldepends on both_couldHistoryBack(which uses history) and_couldShowList(which uses*uploadListandcfg.showEmptyList). Changes to*uploadListwon't trigger this subscription, potentially leaving_couldCancelstale.Consider adding a separate subscription:
this.sub('*history', () => { this._couldCancel = this._couldHistoryBack || this._couldShowList; }); + + this.sub('*uploadList', () => { + this._couldCancel = this._couldHistoryBack || this._couldShowList; + });src/abstract/TypedData.ts (1)
11-14: Context ID collision risk remains.The past review flagged that
UID.generateFastUid()(time+random based) can collide under high churn, causingPubSub.registerCtx()to throw. TheUID.generateRandomUUID()method exists in the codebase and provides a cryptographically stronger alternative whencrypto.randomUUIDis available.Consider switching to the more robust UUID generator:
public constructor(initialValue: T) { - this._ctxId = UID.generateFastUid(); + this._ctxId = UID.generateRandomUUID() as Uid; this._data = PubSub.registerCtx(initialValue, this._ctxId); }src/blocks/Modal/Modal.ts (2)
132-138: Event listeners are added but never removed from the dialog element.This issue was flagged in a previous review. The
_handleDialogRefcallback adds event listeners to the dialog element, but there's no cleanup when the component disconnects or when the ref callback is invoked with a new/undefined element. This can lead to memory leaks or duplicate listeners.Consider removing listeners from the previous dialog element before adding to the new one:
private _handleDialogRef(dialog: Element | undefined): void { + // Clean up old listeners + const oldDialog = this.dialogEl.value; + if (oldDialog) { + oldDialog.removeEventListener('close', this._handleDialogClose); + oldDialog.removeEventListener('mousedown', this._handleDialogMouseDown); + oldDialog.removeEventListener('mouseup', this._handleDialogMouseUp); + } + this.dialogEl = { value: dialog } as typeof this.dialogEl; this.dialogEl.value?.addEventListener('close', this._handleDialogClose); this.dialogEl.value?.addEventListener('mousedown', this._handleDialogMouseDown); this.dialogEl.value?.addEventListener('mouseup', this._handleDialogMouseUp); }
20-26: Body scroll-lock is unconditionally reset, potentially clobbering other overlays.This issue was flagged in a previous review. The
closeDialoganddisconnectedCallbackmethods resetdocument.body.style.overflow = ''without tracking whether this modal instance was the one that set it. This can interfere with other modals or app-defined overflow styles.Track whether this modal applied the scroll lock and restore the previous value:
+ private _prevBodyOverflow: string | null = null; + private _didLockBodyScroll = false; public show(): void { // ...existing dialog.showModal() logic... - if (this.cfg.modalScrollLock) { - document.body.style.overflow = 'hidden'; - } + if (this.cfg.modalScrollLock && !this._didLockBodyScroll) { + this._prevBodyOverflow = document.body.style.overflow; + document.body.style.overflow = 'hidden'; + this._didLockBodyScroll = true; + } } protected closeDialog = (): void => { this.modalManager?.close(this.id); - if (!this.modalManager?.hasActiveModals) { - document.body.style.overflow = ''; - } + this._restoreBodyOverflowIfNeeded(); }; + private _restoreBodyOverflowIfNeeded(): void { + if (!this._didLockBodyScroll) return; + if (this.modalManager?.hasActiveModals) return; + document.body.style.overflow = this._prevBodyOverflow ?? ''; + this._prevBodyOverflow = null; + this._didLockBodyScroll = false; + }Also applies to: 54-57, 122-130
src/lit/LitBlock.ts (1)
125-125: TelemetryManager initialization is not reactive to config changes.The TelemetryManager is created once during
initCallback. IfqualityInsightsor other telemetry-related config changes at runtime, the telemetry behavior won't update accordingly. Consider either making the manager check config dynamically or subscribing to config changes.Based on past review comment from egordidenko and coderabbitai.
src/lit/LitUploaderBlock.ts (1)
340-340: Type mismatch between CTX initialization and SharedState definition.The
*uploadTriggeris typed asSet<Uid>in SharedState.ts but initialized asnew Set<string>()in CTX.ts. WhileUidis a branded string type, the initialization doesn't use the branded type, which could cause subtle type issues.Based on past review comment about type mismatch.
src/blocks/FileItem/FileItem.ts (2)
298-306: Focus state sync is incomplete -_isFocusedis never set to true.The onclick handler toggles the
focusedattribute on instances but never sets_isFocused = true. The template uses?focused=${this._isFocused}at line 465, so the focused styling on the inner div won't reflect user clicks.Based on past review comment about focus state not being reactive.
this.onclick = () => { FileItem.activeInstances.forEach((inst) => { - if (inst === this) { - inst.setAttribute('focused', ''); - } else { - inst.removeAttribute('focused'); - } + const focused = inst === this; + inst._isFocused = focused; + if (focused) inst.setAttribute('focused', ''); + else inst.removeAttribute('focused'); }); };
178-196:_badgeIconis not cleared for non-terminal states.When an entry transitions from FAILED/FINISHED to other states (e.g., retrying an upload),
_badgeIconretains its previous value. Add an else branch to clear it.Based on past review comment about stale badge icon.
private _handleState(entry: UploadEntryTypedData, state: FileItemStateValue): void { if (state === FileItemState.FAILED) { this._badgeIcon = 'badge-error'; } else if (state === FileItemState.FINISHED) { this._badgeIcon = 'badge-success'; + } else { + this._badgeIcon = ''; }src/abstract/UploaderPublicApi.ts (3)
387-387: Usepub()for*currentActivityupdate.This line uses direct assignment instead of
pub(), causing the same state synchronization issue as lines 222 and 302.Apply this diff:
this._ctx.modalManager?.close(this._ctx.$['*currentActivity']); - this._ctx.$['*currentActivity'] = null; + this._ctx.pub('*currentActivity', null);
221-222: Usepub()for*currentActivityupdate.This line uses direct assignment
this._ctx.$['*currentActivity'] = ...which bypasses PubSub notifications. Other parts of this file usepub()(lines 295, 311, 340). Direct assignment prevents subscribers inLitActivityBlockfrom receiving state changes.Apply this diff:
this._ctx.modalManager?.open(LitActivityBlock.activities.UPLOAD_LIST); - this._ctx.$['*currentActivity'] = LitActivityBlock.activities.UPLOAD_LIST; + this._ctx.pub('*currentActivity', LitActivityBlock.activities.UPLOAD_LIST);
302-302: Usepub()for*currentActivityupdate.This line uses direct assignment instead of
pub(), causing the same state synchronization issue as line 222.Apply this diff:
if (srcKey === 'local') { - this._ctx.$['*currentActivity'] = LitActivityBlock.activities.UPLOAD_LIST; + this._ctx.pub('*currentActivity', LitActivityBlock.activities.UPLOAD_LIST); this.openSystemDialog();src/blocks/ExternalSource/ExternalSource.ts (1)
243-245: Duplicate event listener registration.The
loadevent listener is registered twice on lines 243 and 245. This will cause_handleIframeLoadto be called twice for each iframe load event.Apply this diff to remove the duplicate:
iframe.addEventListener('load', this._handleIframeLoad.bind(this)); - iframe.addEventListener('load', this._handleIframeLoad.bind(this)); - if (this._iframeRef.value) {src/abstract/TypedCollection.ts (2)
152-154:read()return type should beundefined, notnull.The underlying
PubSub.read()returnsundefinedfor missing keys, but this method converts it tonullusing the nullish coalescing operator. This creates an inconsistency with the PubSub API and may cause incorrect type checks in consuming code.Apply this diff:
- public read(id: Uid): TypedData<T> | null { - return this._data.read(id) ?? null; + public read(id: Uid): TypedData<T> | undefined { + return this._data.read(id); }Note: This change will require updating call sites that check for
nullto check forundefinedinstead.
188-192: Snapshot IDs before iteration to avoid mutation issues.
clearAll()mutatesthis._items(viathis.remove(id)which callsthis._items.delete(id)) duringforEachiteration. While JavaScript doesn't throw an error, mutating a Set during iteration can cause elements to be skipped depending on insertion order.Apply this diff:
public clearAll(): void { - this._items.forEach((id) => { + const ids = [...this._items]; + for (const id of ids) { this.remove(id); - }); + } }
🧹 Nitpick comments (24)
src/blocks/DropArea/DropArea.ts (1)
52-67: Consider preventing default behavior for keyboard events.When handling Space and Enter keys, the browser's default behavior (e.g., page scrolling for Space) is not prevented. This could lead to unintended side effects when users interact with the clickable drop area.
Apply this diff to prevent default behavior:
private readonly _handleAreaInteraction = (event: Event) => { if (event instanceof KeyboardEvent) { if (event.code !== 'Space' && event.code !== 'Enter') { return; } + event.preventDefault(); } else if (!(event instanceof MouseEvent)) { return; }src/lit/parseCssPropertyValue.ts (1)
5-11: Consider validating matching quote types.
hasLeadingTrailingQuotesreturnstruefor mismatched quotes like"value'or'value". If this is unintentional, consider:function hasLeadingTrailingQuotes(str: string): boolean { if (str.length < 2) return false; + const first = str[0]; + const last = str[str.length - 1]; return ( - (str[0] === DOUBLE_QUOTE || str[0] === SINGLE_QUOTE) && - (str[str.length - 1] === DOUBLE_QUOTE || str[str.length - 1] === SINGLE_QUOTE) + (first === DOUBLE_QUOTE && last === DOUBLE_QUOTE) || + (first === SINGLE_QUOTE && last === SINGLE_QUOTE) ); }src/blocks/Select/Select.ts (2)
21-27: Select value binding may not sync correctly.Binding
.valueon a<select>element in Lit can be unreliable because the value property depends on the options being rendered first. Consider binding theselectedattribute on the matching option instead.Apply this diff:
public override render() { return html` - <select @change=${this._handleChange} .value=${this.value} ?disabled=${this.disabled}> - ${this.options.map((option) => html`<option value=${option.value}>${option.text}</option>`)} + <select @change=${this._handleChange} ?disabled=${this.disabled}> + ${this.options.map((option) => html`<option value=${option.value} ?selected=${option.value === this.value}>${option.text}</option>`)} </select> `; }
29-34: Redundant disabled guard.The native
<select>element with thedisabledattribute won't firechangeevents, so this check is unnecessary. The subsequentpreventDefault/stopPropagationcalls at lines 36-37 also duplicate behavior.Consider simplifying:
private _handleChange = (event: Event): void => { - if (this.disabled) { - event.preventDefault(); - event.stopPropagation(); - return; - } - - event.preventDefault(); event.stopPropagation(); const selectEl = event.currentTarget as HTMLSelectElement | null;src/blocks/CloudImageEditor/src/EditorSlider.ts (1)
54-77: Consider batching state updates to reduce intermediate renders.
_initializeValuescreates multiple intermediate state objects. While functionally correct, this could trigger unnecessary re-renders.Consider batching updates:
private _initializeValues(): void { const operation = this.state.operation; const { range, zero } = COLOR_OPERATIONS_CONFIG[operation]; const [min, max] = range; - this.state = { ...this.state, min, max, zero }; - const editorTransformations = this.$['*editorTransformations'] as Transformations; const transformation = editorTransformations[operation]; if (operation === 'filter') { let value = Number(max); const filterTransformation = transformation as Transformations['filter'] | undefined; if (filterTransformation) { const { name, amount } = filterTransformation; value = name === this.state.filter ? amount : max; } - this.state = { ...this.state, value, defaultValue: value }; + this.state = { ...this.state, min, max, zero, value, defaultValue: value }; return; } const value = typeof transformation !== 'undefined' ? (transformation as number) : zero; - this.state = { ...this.state, value, defaultValue: value }; + this.state = { ...this.state, min, max, zero, value, defaultValue: value }; }src/lit/BaseComponent.ts (1)
1-7: LGTM!The
BaseComponentprovides a clean, unified foundation for Lit-based components by composing the necessary mixins. The mixin chain establishes light DOM rendering, CSS data handling, Symbiote compatibility, and element registration capabilities.Consider adding a brief JSDoc comment describing the purpose and mixin composition order for future maintainers:
+/** + * Base component for all Lit-based blocks. + * Composes: LightDomMixin → CssDataMixin → SymbioteMixin → RegisterableElementMixin + */ export class BaseComponent extends RegisterableElementMixin(SymbioteMixin()(CssDataMixin(LightDomMixin(LitElement)))) {}src/blocks/SourceList/SourceList.ts (1)
27-35: Consider guarding the display style update.The
updatedmethod runs on every property change but only needs to update the display style whensourceListWrapconfig changes. However, sincecfg.sourceListWrapaccess is cheap and the DOM operation is minimal, this is acceptable for now.src/blocks/CloudImageEditor/src/EditorOperationControl.ts (1)
29-48: Consider simplifying the resolver functions.The
resolveTitlePropandresolveTitlefunctions are defined and immediately invoked without leveraging async or deferred execution. This adds unnecessary indirection.Apply this diff to simplify:
private _updateOperationMetadata(operation: ColorOperation): void { this.icon = operation; - const resolveTitleProp = () => { - const label = this.l10n('a11y-cloud-editor-apply-tuning', { - name: this.l10n(operation).toLowerCase(), - }); - this.titleProp = label; - return label; - }; - - const resolveTitle = () => { - const titleText = this.l10n(operation); - this.title = titleText; - return titleText; - }; - - resolveTitleProp(); - resolveTitle(); + const label = this.l10n('a11y-cloud-editor-apply-tuning', { + name: this.l10n(operation).toLowerCase(), + }); + this.titleProp = label; + + const titleText = this.l10n(operation); + this.title = titleText; }src/blocks/Img/ImgBase.ts (2)
297-318: Consider cleanup of event listeners.The
_loaderImagemethod correctly implements promise-based image loading, but theloadanderrorevent listeners are not removed after firing. For long-lived or frequently created images, this could lead to memory accumulation.Apply this diff to clean up listeners:
private _loaderImage({ src, srcset, elNode }: LoaderParams): Promise<HTMLImageElement> { return new Promise((resolve, reject) => { this._setupConfigForImage({ elNode }); elNode.setAttribute(UNRESOLVED_ATTR, ''); - elNode.addEventListener('load', () => { + const handleLoad = () => { elNode.removeAttribute(UNRESOLVED_ATTR); + elNode.removeEventListener('load', handleLoad); + elNode.removeEventListener('error', handleError); resolve(elNode); - }); + }; - elNode.addEventListener('error', () => { + const handleError = () => { + elNode.removeEventListener('load', handleLoad); + elNode.removeEventListener('error', handleError); reject(false); - }); + }; + + elNode.addEventListener('load', handleLoad); + elNode.addEventListener('error', handleError); this._appendURL({ elNode, src, srcset, }); }); }
320-382: Remove unnecessaryawaiton synchronousremove()calls.The
HTMLElement.remove()method is synchronous and doesn't return a promise. Usingawaiton lines 340, 346, 372, and 378 is unnecessary and misleading.Apply this diff to remove unnecessary awaits:
if (this._hasPreviewImage) { - await this._imgPreview.remove(); + this._imgPreview.remove(); } this.appendChild(this._img); } catch { if (this._hasPreviewImage) { - await this._imgPreview?.remove(); + this._imgPreview?.remove(); } this.appendChild(this._img); } }); return; } try { if (this._hasPreviewImage) { await this._loaderImage({ src: this._srcUrlPreview, elNode: this._imgPreview, }); this.appendChild(this._imgPreview); } await this._loaderImage({ src: this._getSrc(), srcset: this._getSrcset(), elNode: this._img, }); if (this._hasPreviewImage) { - await this._imgPreview?.remove(); + this._imgPreview?.remove(); } this.appendChild(this._img); } catch { if (this._hasPreviewImage) { - await this._imgPreview?.remove(); + this._imgPreview?.remove(); } this.appendChild(this._img); }src/lit/LightDomMixin.ts (1)
77-95: The comment on lines 81-85 raises a valid design question.The current implementation treats slots containing only comments and empty text nodes as "empty", which seems reasonable for the stated uncertainty. Consider documenting this behavior in the public API or adding a configuration option if different consumers need different semantics.
src/blocks/SimpleBtn/SimpleBtn.ts (1)
28-39: Inconsistent template indentation.The template literal has inconsistent indentation - the opening content starts at column 4 but the closing backtick/semicolon is also at column 4, while inner content varies. This is minor but affects readability.
public override render() { - return html` - <uc-drop-area .disabled=${!this.dropzone}> - <button type="button" @click=${this._handleClick}> - <uc-icon name="upload"></uc-icon> - <span>${this.l10n(this._buttonTextKey)}</span> - ${this.yield('')} - <div class="uc-visual-drop-area">${this.l10n('drop-files-here')}</div> - </button> - </uc-drop-area> - `; + return html` + <uc-drop-area .disabled=${!this.dropzone}> + <button type="button" @click=${this._handleClick}> + <uc-icon name="upload"></uc-icon> + <span>${this.l10n(this._buttonTextKey)}</span> + ${this.yield('')} + <div class="uc-visual-drop-area">${this.l10n('drop-files-here')}</div> + </button> + </uc-drop-area> + `; }src/blocks/SourceBtn/SourceBtn.ts (2)
95-102: Potential duplicate_applyTypecall on initialization.
_applyTypeis called ininitCallback(line 100) whenthis.typeis set. However,willUpdate(line 146-148) also calls_applyTypewhen thetypeproperty changes. During initial connection, both may fire, causing redundant work. Consider removing the call frominitCallbackand relying solely onwillUpdate.public override initCallback(): void { super.initCallback(); this._initTypes(); - - if (this.type) { - this._applyType(this.type); - } }
156-163: Inconsistent template indentation.Similar to SimpleBtn.ts, the template has inconsistent indentation.
public override render() { - return html` - <button type="button" @click=${this.activate}> - <uc-icon name=${this._iconName}></uc-icon> - <div class="uc-txt">${this.l10n(this._srcTypeKey)}</div> - </button> - `; + return html` + <button type="button" @click=${this.activate}> + <uc-icon name=${this._iconName}></uc-icon> + <div class="uc-txt">${this.l10n(this._srcTypeKey)}</div> + </button> + `; }src/blocks/Img/ImgConfig.ts (1)
22-28: Fragile type casting for default value extraction.The double cast
(config as { default?: CssPropValue })?.defaultis fragile. Consider definingPropConfigmore explicitly or using a type guard.+type PropConfig = { default?: CssPropValue }; + public constructor() { super(); for (const prop in PROPS_MAP) { - const config = PROPS_MAP[prop as keyof typeof PROPS_MAP] as PropConfig | undefined; - this._state[CSS_PREF + prop] = (config as { default?: CssPropValue })?.default || ''; + const config = PROPS_MAP[prop as keyof typeof PROPS_MAP] as PropConfig | undefined; + this._state[CSS_PREF + prop] = config?.default ?? ''; } }Note: Using
?? ''instead of|| ''preserves falsy values like0orfalseif they're valid defaults.src/blocks/CloudImageEditor/src/EditorToolbar.ts (1)
214-222: Consider caching computed style access for performance.The
getComputedStylecall in_syncTabIndicatoris expensive. While this is called on resize and tab changes (not continuous), consider caching or avoiding the computed style lookup when possible.- const width = toggleEl.offsetWidth || Number(getComputedStyle(toggleEl).width.replace('px', '')); + const width = toggleEl.offsetWidth || toggleEl.getBoundingClientRect().width;
getBoundingClientRect()is generally faster thangetComputedStyle()for width measurements.src/blocks/UrlSource/UrlSource.ts (1)
44-63: Consider adding URL validation before upload.The
_handleUploadmethod only checks if the URL is empty but doesn't validate the URL format. Invalid URLs will likely fail at the API level, but early validation could improve UX.private _handleUpload = (event: Event) => { event.preventDefault(); // ... telemetry code ... const input = this._inputRef.value; const url = input?.value?.trim(); if (!url) { return; } + try { + new URL(url); + } catch { + // Optionally show validation error to user + return; + } this.api.addFileFromUrl(url, { source: UploadSource.URL }); // ... };src/blocks/Config/Config.ts (1)
83-93: Consider adding brief comments explaining the type escape.The
@ts-expect-errorcomments suppress type errors for dynamic state key assignment. While this is necessary due to the dynamic nature of config keys, a brief inline comment explaining why the type escape is needed would improve maintainability.private _flushValueToState<TKey extends keyof ConfigType>(key: TKey, value: unknown) { if (this.$[sharedConfigKey(key)] !== value) { if (typeof value === 'undefined' || value === null) { - // @ts-expect-error + // @ts-expect-error - Dynamic key access requires type escape since sharedConfigKey returns a computed string this.$[sharedConfigKey(key)] = initialConfig[key]; } else { - // @ts-expect-error + // @ts-expect-error - Dynamic key access requires type escape since sharedConfigKey returns a computed string this.$[sharedConfigKey(key)] = value; } } }src/abstract/TypedData.ts (1)
38-43:getValuereturnsundefinedfor missing keys after logging a warning.The method warns about missing properties but still calls
this._data.read(prop), which will also log its own warning (fromPubSub#read). This results in double warnings. Consider returning early or suppressing the duplicate warning.public getValue<K extends keyof T>(prop: K): T[K] { if (!this._data.has(prop)) { console.warn(`${MSG_NAME}${String(prop)}`); + return undefined as T[K]; } return this._data.read(prop); }src/blocks/CloudImageEditor/src/CloudImageEditorBlock.ts (1)
354-398: Consider adding error handling for subscription setup failures.The
initEditormethod sets up subscriptions for*networkProblemsand*editorTransformations. If an error occurs during these subscriptions, it may leave the editor in a partially initialized state.src/abstract/managers/ModalManager.ts (1)
136-140: Unnecessary cast due to permissive parameter type.The
callbackparameter is typed asModalCb | undefined, but the delete call casts it back toModalCb. Consider making the parameter non-optional if undefined callbacks shouldn't be unsubscribed.-public unsubscribe(event: ModalEventType, callback: ModalCb | undefined): void { +public unsubscribe(event: ModalEventType, callback: ModalCb): void { if (this._subscribers.has(event)) { - this._subscribers.get(event)?.delete(callback as ModalCb); + this._subscribers.get(event)?.delete(callback); } }src/blocks/CloudImageEditor/src/EditorAspectRatioButtonControl.ts (1)
118-145: Consider extracting title resolution logic.The
_updateAspectRatioPresentationmethod defines inner functionsresolveTitleandresolveTitlePropthat are immediately called. This pattern is verbose; consider simplifying by computing values directly.private _updateAspectRatioPresentation(value: CropAspectRatio): void { const isFreeform = !!value.hasFreeform; this.toggleAttribute('uc-aspect-ratio-freeform', isFreeform); - const resolveTitle = () => { - const titleText = isFreeform ? this.l10n('custom') : `${value.width}:${value.height}`; - this.title = titleText; - return titleText; - }; - - const resolveTitleProp = () => { - const label = this.l10n('a11y-cloud-editor-apply-aspect-ratio', { - name: isFreeform - ? this.l10n('custom').toLowerCase() - : this.l10n('crop-to-shape', { value: `${value.width}:${value.height}` }).toLowerCase(), - value: '', - }); - this.titleProp = label; - return label; - }; - - resolveTitle(); - resolveTitleProp(); + this.title = isFreeform ? this.l10n('custom') : `${value.width}:${value.height}`; + this.titleProp = this.l10n('a11y-cloud-editor-apply-aspect-ratio', { + name: isFreeform + ? this.l10n('custom').toLowerCase() + : this.l10n('crop-to-shape', { value: `${value.width}:${value.height}` }).toLowerCase(), + value: '', + }); if (!isFreeform) { this.requestUpdate(); } }src/blocks/FileItem/FileItem.ts (2)
36-40: Consider using branded type for uid property.The
uidproperty is typed asUidbut initialized to'' as Uid. This type assertion could mask issues if an empty string is used before a valid uid is set. Consider making it nullable or adding validation.-@property({ type: String, attribute: false }) -public uid: Uid = '' as Uid; +@property({ attribute: false }) +public uid: Uid | null = null;Then add a guard in
_handleEntryIdand other methods that access it.
369-380: Image shrink error handling silently swallows exceptions.The catch block at line 377 is empty, which means shrink failures are silently ignored. While the comment says "keep original file if shrinking fails", consider logging a warning for debugging purposes.
try { const settings = parseShrink(this.cfg.imageShrink); if (!settings) { console.warn('Image shrink settings are invalid, skipping shrinking'); } else { file = await shrinkFile(file, settings); } - } catch { + } catch (err) { // keep original file if shrinking fails + console.warn('Image shrinking failed, using original file', err); }
| "suspicious": { | ||
| "noDuplicateProperties": "off", | ||
| "noExplicitAny": "warn" | ||
| "noExplicitAny": "info", |
There was a problem hiding this comment.
Consider the type safety implications of downgrading noExplicitAny.
The severity of noExplicitAny has been downgraded from "warn" to "info". While this may reduce noise during the Lit Element migration, it weakens type safety by making explicit any usage less visible. Consider whether this is a temporary measure or if there's a plan to address explicit any types after the migration.
🤖 Prompt for AI Agents
In biome.json around line 22, the rule "noExplicitAny" was downgraded from
"warn" to "info", which reduces visibility of explicit any usage; either revert
the setting back to "warn" to maintain type-safety, or if the downgrade is
intended as temporary, document that intent (add a comment and link to a tracked
issue/epic) and add a compensating measure such as a targeted override that
keeps "warn" for critical paths or a CI check that prevents new explicit any
occurrences until the migration is complete.
| options.mangleProps = /^_/; | ||
| }, | ||
| noExternal: buildItem.bundleExternalDependencies ? Object.keys(dependencies) : undefined, | ||
| esbuildPlugins: buildItem.minify ? [minifyTemplates(), writeFiles()] : [], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/build.ts | head -70Repository: uploadcare/file-uploader
Length of output: 2794
🌐 Web query:
esbuild-minify-templates plugin writeFiles usage pattern
💡 Result:
Short answer — use writeFiles to persist the in-memory build output after minifying template literals; call it after the minifyTemplates step and run esbuild with write: false. Example patterns:
-
plugin-array form (recommended):
import esbuild from "esbuild";
import { minifyTemplates, writeFiles } from "esbuild-minify-templates";
await esbuild.build({
entryPoints: ["src/index.ts"],
outfile: "dist/index.js",
bundle: true,
sourcemap: true,
write: false, // important
plugins: [minifyTemplates(), writeFiles()] // writeFiles last
}); -
promise-chain form:
import esbuild from "esbuild";
import { minifyTemplates, writeFiles } from "esbuild-minify-templates";
esbuild.build({... , write: false})
.then(minifyTemplates)
.then(writeFiles)
.catch(() => process.exit(1));
Why: esbuild provides no API to modify/write final output when build.write is true, so the plugin minifies templates in-memory and writeFiles must be used to write those modified outputs to disk. Also, if you run esbuild with write: true the plugins are no-ops (useful to skip minification in some builds). [1][2]
Sources:
[1] esbuild-minify-templates README (npm).
[2] esbuild-minify-templates older README example (npm).
🏁 Script executed:
grep -n "write" scripts/build.ts | head -20Repository: uploadcare/file-uploader
Length of output: 292
🌐 Web query:
tsup write false esbuild configuration option
💡 Result:
Short answer: esbuild's write: false tells esbuild to not write output files to disk and instead return them in result.outputFiles. You can pass that option through tsup via esbuildOptions (esbuildOptions: { write: false }), but tsup itself normally manages writing outputs to disk and does not directly expose esbuild's outputFiles — so to get in-memory build output with tsup you must intercept esbuild's build result via an esbuild plugin (e.g., use esbuildPlugins and build.onEnd to capture the result). [1][2][3]
Sources:
- esbuild API — write: false returns outputFiles in the build result. [1]
- Example/guide showing using esbuildOptions.write = false + an esbuild plugin to capture in-memory output when using tsup. [2]
- tsup docs (tsup is a wrapper around esbuild; use esbuildOptions/esbuildPlugins to pass options/plugins). [3]
Add write: false to esbuildOptions for minifyTemplates() and writeFiles() plugins to function correctly.
The plugin order is correct, but the esbuild-minify-templates plugin requires write: false to be set in esbuildOptions. Without it, the plugins become no-ops and template minification will not occur. Update line 48 to include write: false:
esbuildOptions(options) {
options.write = false; // Required for minifyTemplates/writeFiles plugins
options.conditions = ['browser'];
// ... rest of options
}🤖 Prompt for AI Agents
In scripts/build.ts around lines 48 and 55, the esbuildOptions must set
options.write = false so the esbuild-minify-templates and writeFiles plugins can
run; update the esbuildOptions(options) function (near line 48) to assign
options.write = false before other option changes (keep options.conditions =
['browser'] etc.), and keep the existing esbuildPlugins line at 55 unchanged so
minifyTemplates() and writeFiles() are executed correctly.
| public override render() { | ||
| const clickHandler = this.onClick; | ||
| const title = this.title; | ||
|
|
||
| return html` | ||
| <button | ||
| role="option" | ||
| type="button" | ||
| aria-label=${ifDefined(this.titleProp)} | ||
| title=${ifDefined(this.titleProp)} | ||
| @click=${clickHandler} | ||
| > | ||
| <uc-icon name=${this.icon}></uc-icon> | ||
| <div class="uc-title" ?hidden=${!title}>${title}</div> | ||
| </button> | ||
| `; | ||
| } |
There was a problem hiding this comment.
Potential this context issue with click handler.
Assigning this.onClick to a local variable and using it in the template may lose the this context if onClick is overridden in subclasses and not bound. Also, the title variable on line 53 is unused since this.title is accessed directly on line 64.
public override render() {
- const clickHandler = this.onClick;
- const title = this.title;
-
return html`
<button
role="option"
type="button"
aria-label=${ifDefined(this.titleProp)}
title=${ifDefined(this.titleProp)}
- @click=${clickHandler}
+ @click=${this.onClick}
>
<uc-icon name=${this.icon}></uc-icon>
- <div class="uc-title" ?hidden=${!title}>${title}</div>
+ <div class="uc-title" ?hidden=${!this.title}>${this.title}</div>
</button>
`;
}📝 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 override render() { | |
| const clickHandler = this.onClick; | |
| const title = this.title; | |
| return html` | |
| <button | |
| role="option" | |
| type="button" | |
| aria-label=${ifDefined(this.titleProp)} | |
| title=${ifDefined(this.titleProp)} | |
| @click=${clickHandler} | |
| > | |
| <uc-icon name=${this.icon}></uc-icon> | |
| <div class="uc-title" ?hidden=${!title}>${title}</div> | |
| </button> | |
| `; | |
| } | |
| public override render() { | |
| return html` | |
| <button | |
| role="option" | |
| type="button" | |
| aria-label=${ifDefined(this.titleProp)} | |
| title=${ifDefined(this.titleProp)} | |
| @click=${this.onClick} | |
| > | |
| <uc-icon name=${this.icon}></uc-icon> | |
| <div class="uc-title" ?hidden=${!this.title}>${this.title}</div> | |
| </button> | |
| `; | |
| } |
🤖 Prompt for AI Agents
In src/blocks/CloudImageEditor/src/EditorButtonControl.ts around lines 51-67,
the code assigns this.onClick and this.title to local variables which can lose
the correct this context for overridden subclass methods and the local title is
unused; remove those local variables and ensure the handler is invoked with the
component instance as context (either bind this.onClick in the constructor or
call it via an inline arrow wrapper in the template so it executes as
this.onClick(event) with the correct this), and reference this.title
consistently (or read it into a single variable and use that) instead of leaving
an unused title variable.
| @property({ attribute: 'aria-role' }) | ||
| public ariaRole = ''; |
There was a problem hiding this comment.
Confusing attribute-to-property mapping for role.
The property ariaRole is mapped to the HTML attribute aria-role, but in the template (line 83) it's used to set the button's role attribute, not aria-role. The role attribute is a standard HTML attribute, not an ARIA attribute. Consider renaming to simply role for clarity:
- @property({ attribute: 'aria-role' })
- public ariaRole = '';
+ @property({ type: String })
+ public role = '';And update line 83:
- role=${ifDefined(this.ariaRole || undefined)}
+ role=${ifDefined(this.role || undefined)}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/blocks/CloudImageEditor/src/elements/button/BtnUi.ts around lines 23–24,
the component defines a property named ariaRole mapped to the attribute
'aria-role' but the template sets the element's standard HTML role attribute;
rename the property to role and map it to the standard 'role' attribute, update
all references (including the template use at line ~83) to use role instead of
ariaRole, and remove any leftover 'aria-role' attribute handling so the
property-to-attribute mapping matches the actual HTML role attribute usage.
| private _applyThemeClass(): void { | ||
| if (this.theme && this.theme !== 'custom') { | ||
| this.className = `uc-${this.theme}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Direct className assignment replaces all classes.
Line 56 replaces the entire className property, which will remove any external classes applied to this component. Consider using classList methods instead:
private _applyThemeClass(): void {
+ // Remove previous theme class
+ this.classList.forEach(cls => {
+ if (cls.startsWith('uc-') && cls !== 'uc-icon' && cls !== 'uc-text') {
+ this.classList.remove(cls);
+ }
+ });
+
if (this.theme && this.theme !== 'custom') {
- this.className = `uc-${this.theme}`;
+ this.classList.add(`uc-${this.theme}`);
}
}Alternatively, if complete class control is intended, document this behavior clearly.
📝 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 _applyThemeClass(): void { | |
| if (this.theme && this.theme !== 'custom') { | |
| this.className = `uc-${this.theme}`; | |
| } | |
| } | |
| private _applyThemeClass(): void { | |
| // Remove previous theme class | |
| this.classList.forEach(cls => { | |
| if (cls.startsWith('uc-') && cls !== 'uc-icon' && cls !== 'uc-text') { | |
| this.classList.remove(cls); | |
| } | |
| }); | |
| if (this.theme && this.theme !== 'custom') { | |
| this.classList.add(`uc-${this.theme}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/blocks/CloudImageEditor/src/elements/button/BtnUi.ts around lines 54 to
58, the method currently assigns this.className directly which overwrites all
existing classes on the element; instead preserve other classes and only
add/remove the theme class: compute the prefixed theme class name (e.g.
`uc-${theme}`), remove any previous `uc-` theme classes if present, and then add
the new one via this.classList.add(thisTheme) (or remove it when theme is
'custom') so external classes remain intact; if full class control is intended,
document that this method will replace all classes.
| this.cfg.useCloudImageEditor && | ||
| this.hasBlockInCtx((block) => block.activityType === ActivityBlock.activities.CLOUD_IMG_EDIT) | ||
| this.hasBlockInCtx((block) => block.activityType === LitActivityBlock.activities.CLOUD_IMG_EDIT) | ||
| ) { | ||
| this.$['*currentActivityParams'] = { | ||
| internalId: entry.uid, | ||
| }; | ||
| this.$['*currentActivity'] = ActivityBlock.activities.CLOUD_IMG_EDIT; | ||
| this.modalManager?.open(ActivityBlock.activities.CLOUD_IMG_EDIT); | ||
| this.$['*currentActivity'] = LitActivityBlock.activities.CLOUD_IMG_EDIT; | ||
| this.modalManager?.open(LitActivityBlock.activities.CLOUD_IMG_EDIT); | ||
| } |
There was a problem hiding this comment.
Potential null pointer when opening cloud image editor.
At line 376, this.modalManager?.open() uses optional chaining, but line 375 unconditionally sets this.$['*currentActivity']. If modalManager is null, the activity state will be set but the modal won't open, potentially causing inconsistent UI state.
if (
entry &&
this.uploadCollection.size === 1 &&
this.cfg.useCloudImageEditor &&
this.hasBlockInCtx((block) => block.activityType === LitActivityBlock.activities.CLOUD_IMG_EDIT)
) {
+ if (!this.modalManager) return;
this.$['*currentActivityParams'] = {
internalId: entry.uid,
};
this.$['*currentActivity'] = LitActivityBlock.activities.CLOUD_IMG_EDIT;
- this.modalManager?.open(LitActivityBlock.activities.CLOUD_IMG_EDIT);
+ this.modalManager.open(LitActivityBlock.activities.CLOUD_IMG_EDIT);
}🤖 Prompt for AI Agents
In src/lit/LitUploaderBlock.ts around lines 369 to 377, the code sets
this.$['*currentActivityParams'] and this.$['*currentActivity'] unconditionally
but calls this.modalManager?.open(), which can leave activity state set when
modalManager is null; change the logic to first check that modalManager exists
(and modalManager.open is callable) and only then set the activity params and
activity and call open, or alternatively clear/unset the activity when
modalManager is absent; implement the guard so state and UI remain consistent
(i.e., wrap the two this.$[...] assignments and the open() call in a
modalManager existence check or ensure state is rolled back if modalManager is
missing).
| '*collectionErrors': OutputErrorCollection[]; | ||
| '*collectionState': OutputCollectionState | null; | ||
| '*groupInfo': UploadcareGroup | null; | ||
| '*uploadTrigger': Set<Uid>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "uploadTrigger" --type ts src/abstract/CTX.ts -B2 -A2Repository: uploadcare/file-uploader
Length of output: 234
🏁 Script executed:
rg -n "uploadTrigger" --type ts src/lit/SharedState.ts -B2 -A2Repository: uploadcare/file-uploader
Length of output: 208
Fix type mismatch for *uploadTrigger.
The type declaration in SharedState.ts line 46 specifies Set<Uid>, but CTX.ts line 30 initializes it as new Set<string>(). Update the initialization to use the branded Uid type for consistency.
🤖 Prompt for AI Agents
In src/lit/SharedState.ts around line 46, the property '*uploadTrigger' is
declared as Set<Uid> but elsewhere (CTX.ts line 30) it's initialized as new
Set<string>(); change the initialization to use the branded Uid type (new
Set<Uid>()) or cast the created set to Set<Uid> and ensure any additions are of
type Uid so the declaration and initialization types match.
| <button | ||
| type="button" | ||
| class="uc-secondary-btn" | ||
| @click=${this.$['*historyBack']} | ||
| >${this.l10n('start-from-cancel')}</button> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts '\*historyBack' -A 3Repository: uploadcare/file-uploader
Length of output: 2638
Use optional chaining to safely handle the potentially null *historyBack property.
The type definition shows '*historyBack': (() => void) | null (initialized as null), so direct template binding risks errors. Other components like FileUploaderInline.ts handle this safely with optional chaining:
const historyBack = this.$['*historyBack'] as (() => void) | undefined;
historyBack?.();Apply the same defensive pattern here.
🤖 Prompt for AI Agents
In src/solutions/file-uploader/minimal/FileUploaderMinimal.ts around lines 159
to 163, the template binds this.$['*historyBack'] directly which can be null;
change the click handler to safely call it using optional chaining: read the
property into a const typed as (() => void) | undefined (or cast to that type)
and invoke it with historyBack?.(); update the template @click to call that safe
wrapper or point it to a method that does the same.
| import { page } from '@vitest/browser/context'; | ||
| import { beforeAll, beforeEach, describe, it } from 'vitest'; |
There was a problem hiding this comment.
Inconsistent import path with other test files.
This file imports page from @vitest/browser/context, while tests/api.e2e.test.tsx (also in this PR) imports from vitest/browser. According to the AI summary, the broader PR migration moves test files to the public vitest/browser module. This inconsistency should be resolved.
Apply this diff to align with the migration pattern:
-import { page } from '@vitest/browser/context';
+import { page } from 'vitest/browser';
import { beforeAll, beforeEach, describe, it } from 'vitest';📝 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 { page } from '@vitest/browser/context'; | |
| import { beforeAll, beforeEach, describe, it } from 'vitest'; | |
| import { page } from 'vitest/browser'; | |
| import { beforeAll, beforeEach, describe, it } from 'vitest'; |
🤖 Prompt for AI Agents
In tests/adaptive-image.e2e.test.tsx around lines 1 to 2, the import for `page`
uses the private path `@vitest/browser/context` which is inconsistent with the
PR migration to the public module; change that import to import `page` from
`vitest/browser` (keeping the other vitest imports as-is) so the file matches
tests/api.e2e.test.tsx and the public `vitest/browser` API surface.
| @@ -1,5 +1,5 @@ | |||
| import { page } from '@vitest/browser/context'; | |||
| import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; | |||
| import { page } from 'vitest/browser'; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the vitest/browser import path and check for old imports
echo "=== Checking vitest package exports ==="
if [ -f "node_modules/vitest/package.json" ]; then
jq '.exports | keys' node_modules/vitest/package.json 2>/dev/null || echo "Could not read package.json"
else
echo "Vitest package.json not found"
fi
echo ""
echo "=== Checking for @vitest/browser/context imports ==="
rg "@vitest/browser/context" --type ts --type tsx || echo "No matches found"
echo ""
echo "=== Checking for vitest/browser imports ==="
rg "vitest/browser" --type ts --type tsx || echo "No matches found"
echo ""
echo "=== Checking the specific file ==="
head -5 tests/api.e2e.test.tsxRepository: uploadcare/file-uploader
Length of output: 625
🏁 Script executed:
#!/bin/bash
# Search more broadly for import patterns
echo "=== Checking for @vitest/browser/context imports (no type filter) ==="
rg "@vitest/browser/context" . 2>/dev/null || echo "No matches found"
echo ""
echo "=== Checking for vitest/browser imports (no type filter) ==="
rg "vitest/browser" . 2>/dev/null || echo "No matches found"
echo ""
echo "=== Checking for @vitest/browser imports in general ==="
rg "@vitest/browser" . 2>/dev/null || echo "No matches found"
echo ""
echo "=== Listing all test files ==="
find . -name "*.test.*" -o -name "*.spec.*" 2>/dev/null | head -20Repository: uploadcare/file-uploader
Length of output: 6831
Import path migration is correct, but the migration is incomplete across the test suite.
The change from @vitest/browser/context to vitest/browser is valid—the new import path is already in use in other test files (file-uploader-regular.e2e.test.tsx, validation.e2e.test.tsx, config.e2e.test.tsx, bundles.e2e.test.tsx, and utils/test-renderer.tsx). However, four other test files still use the deprecated import path:
tests/file-uploader-minimal.e2e.test.tsxtests/file-uploader-inline.e2e.test.tsxtests/cloud-image-editor.e2e.test.tsxtests/adaptive-image.e2e.test.tsx
Update these remaining files to use the new import path for consistency.
🤖 Prompt for AI Agents
In tests/api.e2e.test.tsx around line 2 and across the test suite, the import
path was partially migrated from @vitest/browser/context to vitest/browser;
update the remaining files tests/file-uploader-minimal.e2e.test.tsx,
tests/file-uploader-inline.e2e.test.tsx, tests/cloud-image-editor.e2e.test.tsx,
and tests/adaptive-image.e2e.test.tsx to replace any imports of
"@vitest/browser/context" with "vitest/browser" (keeping named imports the same)
so all tests use the new path consistently.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.