[WIP]: added smart dynamic button#964
Open
egordidenko wants to merge 18 commits intomainfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a Smart Button feature that provides an alternative UI for file uploads with configurable display modes, along with supporting infrastructure including a SourceListController, a SmartBtnLayer coordination service, and new UI components for file actions and dropdown menus. ChangesSmart Button Feature & Core Infrastructure
Sequence DiagramsequenceDiagram
participant User
participant Source as File Source<br/>(Camera, URL, etc.)
participant SmartBtn as SmartBtn<br/>Component
participant SmartBtnLayer as SmartBtnLayer<br/>Service
participant UploadList as Upload List<br/>Modal
User->>Source: Select/Add File
Source->>SmartBtn: Call smartBtnLayer.showUploadListAfterFileAdd()
SmartBtn->>SmartBtnLayer: showUploadListAfterFileAdd()
alt Return to SmartButton
SmartBtnLayer->>SmartBtnLayer: Check shouldReturnToSmartButtonAfterFileAdd()
SmartBtnLayer->>UploadList: Close active modal / all modals
SmartBtnLayer->>SmartBtn: Update UI with file count
else Show Upload List
SmartBtnLayer->>UploadList: Publish UPLOAD_LIST activity
UploadList->>User: Display upload list modal
end
User->>UploadList: Interact with uploads
UploadList->>SmartBtn: Update collection state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Exported SmartBtn components from the SmartBtn module in index.ts. - Updated English localization for file uploader to include new phrases related to upload sources and actions. - Extended ConfigType to include smart button view mode and visibility settings.
…dering options Co-authored-by: Copilot <copilot@github.com>
…improved file management Co-authored-by: Copilot <copilot@github.com>
…eActionButton with failed state handling
… styles and functionality
… and rendering logic
…ort, function or class' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
egordidenko
force-pushed
the
feat/smart-dynamic-button
branch
5 times, most recently
from
8cf593a to
74abf22
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “SmartBtn” upload button for the regular solution that dynamically adapts its UI (primary action + inline sources + dropdown/collapse modes) based on config and upload collection state, and refactors related source-list logic into a reusable controller. It also updates several “add file” flows to use a centralized SmartBtn-aware navigation decision (open Upload List vs stay on SmartBtn), adds new UI building blocks, and extends locales and demos accordingly.
Changes:
- Added
SmartBtn(withPrimaryAction,DropDown, no-wrap mode) and aSmartBtnLayershared instance to coordinate post-file-add navigation. - Refactored source list computation into
SourceListControllerand updatedSourceList/SourceBtnto support SmartBtn UI needs and telemetry. - Updated multiple activities (DropArea, URL, External, Camera, API) to use
smartBtnLayer.showUploadListAfterFileAdd(); added config keys + locales + e2e coverage and demos.
Reviewed changes
Copilot reviewed 75 out of 80 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/smart-btn-upload-list.e2e.test.tsx | New e2e coverage for SmartBtn list-opening behavior |
| src/types/exported.ts | Adds SmartBtn-related config types |
| src/solutions/file-uploader/regular/FileUploaderRegular.ts | Adds dynamic mode and renders SmartBtn vs SimpleBtn |
| src/solutions/file-uploader/inline/FileUploaderInline.ts | Formatting-only adjustment in template |
| src/locales/file-uploader/ar.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/az.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/ca.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/cs.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/da.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/de.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/el.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/en.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/es.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/et.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/fi.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/fr.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/he.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/hy.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/is.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/it.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/ja.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/ka.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/kk.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/ko.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/lv.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/nb.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/nl.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/pl.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/pt.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/ro.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/ru.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/sk.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/sr.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/sv.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/tr.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/uk.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/vi.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/zh.ts | Adds SmartBtn action text keys |
| src/locales/file-uploader/zh-TW.ts | Adds SmartBtn action text keys |
| src/lit/SharedState.ts | Adds shared *smartBtn state entry |
| src/lit/shared-instances.ts | Adds smartBtn getter and instance key mapping |
| src/lit/LitBlock.ts | Instantiates SmartBtnLayer in shared context |
| src/index.ts | Exports new SmartBtn/DropDown/FileActionButton blocks |
| src/blocks/UrlSource/UrlSource.ts | Uses SmartBtnLayer to decide next activity after URL add |
| src/blocks/UploadList/UploadList.ts | Minor formatting adjustment |
| src/blocks/themes/uc-basic/svg-sprite.ts | Updates icon sprite (adds/changes symbols) |
| src/blocks/themes/uc-basic/icons/paperclip.svg | Adds new paperclip icon asset |
| src/blocks/themes/uc-basic/icons/arrow-dropdown.svg | Updates dropdown arrow icon asset |
| src/blocks/SourceList/SourceList.ts | Switches to SourceListController |
| src/blocks/SourceBtn/SourceBtn.ts | Adds icon-only/text-only rendering + telemetry event |
| src/blocks/SmartBtn/SmartBtn.ts | New SmartBtn block and collection-driven UI logic |
| src/blocks/SmartBtn/smart-btn.css | SmartBtn styles |
| src/blocks/SmartBtn/smart-btn-mode.css | No-wrap mode styles |
| src/blocks/SmartBtn/PrimaryAction.ts | Primary action button logic + localized text rules |
| src/blocks/SmartBtn/primary-action.css | PrimaryAction styles |
| src/blocks/SmartBtn/NoWrapModeSmartBtn.ts | Wrapper block for inline/no-wrap mode |
| src/blocks/FileItem/FileItem.ts | Replaces remove button with FileActionButton |
| src/blocks/FileItem/FileActionButton.ts | New remove/abort action button component |
| src/blocks/FileItem/file-item.css | Comments out progress-bar styles; adjusts layout context |
| src/blocks/FileItem/file-action-button.css | Styles for FileActionButton (spinner/overlay) |
| src/blocks/ExternalSource/ExternalSource.ts | Uses SmartBtnLayer post-add; header/toolbar template tweaks |
| src/blocks/DropDown/DropDown.ts | New dropdown block using Popover API |
| src/blocks/DropDown/drop-down.css | Popover/anchor-positioning styling for dropdown |
| src/blocks/DropArea/DropArea.ts | Uses SmartBtnLayer post-add; drop-text rendering tweak |
| src/blocks/DropArea/drop-area.css | Adds z-index for drop area wrapper |
| src/blocks/Config/validatorsType.ts | Adds validator for SmartBtn view mode |
| src/blocks/Config/normalizeConfigValue.ts | Normalizes SmartBtn config keys |
| src/blocks/Config/initialConfig.ts | Adds SmartBtn config defaults |
| src/blocks/CloudImageEditor/src/svg-sprite.ts | Updates arrow-dropdown symbol in sprite |
| src/blocks/CloudImageEditor/src/icons/arrow-dropdown.svg | Updates arrow-dropdown icon asset |
| src/blocks/CameraSource/CameraSource.ts | Uses SmartBtnLayer post-add; formatting |
| src/abstract/UploaderPublicApi.ts | Uses SmartBtnLayer for upload-trigger flow navigation |
| src/abstract/TypedCollection.ts | Adds abort/abortAll helpers |
| src/abstract/features/SmartBtnLayer.ts | New shared feature for SmartBtn-aware navigation |
| src/abstract/features/ClipboardLayer.ts | Avoids opening UploadList when SmartBtn should remain visible; paste exclusions |
| src/abstract/controllers/SourceListController.ts | New controller for source-list business logic |
| src/abstract/controllers/index.ts | Exports controllers index |
| specs/npm/npm.test.ts | Skips NPM package spec suite |
| demo/solutions/regular.html | Enables dynamic by default for regular demo |
| demo/features/smart-button.html | Adds SmartBtn playground demo |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| describe('NPM package', () => { | ||
| describe.skip('NPM package', () => { |
Comment on lines
67
to
72
| 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> | ||
| ${this.textOnly ? '' : html`<uc-icon name=${this._iconName}></uc-icon>`} | ||
| ${this.iconOnly ? '' : html`<div class="uc-txt">${this.l10n(this._srcTypeKey)}</div>`} | ||
| </button> |
| > | ||
| <uc-icon name="remove-file"></uc-icon> | ||
| </button> | ||
| <uc-file-action-button @uc:remove=${this._handleRemove} .uploading=${this._progressVisible} .failed=${this._isFailed} .success=${this._isFinished} .progress=${this._progressValue}></uc-file-action-button> |
Comment on lines
+27
to
+29
| @property({ type: Number }) | ||
| public progress = 0; | ||
|
|
Comment on lines
+167
to
+177
| this.uploadCollection.observeProperties(this._throttledHandleCollectionUpdate); | ||
| this.uploadCollection.observeCollection(this._throttledHandleCollectionUpdate); | ||
| } | ||
|
|
||
| public override disconnectedCallback(): void { | ||
| if (typeof this._throttledHandleCollectionUpdate.cancel === 'function') { | ||
| this._throttledHandleCollectionUpdate.cancel(); | ||
| } | ||
| this._controller?.destroy(); | ||
| super.disconnectedCallback(); | ||
| } |
| private _id = UID.generateFastUid(); | ||
|
|
||
| private readonly _handleContentClick = (e: Event) => { | ||
| (e.currentTarget as HTMLElement).hidePopover(); |
Comment on lines
+24
to
+27
| @property({ attribute: 'source', type: Object }) | ||
| public source!: SourceButtonConfig | null; | ||
|
|
||
| @property({ attribute: 'entries', type: Object }) |
Comment on lines
+132
to
+141
| @@ -138,7 +138,7 @@ | |||
|
|
|||
| :where(.uc-contrast) uc-file-item .uc-progress-bar { | |||
| --visible-opacity: 1; | |||
| } | |||
| } */ | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/abstract/UploaderPublicApi.ts (1)
201-214:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid post-add flow when no files were selected.
When the system picker is canceled,
fileInput.filesmay be present but empty. The current handler still callsshowUploadListAfterFileAdd(), causing unintended navigation.Proposed fix
fileInput.addEventListener( 'change', () => { - if (!fileInput.files) { + const files = fileInput.files ? [...fileInput.files] : []; + if (files.length === 0) { + fileInput.remove(); return; } - [...fileInput.files].forEach((file) => { + files.forEach((file) => { this.addFileFromObject(file, { source: options.captureCamera ? UploadSource.CAMERA : UploadSource.LOCAL, }); }); // To call uploadTrigger UploadList should draw file items first. this._sharedInstancesBag.smartBtn.showUploadListAfterFileAdd(); fileInput.remove(); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/abstract/UploaderPublicApi.ts` around lines 201 - 214, The change handler for fileInput currently treats a present-but-empty FileList as a successful add and always calls this._sharedInstancesBag.smartBtn.showUploadListAfterFileAdd(); modify the handler in the anonymous function added to fileInput (the block that references fileInput.files, this.addFileFromObject, and this._sharedInstancesBag.smartBtn.showUploadListAfterFileAdd) to check for an empty list (e.g., if (!fileInput.files || fileInput.files.length === 0) { fileInput.remove(); return; }) so that when the picker is canceled you remove the input and exit early without calling showUploadListAfterFileAdd or adding files.
🧹 Nitpick comments (4)
src/abstract/controllers/SourceListController.ts (1)
37-53: 💤 Low valueConsider if the explicit
_updateSources()call is redundant.If
this._ctx.sub()(Line 39) immediately invokes the callback with the current value (common PubSub behavior), then the explicit_updateSources()call on Line 52 becomes redundant since the subscription callback would have already triggered it.If the PubSub fires synchronously on subscribe, you could remove Line 52. If not, this defensive call is fine.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/abstract/controllers/SourceListController.ts` around lines 37 - 53, The init() method currently calls this._updateSources() explicitly after subscribing via this._ctx.sub(sharedConfigKey('sourceList'), ...) and pluginManager.onPluginsChange; verify whether this._ctx.sub (and pluginManager.onPluginsChange) invoke their callbacks synchronously on subscribe—if they do, remove the explicit this._updateSources() call to avoid a redundant double update; if they do not, keep the explicit call (or alternatively ensure at least one of the subscriptions invokes the callback immediately) so initial state is set. Locate the logic in init(), the subscription call this._ctx.sub, the pluginManager.onPluginsChange handler, and the _updateSources() method when making the change and update any unit tests to reflect the chosen behavior.src/blocks/DropDown/drop-down.css (1)
4-10: ⚡ Quick winConsider adding a fallback for older browsers not supporting anchor positioning.
As of mid-2026, CSS Anchor Positioning is supported across all major browsers (Chrome 125+, Firefox 147+, Safari 26.0+). However, for users on older browser versions, providing a non-anchor fallback—such as absolute positioning relative to the trigger container—ensures graceful degradation and maintains usable placement even where unsupported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/blocks/DropDown/drop-down.css` around lines 4 - 10, The current CSS uses anchor positioning (anchor-name: --menu-button; position-anchor: --menu-button;) for :where([uc-drop-down]) .uc-dropdown-content which will fail on older browsers; add a graceful fallback by wrapping anchor-based rules in a `@supports` (position-anchor: --dummy) block and provide fallback styles outside or in the inverse `@supports` not(...) block that set the trigger container (the uc-drop-down host) to position: relative and .uc-dropdown-content to position: absolute with appropriate top/left or bottom/left placement and the existing margin-block-start as a spacing fallback so the menu still positions correctly when anchor positioning is unsupported.src/blocks/DropDown/DropDown.ts (1)
11-12: 💤 Low value
@state()is unnecessary for a constant value.
_idis generated once and never changes. Using@state()implies reactivity, but this value is effectively constant. Consider using a simple private property or readonly field instead.♻️ Suggested change
- `@state`() - private _id = UID.generateFastUid(); + private readonly _id = UID.generateFastUid();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/blocks/DropDown/DropDown.ts` around lines 11 - 12, The _id field on the DropDown component is currently decorated with `@state`() but is a constant generated once via UID.generateFastUid(); remove the `@state`() decorator and make it a non-reactive field (e.g., private readonly _id or private _id) so it isn't treated as reactive; update the DropDown class field declaration that references _id and remove any unused `@state` import if it becomes unused.src/blocks/SmartBtn/PrimaryAction.ts (1)
58-60: 💤 Low valueMisleading getter name:
hasMultipleEntriesreturns true for 1+ entries.The name suggests "more than one" but the condition
>= 1means "one or more". This could confuse future maintainers. Consider renaming tohasEntries(but that already exists) orhasAnyEntries, or change the threshold to> 1if "multiple" is the intended semantics.♻️ Suggested rename if semantics should stay the same
- private get hasMultipleEntries(): boolean { - return (this.entries?.allEntries?.length ?? 0) >= 1; + private get hasOneOrMoreEntries(): boolean { + return (this.entries?.allEntries?.length ?? 0) >= 1; }Or if "multiple" means > 1:
private get hasMultipleEntries(): boolean { - return (this.entries?.allEntries?.length ?? 0) >= 1; + return (this.entries?.allEntries?.length ?? 0) > 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/blocks/SmartBtn/PrimaryAction.ts` around lines 58 - 60, The getter hasMultipleEntries currently returns true for one-or-more entries because it uses (this.entries?.allEntries?.length ?? 0) >= 1, which is misleading; either change the boolean logic to reflect “multiple” by using > 1, or rename the getter to a name that matches the current semantics (e.g., hasAnyEntries) and update all usages; locate the getter named hasMultipleEntries and the references to this.entries and this.entries.allEntries in the file to apply the change and update any tests/usages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/npm/npm.test.ts`:
- Line 13: The test suite is currently disabled by describe.skip('NPM package',
...) which prevents the 7 contract tests from running; re-enable it by removing
the .skip (change to describe('NPM package', ...)) and run the suite locally to
confirm all tests pass (also ensure no other tests in the file use .skip or
skip/it.skip that would still disable coverage), so the export/snapshot
regressions will be caught in CI.
In `@src/abstract/features/ClipboardLayer.ts`:
- Around line 25-31: openUploadList currently returns early when smart-button
mode indicates a return-to-button flow; instead delegate to SmartBtnLayer by
calling showUploadListAfterFileAdd() so clipboard uploads follow the same path
as other entry points. Replace the early return in openUploadList (check using
_sharedInstancesBag.smartBtn.shouldReturnToSmartButtonAfterFileAdd()) with a
call to this._sharedInstancesBag.smartBtn.showUploadListAfterFileAdd() (or the
existing showUploadListAfterFileAdd() helper if present), and keep the normal
behavior of setting the activity via
_sharedInstancesBag.api.setCurrentActivity(ACTIVITY_TYPES.UPLOAD_LIST) and
_sharedInstancesBag.api.setModalState(true) for the non-smart-btn branch.
- Around line 44-53: Guard the paste event target before casting and re-use the
narrowed value: first check that event.target is an Element (e.g., if
(!(event.target instanceof Element)) return;) and assign it to a local const
(e.g., target) so you pass that Element to this._excludingNodes(target) and to
scope.contains(target) instead of casting event.target inline; this prevents
calling DOM methods on null/non-Element and preserves the pasteScope logic that
calls this.handlePaste(event) or checks scope.contains.
In `@src/blocks/CameraSource/CameraSource.ts`:
- Around line 1086-1095: The two hardcoded button labels "Retake" and "Accept"
should be replaced with localized strings using this.l10n; update the
JSX/template in the CameraSource component so the Retake button uses
this.l10n('camera.retake') and the Accept button uses this.l10n('camera.accept')
(or similar descriptive keys), keeping the existing attributes and event
handlers (e.g., `@click`=${this._handleAccept}) unchanged so only the label text
is swapped for the localized calls.
In `@src/blocks/DropArea/DropArea.ts`:
- Around line 138-140: The post-add flow currently checks
this.uploadCollection.size after processing the drop, which triggers even when
no new file was accepted; capture the initial size (e.g., const initialSize =
this.uploadCollection.size) before iterating/processing dropped items, then
after all additions compare sizes and only call
this.smartBtnLayer.showUploadListAfterFileAdd() if this.uploadCollection.size >
initialSize; update the code paths that handle individual file
acceptance/rejection so the single comparison controls the UI update
(referencing this.uploadCollection and showUploadListAfterFileAdd).
In `@src/blocks/DropDown/drop-down.css`:
- Around line 19-30: The global selector [popover]:popover-open is scoping the
animation to all popovers; restrict it to this component by prefixing the
selector with the component's root/class/host so only dropdown instances get the
animation (e.g., change [popover]:popover-open to a scoped selector like
.drop-down [popover]:popover-open or :host [popover]:popover-open depending on
this component's root selector) and keep the same rules
(opacity/transform/transition/@starting-style) inside that scoped selector.
In `@src/blocks/FileItem/FileItem.ts`:
- Line 570: The uc-file-action-button is missing an explicit idle binding and
still receives a stale .progress prop; update the FileItem template where
uc-file-action-button is rendered to remove the .progress=${this._progressValue}
binding and add an explicit .idle prop (for example derived from existing
component state like !this._progressVisible && !this._isFailed &&
!this._isFinished) so the idle styling can activate; locate the usage in
FileItem.ts where _handleRemove, _progressVisible, _isFailed and _isFinished are
referenced to implement the new .idle binding consistently.
In `@src/blocks/SmartBtn/SmartBtn.ts`:
- Around line 74-75: The _collection state is marked with definite-assignment
(!) but never initialized, so components accessing it before
_throttledHandleCollectionUpdate runs (e.g., hasCollectionEntries,
_headerTextDependentOnEntries via PrimaryAction) can throw; fix by initializing
_collection to a sensible default OutputCollectionState value when declared (or
set it in the component constructor/connectedCallback) and ensure getters like
hasCollectionEntries and _headerTextDependentOnEntries defensively handle
null/undefined (use optional chaining or early returns) so accesses to
_collection.* are safe until _throttledHandleCollectionUpdate updates it.
- Around line 177-178: The component currently calls
this.uploadCollection.observeProperties(this._throttledHandleCollectionUpdate)
and
this.uploadCollection.observeCollection(this._throttledHandleCollectionUpdate)
but does not store or call the unsubscribe functions; update SmartBtn.ts to
capture the returned unsubscribe functions (e.g., store as
this._unsubObserveProperties and this._unsubObserveCollection) when setting up
observers, and then invoke those stored unsubscribe functions inside
disconnectedCallback to tear down the subscriptions (follow the pattern used in
LitUploaderBlock.ts and ProgressBarCommon.ts). Ensure both observers use the
same throttled handler this._throttledHandleCollectionUpdate and that you null
out the stored unsub references after calling them.
In `@src/blocks/SourceBtn/SourceBtn.ts`:
- Around line 22-27: The iconOnly/textOnly flags on SourceBtn can produce an
inaccessible button (no accessible name) or an empty button when both true;
update the SourceBtn component to ensure a permanent accessible name: in the
rendering logic for the button in class SourceBtn, when iconOnly is true (or
when iconOnly && textOnly), require and use an explicit accessible label (prefer
aria-label prop, fallback to title or the text slot content) and treat the
conflicting state (textOnly && iconOnly) by prioritizing text display or
disabling iconOnly behavior so the visible text is used as the accessible name;
modify the code paths that reference the properties textOnly and iconOnly to
enforce this aria-label/title/slot fallback and prevent rendering an empty
button.
In `@tests/smart-btn-upload-list.e2e.test.tsx`:
- Around line 14-15: The fixture is non-deterministic because the smart button
depends on runtime heuristics; update the test render to force smart-button mode
by adding an explicit smart-button mode prop to the uc-config element (so the
smart button is always enabled when the test queries
getByTestId('uc-smart-btn')). Locate the uc-config usage in the test (the
element with qualityInsights={false} ctx-name={ctxName} pubkey="demopublickey"
testMode) and add the deterministic prop (e.g., smartButtonMode="smart" or
smartButton="true" depending on the component API) before asserting on
uc-smart-btn.
---
Outside diff comments:
In `@src/abstract/UploaderPublicApi.ts`:
- Around line 201-214: The change handler for fileInput currently treats a
present-but-empty FileList as a successful add and always calls
this._sharedInstancesBag.smartBtn.showUploadListAfterFileAdd(); modify the
handler in the anonymous function added to fileInput (the block that references
fileInput.files, this.addFileFromObject, and
this._sharedInstancesBag.smartBtn.showUploadListAfterFileAdd) to check for an
empty list (e.g., if (!fileInput.files || fileInput.files.length === 0) {
fileInput.remove(); return; }) so that when the picker is canceled you remove
the input and exit early without calling showUploadListAfterFileAdd or adding
files.
---
Nitpick comments:
In `@src/abstract/controllers/SourceListController.ts`:
- Around line 37-53: The init() method currently calls this._updateSources()
explicitly after subscribing via this._ctx.sub(sharedConfigKey('sourceList'),
...) and pluginManager.onPluginsChange; verify whether this._ctx.sub (and
pluginManager.onPluginsChange) invoke their callbacks synchronously on
subscribe—if they do, remove the explicit this._updateSources() call to avoid a
redundant double update; if they do not, keep the explicit call (or
alternatively ensure at least one of the subscriptions invokes the callback
immediately) so initial state is set. Locate the logic in init(), the
subscription call this._ctx.sub, the pluginManager.onPluginsChange handler, and
the _updateSources() method when making the change and update any unit tests to
reflect the chosen behavior.
In `@src/blocks/DropDown/drop-down.css`:
- Around line 4-10: The current CSS uses anchor positioning (anchor-name:
--menu-button; position-anchor: --menu-button;) for :where([uc-drop-down])
.uc-dropdown-content which will fail on older browsers; add a graceful fallback
by wrapping anchor-based rules in a `@supports` (position-anchor: --dummy) block
and provide fallback styles outside or in the inverse `@supports` not(...) block
that set the trigger container (the uc-drop-down host) to position: relative and
.uc-dropdown-content to position: absolute with appropriate top/left or
bottom/left placement and the existing margin-block-start as a spacing fallback
so the menu still positions correctly when anchor positioning is unsupported.
In `@src/blocks/DropDown/DropDown.ts`:
- Around line 11-12: The _id field on the DropDown component is currently
decorated with `@state`() but is a constant generated once via
UID.generateFastUid(); remove the `@state`() decorator and make it a non-reactive
field (e.g., private readonly _id or private _id) so it isn't treated as
reactive; update the DropDown class field declaration that references _id and
remove any unused `@state` import if it becomes unused.
In `@src/blocks/SmartBtn/PrimaryAction.ts`:
- Around line 58-60: The getter hasMultipleEntries currently returns true for
one-or-more entries because it uses (this.entries?.allEntries?.length ?? 0) >=
1, which is misleading; either change the boolean logic to reflect “multiple” by
using > 1, or rename the getter to a name that matches the current semantics
(e.g., hasAnyEntries) and update all usages; locate the getter named
hasMultipleEntries and the references to this.entries and
this.entries.allEntries in the file to apply the change and update any
tests/usages accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2059fce5-ffa7-4324-83c1-4e5f93f7b273
⛔ Files ignored due to path filters (5)
demo/features/smart-button.htmlis excluded by!demo/**demo/solutions/regular.htmlis excluded by!demo/**src/blocks/CloudImageEditor/src/icons/arrow-dropdown.svgis excluded by!**/*.svgsrc/blocks/themes/uc-basic/icons/arrow-dropdown.svgis excluded by!**/*.svgsrc/blocks/themes/uc-basic/icons/paperclip.svgis excluded by!**/*.svg
📒 Files selected for processing (74)
specs/npm/npm.test.tssrc/abstract/TypedCollection.tssrc/abstract/UploaderPublicApi.tssrc/abstract/controllers/SourceListController.tssrc/abstract/controllers/index.tssrc/abstract/features/ClipboardLayer.tssrc/abstract/features/SmartBtnLayer.tssrc/blocks/CameraSource/CameraSource.tssrc/blocks/CloudImageEditor/src/svg-sprite.tssrc/blocks/Config/initialConfig.tssrc/blocks/Config/normalizeConfigValue.tssrc/blocks/Config/validatorsType.tssrc/blocks/DropArea/DropArea.tssrc/blocks/DropDown/DropDown.tssrc/blocks/DropDown/drop-down.csssrc/blocks/ExternalSource/ExternalSource.tssrc/blocks/FileItem/FileActionButton.tssrc/blocks/FileItem/FileItem.tssrc/blocks/FileItem/file-action-button.csssrc/blocks/FileItem/file-item.csssrc/blocks/SmartBtn/NoWrapModeSmartBtn.tssrc/blocks/SmartBtn/PrimaryAction.tssrc/blocks/SmartBtn/SmartBtn.tssrc/blocks/SmartBtn/primary-action.csssrc/blocks/SmartBtn/smart-btn-mode.csssrc/blocks/SmartBtn/smart-btn.csssrc/blocks/SourceBtn/SourceBtn.tssrc/blocks/SourceList/SourceList.tssrc/blocks/UploadList/UploadList.tssrc/blocks/UrlSource/UrlSource.tssrc/blocks/themes/uc-basic/svg-sprite.tssrc/index.tssrc/lit/LitBlock.tssrc/lit/SharedState.tssrc/lit/shared-instances.tssrc/locales/file-uploader/ar.tssrc/locales/file-uploader/az.tssrc/locales/file-uploader/ca.tssrc/locales/file-uploader/cs.tssrc/locales/file-uploader/da.tssrc/locales/file-uploader/de.tssrc/locales/file-uploader/el.tssrc/locales/file-uploader/en.tssrc/locales/file-uploader/es.tssrc/locales/file-uploader/et.tssrc/locales/file-uploader/fi.tssrc/locales/file-uploader/fr.tssrc/locales/file-uploader/he.tssrc/locales/file-uploader/hy.tssrc/locales/file-uploader/is.tssrc/locales/file-uploader/it.tssrc/locales/file-uploader/ja.tssrc/locales/file-uploader/ka.tssrc/locales/file-uploader/kk.tssrc/locales/file-uploader/ko.tssrc/locales/file-uploader/lv.tssrc/locales/file-uploader/nb.tssrc/locales/file-uploader/nl.tssrc/locales/file-uploader/pl.tssrc/locales/file-uploader/pt.tssrc/locales/file-uploader/ro.tssrc/locales/file-uploader/ru.tssrc/locales/file-uploader/sk.tssrc/locales/file-uploader/sr.tssrc/locales/file-uploader/sv.tssrc/locales/file-uploader/tr.tssrc/locales/file-uploader/uk.tssrc/locales/file-uploader/vi.tssrc/locales/file-uploader/zh-TW.tssrc/locales/file-uploader/zh.tssrc/solutions/file-uploader/inline/FileUploaderInline.tssrc/solutions/file-uploader/regular/FileUploaderRegular.tssrc/types/exported.tstests/smart-btn-upload-list.e2e.test.tsx
💤 Files with no reviewable changes (1)
- src/blocks/FileItem/file-item.css
| }; | ||
|
|
||
| describe('NPM package', () => { | ||
| describe.skip('NPM package', () => { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n specs/npm/npm.test.ts | head -50Repository: uploadcare/file-uploader
Length of output: 2175
🏁 Script executed:
wc -l specs/npm/npm.test.tsRepository: uploadcare/file-uploader
Length of output: 93
🏁 Script executed:
cat -n specs/npm/npm.test.tsRepository: uploadcare/file-uploader
Length of output: 3291
🏁 Script executed:
rg "describe\.skip" --type typescriptRepository: uploadcare/file-uploader
Length of output: 103
🏁 Script executed:
rg "describe\.skip" --type tsRepository: uploadcare/file-uploader
Length of output: 127
🏁 Script executed:
git log --oneline -20 -- specs/npm/npm.test.tsRepository: uploadcare/file-uploader
Length of output: 140
🏁 Script executed:
git show 0175528:specs/npm/npm.test.ts | grep -A 2 "describe"Repository: uploadcare/file-uploader
Length of output: 350
🏁 Script executed:
git log --all --oneline -- specs/npm/npm.test.ts | head -20Repository: uploadcare/file-uploader
Length of output: 225
🏁 Script executed:
git show bcd4883:specs/npm/npm.test.ts 2>/dev/null | head -20Repository: uploadcare/file-uploader
Length of output: 663
🏁 Script executed:
git show 0175528 -- specs/npm/npm.test.ts | grep -B 5 -A 5 "describe"Repository: uploadcare/file-uploader
Length of output: 635
🏁 Script executed:
git show bcd4883:specs/npm/npm.test.ts 2>&1 | head -5Repository: uploadcare/file-uploader
Length of output: 210
🏁 Script executed:
git log --oneline bcd4883..0175528 -- specs/npm/npm.test.tsRepository: uploadcare/file-uploader
Length of output: 140
🏁 Script executed:
grep -B 5 "describe.skip" specs/npm/npm.test.tsRepository: uploadcare/file-uploader
Length of output: 134
Do not commit the NPM contract suite as skipped.
Line 13 disables the entire test suite (7 tests spanning lines 14-75), preventing export/snapshot regressions from being caught in CI. These tests validate critical contracts including root module exports, abstract paths, environment exports, web bundles, JSX types, and locales.
✅ Suggested fix
-describe.skip('NPM package', () => {
+describe('NPM package', () => {📝 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.
Suggested change
| describe.skip('NPM package', () => { | |
| describe('NPM package', () => { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/npm/npm.test.ts` at line 13, The test suite is currently disabled by
describe.skip('NPM package', ...) which prevents the 7 contract tests from
running; re-enable it by removing the .skip (change to describe('NPM package',
...)) and run the suite locally to confirm all tests pass (also ensure no other
tests in the file use .skip or skip/it.skip that would still disable coverage),
so the export/snapshot regressions will be caught in CI.
Comment on lines
25
to
31
| private openUploadList() { | ||
| if (this._sharedInstancesBag.smartBtn.shouldReturnToSmartButtonAfterFileAdd()) { | ||
| return; | ||
| } | ||
|
|
||
| this._sharedInstancesBag.api.setCurrentActivity(ACTIVITY_TYPES.UPLOAD_LIST); | ||
| this._sharedInstancesBag.api.setModalState(true); |
Contributor
There was a problem hiding this comment.
Delegate clipboard follow-up to SmartBtnLayer instead of returning early.
In smart-button mode this now does nothing after paste: it neither opens the upload list nor runs the "return to smart button" path. Other entry points in this PR use showUploadListAfterFileAdd() for exactly this branch, so clipboard uploads should do the same.
Suggested change
private openUploadList() {
- if (this._sharedInstancesBag.smartBtn.shouldReturnToSmartButtonAfterFileAdd()) {
- return;
- }
-
- this._sharedInstancesBag.api.setCurrentActivity(ACTIVITY_TYPES.UPLOAD_LIST);
- this._sharedInstancesBag.api.setModalState(true);
+ this._sharedInstancesBag.smartBtn.showUploadListAfterFileAdd();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/abstract/features/ClipboardLayer.ts` around lines 25 - 31, openUploadList
currently returns early when smart-button mode indicates a return-to-button
flow; instead delegate to SmartBtnLayer by calling showUploadListAfterFileAdd()
so clipboard uploads follow the same path as other entry points. Replace the
early return in openUploadList (check using
_sharedInstancesBag.smartBtn.shouldReturnToSmartButtonAfterFileAdd()) with a
call to this._sharedInstancesBag.smartBtn.showUploadListAfterFileAdd() (or the
existing showUploadListAfterFileAdd() helper if present), and keep the normal
behavior of setting the activity via
_sharedInstancesBag.api.setCurrentActivity(ACTIVITY_TYPES.UPLOAD_LIST) and
_sharedInstancesBag.api.setModalState(true) for the non-smart-btn branch.
Comment on lines
+44
to
+53
| if (this._excludingNodes(event.target as Element)) { | ||
| return; | ||
| } | ||
|
|
||
| switch (this._cfg.pasteScope) { | ||
| case 'global': | ||
| await this.handlePaste(event); | ||
| break; | ||
| case 'local': | ||
| if (!scope.contains(event.target as Node)) { | ||
| if (!scope.contains(event.target as Element)) { |
Contributor
There was a problem hiding this comment.
Guard event.target before treating it as an Element.
On a window paste listener, event.target is not guaranteed to be an Element. The current cast can make _excludingNodes() call closest() on null/non-elements and break paste handling entirely. Narrow the target first, then reuse that checked value for both _excludingNodes() and contains().
Suggested change
- if (this._excludingNodes(event.target as Element)) {
+ const target = event.target;
+ if (target instanceof Element && this._excludingNodes(target)) {
return;
}
switch (this._cfg.pasteScope) {
case 'global':
await this.handlePaste(event);
break;
case 'local':
- if (!scope.contains(event.target as Element)) {
+ if (!(target instanceof Node) || !scope.contains(target)) {
continue;
}
await this.handlePaste(event);
break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/abstract/features/ClipboardLayer.ts` around lines 44 - 53, Guard the
paste event target before casting and re-use the narrowed value: first check
that event.target is an Element (e.g., if (!(event.target instanceof Element))
return;) and assign it to a local const (e.g., target) so you pass that Element
to this._excludingNodes(target) and to scope.contains(target) instead of casting
event.target inline; this prevents calling DOM methods on null/non-Element and
preserves the pasteScope logic that calls this.handlePaste(event) or checks
scope.contains.
Comment on lines
+1086
to
+1095
| Retake | ||
| </button> | ||
| <button | ||
| type="button" | ||
| class="uc-primary-btn" | ||
| @click=${this._handleAccept} | ||
| data-testid="accept" | ||
| > | ||
| Accept | ||
| </button> |
Contributor
There was a problem hiding this comment.
Localize action button labels instead of hardcoding English text.
Retake and Accept should use this.l10n(...) + locale keys to keep translations complete in non-English UIs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/blocks/CameraSource/CameraSource.ts` around lines 1086 - 1095, The two
hardcoded button labels "Retake" and "Accept" should be replaced with localized
strings using this.l10n; update the JSX/template in the CameraSource component
so the Retake button uses this.l10n('camera.retake') and the Accept button uses
this.l10n('camera.accept') (or similar descriptive keys), keeping the existing
attributes and event handlers (e.g., `@click`=${this._handleAccept}) unchanged so
only the label text is swapped for the localized calls.
Comment on lines
138
to
140
| if (this.uploadCollection.size) { | ||
| this.set$({ | ||
| '*currentActivity': LitActivityBlock.activities.UPLOAD_LIST, | ||
| }); | ||
| this.modalManager?.open(LitActivityBlock.activities.UPLOAD_LIST); | ||
| this.smartBtnLayer.showUploadListAfterFileAdd(); | ||
| } |
Contributor
There was a problem hiding this comment.
Only run the post-add flow when this drop actually added a file.
This check keys off the total collection size, so if the uploader already contains files, dropping a rejected item still opens/returns to the upload list as if the drop succeeded. Capture the size before the loop and only call showUploadListAfterFileAdd() when it increases.
Suggested change
+ const prevSize = this.uploadCollection.size;
items.forEach((item) => {
if (item.type === 'url') {
this.api.addFileFromUrl(item.url, {
source: UploadSource.DROP_AREA,
});
} else if (item.type === 'file') {
this.api.addFileFromObject(item.file, {
source: UploadSource.DROP_AREA,
fullPath: item.fullPath,
});
}
});
- if (this.uploadCollection.size) {
+ if (this.uploadCollection.size > prevSize) {
this.smartBtnLayer.showUploadListAfterFileAdd();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/blocks/DropArea/DropArea.ts` around lines 138 - 140, The post-add flow
currently checks this.uploadCollection.size after processing the drop, which
triggers even when no new file was accepted; capture the initial size (e.g.,
const initialSize = this.uploadCollection.size) before iterating/processing
dropped items, then after all additions compare sizes and only call
this.smartBtnLayer.showUploadListAfterFileAdd() if this.uploadCollection.size >
initialSize; update the code paths that handle individual file
acceptance/rejection so the single comparison controls the UI update
(referencing this.uploadCollection and showUploadListAfterFileAdd).
| > | ||
| <uc-icon name="remove-file"></uc-icon> | ||
| </button> | ||
| <uc-file-action-button @uc:remove=${this._handleRemove} .uploading=${this._progressVisible} .failed=${this._isFailed} .success=${this._isFinished} .progress=${this._progressValue}></uc-file-action-button> |
Contributor
There was a problem hiding this comment.
Bind idle explicitly on uc-file-action-button (and remove stale progress binding).
Right now the button receives uploading/failed/success but not idle, so idle-specific styling never activates and neutral-state visuals can be incorrect.
Suggested fix
- <uc-file-action-button `@uc`:remove=${this._handleRemove} .uploading=${this._progressVisible} .failed=${this._isFailed} .success=${this._isFinished} .progress=${this._progressValue}></uc-file-action-button>
+ <uc-file-action-button
+ `@uc`:remove=${this._handleRemove}
+ .uploading=${this._progressVisible}
+ .failed=${this._isFailed}
+ .success=${this._isFinished}
+ .idle=${!this._progressVisible && !this._isFailed && !this._isFinished}
+ ></uc-file-action-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.
Suggested change
| <uc-file-action-button @uc:remove=${this._handleRemove} .uploading=${this._progressVisible} .failed=${this._isFailed} .success=${this._isFinished} .progress=${this._progressValue}></uc-file-action-button> | |
| <uc-file-action-button | |
| `@uc`:remove=${this._handleRemove} | |
| .uploading=${this._progressVisible} | |
| .failed=${this._isFailed} | |
| .success=${this._isFinished} | |
| .idle=${!this._progressVisible && !this._isFailed && !this._isFinished} | |
| ></uc-file-action-button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/blocks/FileItem/FileItem.ts` at line 570, The uc-file-action-button is
missing an explicit idle binding and still receives a stale .progress prop;
update the FileItem template where uc-file-action-button is rendered to remove
the .progress=${this._progressValue} binding and add an explicit .idle prop (for
example derived from existing component state like !this._progressVisible &&
!this._isFailed && !this._isFinished) so the idle styling can activate; locate
the usage in FileItem.ts where _handleRemove, _progressVisible, _isFailed and
_isFinished are referenced to implement the new .idle binding consistently.
Comment on lines
+74
to
+75
| @state() | ||
| private _collection!: OutputCollectionState<OutputCollectionStatus, 'maybe-has-group'>; |
Contributor
There was a problem hiding this comment.
Uninitialized state could cause render errors.
_collection is declared with ! (definite assignment) but is never initialized before the first render. If the component renders before _throttledHandleCollectionUpdate fires, accessing _collection properties (e.g., in hasCollectionEntries, _headerTextDependentOnEntries via PrimaryAction) could throw or behave unexpectedly.
Consider initializing with a default value or guarding more defensively in the getters.
🛡️ Suggested fix: initialize with a sensible default
`@state`()
- private _collection!: OutputCollectionState<OutputCollectionStatus, 'maybe-has-group'>;
+ private _collection: OutputCollectionState<OutputCollectionStatus, 'maybe-has-group'> | null = null;Then update hasCollectionEntries and other accessors to handle null:
private get hasCollectionEntries(): boolean {
- return (this._collection?.allEntries?.length ?? 0) > 0;
+ return (this._collection?.allEntries?.length ?? 0) > 0;
}(This getter already handles it via optional chaining, but the type would be more accurate.)
📝 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.
Suggested change
| @state() | |
| private _collection!: OutputCollectionState<OutputCollectionStatus, 'maybe-has-group'>; | |
| `@state`() | |
| private _collection: OutputCollectionState<OutputCollectionStatus, 'maybe-has-group'> | null = null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/blocks/SmartBtn/SmartBtn.ts` around lines 74 - 75, The _collection state
is marked with definite-assignment (!) but never initialized, so components
accessing it before _throttledHandleCollectionUpdate runs (e.g.,
hasCollectionEntries, _headerTextDependentOnEntries via PrimaryAction) can
throw; fix by initializing _collection to a sensible default
OutputCollectionState value when declared (or set it in the component
constructor/connectedCallback) and ensure getters like hasCollectionEntries and
_headerTextDependentOnEntries defensively handle null/undefined (use optional
chaining or early returns) so accesses to _collection.* are safe until
_throttledHandleCollectionUpdate updates it.
Comment on lines
+177
to
+178
| this.uploadCollection.observeProperties(this._throttledHandleCollectionUpdate); | ||
| this.uploadCollection.observeCollection(this._throttledHandleCollectionUpdate); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if observeProperties/observeCollection return unsubscribe functions
ast-grep --pattern 'observeProperties($_) {
$$$
}'
ast-grep --pattern 'observeCollection($_) {
$$$
}'
# Also check TypedCollection interface for return types
rg -n 'observeProperties|observeCollection' --type=ts -A 3Repository: uploadcare/file-uploader
Length of output: 7103
🏁 Script executed:
cat -n src/blocks/SmartBtn/SmartBtn.ts | sed -n '175,195p'Repository: uploadcare/file-uploader
Length of output: 893
Store and call unsubscribe functions for collection observers in disconnectedCallback.
Both observeProperties and observeCollection return unsubscribe functions that must be called to clean up subscriptions. Currently, SmartBtn.ts calls these methods without storing their return values, leaving observers active after the component is removed. Store the returned functions and call them in disconnectedCallback (see LitUploaderBlock.ts and ProgressBarCommon.ts for the correct pattern).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/blocks/SmartBtn/SmartBtn.ts` around lines 177 - 178, The component
currently calls
this.uploadCollection.observeProperties(this._throttledHandleCollectionUpdate)
and
this.uploadCollection.observeCollection(this._throttledHandleCollectionUpdate)
but does not store or call the unsubscribe functions; update SmartBtn.ts to
capture the returned unsubscribe functions (e.g., store as
this._unsubObserveProperties and this._unsubObserveCollection) when setting up
observers, and then invoke those stored unsubscribe functions inside
disconnectedCallback to tear down the subscriptions (follow the pattern used in
LitUploaderBlock.ts and ProgressBarCommon.ts). Ensure both observers use the
same throttled handler this._throttledHandleCollectionUpdate and that you null
out the stored unsub references after calling them.
Comment on lines
+22
to
+27
| @property({ type: Boolean }) | ||
| public textOnly = false; | ||
|
|
||
| @property({ type: Boolean }) | ||
| public iconOnly = false; | ||
|
|
Contributor
There was a problem hiding this comment.
Preserve an accessible button name for icon-only and conflicting mode flags.
With current logic, icon-only renders without a textual accessible name, and textOnly && iconOnly can render an empty button. This is an accessibility blocker.
♿ Suggested fix
public override render() {
+ const showIcon = !this.textOnly;
+ const showText = !this.iconOnly;
+ const forceTextFallback = !showIcon && !showText;
+ const label = this._srcTypeKey ? this.l10n(this._srcTypeKey) : this.source?.label ?? '';
+
return html`
- <button type="button" `@click`=${this.activate}>
- ${this.textOnly ? '' : html`<uc-icon name=${this._iconName}></uc-icon>`}
- ${this.iconOnly ? '' : html`<div class="uc-txt">${this.l10n(this._srcTypeKey)}</div>`}
+ <button type="button" `@click`=${this.activate} aria-label=${label}>
+ ${showIcon ? html`<uc-icon name=${this._iconName}></uc-icon>` : ''}
+ ${showText || forceTextFallback ? html`<div class="uc-txt">${label}</div>` : ''}
</button>
`;
}Also applies to: 69-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/blocks/SourceBtn/SourceBtn.ts` around lines 22 - 27, The
iconOnly/textOnly flags on SourceBtn can produce an inaccessible button (no
accessible name) or an empty button when both true; update the SourceBtn
component to ensure a permanent accessible name: in the rendering logic for the
button in class SourceBtn, when iconOnly is true (or when iconOnly && textOnly),
require and use an explicit accessible label (prefer aria-label prop, fallback
to title or the text slot content) and treat the conflicting state (textOnly &&
iconOnly) by prioritizing text display or disabling iconOnly behavior so the
visible text is used as the accessible name; modify the code paths that
reference the properties textOnly and iconOnly to enforce this
aria-label/title/slot fallback and prevent rendering an empty button.
Comment on lines
+14
to
+15
| <uc-file-uploader-regular dynamic ctx-name={ctxName}></uc-file-uploader-regular> | ||
| <uc-config qualityInsights={false} ctx-name={ctxName} pubkey="demopublickey" testMode></uc-config> |
Contributor
There was a problem hiding this comment.
Make the fixture render SmartBtn deterministically.
Both e2e failures come from this setup: CI renders uc-simple-btn, so the first getByTestId('uc-smart-btn') lookup never succeeds. dynamic alone is not making the smart button active here. Set a fixed smart-button mode in the test config before asserting on uc-smart-btn so the fixture stops depending on runtime heuristics.
Suggested change
<uc-file-uploader-regular dynamic ctx-name={ctxName}></uc-file-uploader-regular>
- <uc-config qualityInsights={false} ctx-name={ctxName} pubkey="demopublickey" testMode></uc-config>
+ <uc-config
+ qualityInsights={false}
+ smartButtonViewMode="nowrap"
+ ctx-name={ctxName}
+ pubkey="demopublickey"
+ testMode
+ ></uc-config>📝 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.
Suggested change
| <uc-file-uploader-regular dynamic ctx-name={ctxName}></uc-file-uploader-regular> | |
| <uc-config qualityInsights={false} ctx-name={ctxName} pubkey="demopublickey" testMode></uc-config> | |
| <uc-file-uploader-regular dynamic ctx-name={ctxName}></uc-file-uploader-regular> | |
| <uc-config | |
| qualityInsights={false} | |
| smartButtonViewMode="nowrap" | |
| ctx-name={ctxName} | |
| pubkey="demopublickey" | |
| testMode | |
| ></uc-config> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/smart-btn-upload-list.e2e.test.tsx` around lines 14 - 15, The fixture
is non-deterministic because the smart button depends on runtime heuristics;
update the test render to force smart-button mode by adding an explicit
smart-button mode prop to the uc-config element (so the smart button is always
enabled when the test queries getByTestId('uc-smart-btn')). Locate the uc-config
usage in the test (the element with qualityInsights={false} ctx-name={ctxName}
pubkey="demopublickey" testMode) and add the deterministic prop (e.g.,
smartButtonMode="smart" or smartButton="true" depending on the component API)
before asserting on uc-smart-btn.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Checklist
Summary by CodeRabbit
New Features
Improvements