Add loading, progress, and invalid states to limel-file (component and per-file)#4154
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds progress and loading state support to chip and file rendering, expands shared file metadata, and introduces updated file examples plus a chip progress example. ChangesFile loading and progress feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Example
participant LimelFile
participant LimelChipSet
participant LimelChip
participant Spinner
Example->>LimelFile: value with loading/progress
LimelFile->>Spinner: render while busy
LimelFile->>LimelChipSet: render chip with helperText
LimelChipSet->>LimelChip: forward chip progress
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4154/ |
Kiarokh
force-pushed
the
file-loading-progress
branch
from
81c1a7f to
5b73004
Compare
Kiarokh
force-pushed
the
file-loading-progress
branch
from
5b73004 to
a1965ff
Compare
Kiarokh
changed the title
Kiarokh
force-pushed
the
file-loading-progress
branch
3 times, most recently
from
809257b to
3b78e08
Compare
Kiarokh
changed the title
| import { Component, h, Host, State } from '@stencil/core'; | ||
|
|
||
| /** | ||
| * Per-file loading |
There was a problem hiding this comment.
Why is this independent of the component-level loading prop you wonder? (see the loading example)
Because this is what a future multi-file limel-file would use to reflect the status of each individual file. Today, limel-file only allows consumers to select one single file. But we know that many have requested a file-picker component that allows selection of multiple files. In such a file picker, each file can have its own various states (e.g. loading, progress, invalid, etc…)
Kiarokh
force-pushed
the
file-loading-progress
branch
5 times, most recently
from
3c7d9ec to
8746a14
Compare
| * Optional helper text to display below the component. When the component | ||
| * is `invalid`, it is rendered as an error message. |
There was a problem hiding this comment.
This description with the invalid behavior makes a general-purpose prop sound error-specific.
| * Optional helper text to display below the component. When the component | |
| * is `invalid`, it is rendered as an error message. | |
| * Optional helper text to display below the component. |
| } | ||
|
|
||
| private handleChange = (event: CustomEvent<FileInfo>) => { | ||
| this.value = event.detail; |
There was a problem hiding this comment.
Maybe is it good to set this.loading = false if the attached file is removed? It doesn't make sense that it shows loading icons while the file is already removed. I know that this example is only to show the loading example but might be good to have a realistic example
There was a problem hiding this comment.
I think the component might be loading due to some other reason. Maybe it doesn't have the file, but some background process is making it ready for the upload, or whatever... It's basically trying to get ready to go to the file upload party
| value={value} | ||
| /> | ||
| <limel-example-controls> | ||
| <limel-example-controls |
There was a problem hiding this comment.
This change should not supposed to be in this commit
There was a problem hiding this comment.
I know 😄 Just ignore it please
Kiarokh
force-pushed
the
file-loading-progress
branch
from
8746a14 to
edd41bd
Compare
Add a `helperText` prop, forwarded to the underlying chip-set, so a consumer can display guidance below the component — and, when the component is `invalid`, an error message — like the other input-type components. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a `loading` prop that puts the whole component in an indeterminate busy state — rendering a spinner and setting `aria-busy` — without disabling its interactivity. It is the component's own loading state, controlled entirely by the consumer: the spinner shows whenever `loading` is `true`, whether or not a file is selected, and stays until the consumer sets it back to `false`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add `loading` to `FileInfo`, rendered as an indeterminate indicator on that file's chip. Because a busy file means the component has work in progress, a loading file also puts the parent component into its loading state (`aria-busy` and the component spinner). This is independent of the component-level `loading` prop, and is what a future multi-file `limel-file` will use to reflect the status of each file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`limel-chip` already supports a determinate `progress` bar, but the `Chip` model consumed by `limel-chip-set` did not expose it. Add `progress` to the `Chip` type and forward it to the underlying chip, so consumers can render determinate progress on a chip in the set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Kiarokh
force-pushed
the
file-loading-progress
branch
from
8953176 to
034257d
Compare
Consolidated PR Review — updatedPR SummaryAdds loading / progress / invalid states to Merge Readiness — READY TO MERGE ✅Strictly additive and better than
Dimensions1. Backward Compatibility — SAFE ✅Purely additive; nothing existing changes. What works well: every new surface ( Issues: None. 2. Code Quality — GOOD ✅Consistent with siblings; docs match behavior. What works well: prop declarations, the Issues: None. Minor nits (1)
3. Architecture — GOOD ✅Coherent two-tier model, now explicitly motivated. What works well: the data flow Issues: None. 4. Security — SAFE ✅No new injection surface. What works well: Issues: None. Minor nits (1)
5. Observability — GOOD ✅Real busy signal, now accurate for uploads too. What works well: Issues:
Minor nits (2)
6. Performance — SAFE ✅No new hot paths or leaks. What works well: the spinner renders only while busy and unmounts declaratively (no timers/listeners); its animation is pure CSS on the compositor; progress updates change a single CSS custom property, not DOM structure; the new chip fields are trivial pass-throughs reusing the chip's existing progress rendering. Issues: None. Minor nits (2)
Top Recommendations
|
Add `progress` to `FileInfo`, rendered as a determinate progress bar on that file's chip, reflecting the percentage of an ongoing process such as an upload or a client-side resize. Set per file so each file owns its own progress, ready for future multi-file support. While per-file `loading` covers indeterminate work, `progress` covers measurable work; the two are independent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add `invalid` to `FileInfo`, forwarded to the file's chip, so an individual file can be marked as invalid — for example one that failed to upload or is of a disallowed type. This is independent of the component-level `invalid` prop (which marks the whole field), matching how per-chip `invalid` works in `limel-chip-set`, and readies the component for per-file validation once it supports multiple files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| * by the consumer clearing these — not by `progress` reaching `100`, since | ||
| * a file at `100%` may still be finalizing (e.g. awaiting the server). | ||
| */ | ||
| private get isBusy(): boolean { |
There was a problem hiding this comment.
why this is called isBusy and not isLoading?
Because I got a feedback,
See the comment
src/components/file/file.tsx:602-608 — isLoading's "busy" semantics disagree with the chip's own progress rendering at the value boundaries. isLoading treats any defined progress (including 0) as busy: this.value?.progress !== undefined. But renderProgressBar() in src/components/chip/chip.tsx:442 uses a falsy check, if (!this.progress) return;, which was previously dead code for this component since chip-set.tsx never forwarded progress — this PR activates it. Result: - progress: 0 (the exact value the shipped example file-per-file-progress.tsx starts at): the file shows a spinner + aria-busy="true", but the chip's own progress bar doesn't render at all. - progress: 1–99: both the file-level indeterminate spinner and the chip's determinate bar render simultaneously — two contradictory busy indicators for the same operation. - progress: 100: the spinner keeps spinning even though the value implies completion. This was independently flagged by all four review angles — high confidence.and the gist of it was:
Don't show the indeterminate spinner while the chip is already showing a determinate bar.
I disagreed. Because if one (or several files in future version of this component) are in progress, for the parent component it means something is happening inside me. I'm busy for whatever reason. So I'll remain busy until the internal processes of my children are done. Then I'll show that I'm done.
Sanity-checking the matter with Claude, and this is the final comment form that discussion. Posting it here, in case anyone else wonders about these decissions:
You're right, and it's a better argument than the one I was making. I'll drop the recommendation.
The flaw in "don't show the spinner while a bar is showing" is that it treats the spinner and the bar as two answers to the same question. They're not — they're at different scopes:
- Component spinner answers "does this component have unsettled work?" — a boolean about the container.
- Chip bar answers "how far along is this file?" — a detail about one child.
The spinner isn't claiming the file's duration is unknown; it's claiming the component isn't settled. Both can be true at once without contradiction. So your model — "I'm busy for whatever reason, and I stay busy until my children's work is done, then I show done" — is coherent, and the three "boundaries" the reviewer flagged all resolve cleanly under it:
- 0 → component busy (spinner), child has nothing measurable to draw yet. Fine.
- 1–99 → component busy (spinner) + child detail (bar). Complementary, not contradictory.
- 100 → 100% is bytes transferred, not operation complete. With the upload flow you're still waiting on the server's 201. The component is legitimately still finalizing, so it stays busy until the consumer clears the state. The spinner only "spins forever" if a consumer forgets to clear it — which is a consumer bug, not ours.
The clincher is exactly the multi-file future you raised: with N files at 10/40/90/100%, the parent can't show a determinate bar — no single percentage is correct. An indeterminate "I'm busy" is the only right parent-level visual, with each chip carrying its own bar. So keeping the spinner during progress isn't just acceptable, it's the design that scales. My suggestion would've left multi-file with no parent busy affordance at all — a step backward.
This also lines up with what I already posted: the review's Observability section calls the "busy signal driven by loading and progress" a positive. I stand by that over this new finding. The one real kernel — isLoading uses progress !== undefined (so 0 counts) while the chip uses !progress (so 0 doesn't draw) — isn't an inconsistency to fix; those are two correct answers to two different questions ("is work attached?" vs. "is there a percentage to paint?").
So: no code change. The finding is a mismatch between the reviewer's mental model and your (deliberate, forward-compatible) one.
If you want to preempt the next person raising it, the only cheap things worth doing — all optional:
A one-line comment on the getter documenting the aggregate-busy decision ("stays busy while a file is loading or has progress, incl. 0; completion is consumer-signalled, not progress === 100").
Optionally rename isLoading → isBusy to match "busy for whatever reason" and stop the name implying "indeterminate only."
Kiarokh
force-pushed
the
file-loading-progress
branch
from
78fc20a to
f45838f
Compare
|
🎉 This PR is included in version 39.39.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
Why
limel-filehad no way to reflect that something is happening to the selected file — a known gap. Its consumerlimec-file-pickercan today only grey the field out viadisabledduring work, and there is no way to surface upload progress at all.Important
Before starting the review, please note that some of the decisions in this PR may feel initially strange. This is due to two reasons:
limel-fileallows only selection of one single file. We know consumers have requested multiple file selection possibility. We want to enable this feature in future. To keep this component future-proof this means, there shouldn't be a one-on-one relationship between the file(chip) and the file picker. Each file can individually becomeinvalidor be onloadingstate.FileInfointerface, should have astatusprop or similar (name not decided yet). That would allow each file to reflect its own status. For example: "Uploading…", "Failed!", "43%" (progressed, loaded, or uploaded), etc… Right now in this PR, we have some accessibility issues with aria-live, and we can't properly explain to the assistive tech what is going on. But a future per-filestatusin the follow up PR will help solve this issue. Such statuses will also be reflected on the chip's badge itself, helping sighted users to understand the status of what is happening betterTwo tiers of "busy" state, built up in atomic commits:
feat(file): add a loading prop— a component-levelloadingprop: the component's own indeterminate busy state (spinner +aria-busy), controlled entirely by the consumer, shown whether or not a file is selected. Does not disable interactivity.feat(file): support a per-file loading state—FileInfo.loading, rendered as an indeterminate indicator on that file's chip. A loading file also puts the parent into its loading state. Independent of the component prop; the multi-file-ready per-file model.feat(chip-set): forward the progress property to the chip—limel-chipalready had a determinateprogressbar, but theChipmodel consumed bylimel-chip-setdidn't expose it. AddsprogresstoChip+ forwards it (with a chip-set example).feat(file): support a per-file progress state—FileInfo.progress(0–100), a determinate bar on the file's chip, for measurable work like uploads or client-side resizing.feat(file): support a per-file invalid state—FileInfo.invalid, an error state on the file's chip, independent of the component-levelinvalidprop.Per-file status lives on
FileInfo, so it maps 1:1 onto the chip and is ready for a future multi-filelimel-file— each file owns its own state, the pattern already used bylimel-chip-set'sChip[]andlimebb-document-chips. Every field is optional and defaults to off, so this is fully backward compatible. Each feat carries its own documentation example.Design notes
loadingexists both at the component level (the whole picker is busy — a spinner) and per file (FileInfo.loading— the chip's own indicator). A loading file bubbles up to the component's busy state (aria-busy+ spinner); the two are otherwise independent.progressand per-fileinvalidare per-file only. There is intentionally no component-levelprogress— a cumulative/overall bar can be added later if/when the component supports multiple files.loadingdoes not disable interactivity, matching other components' loading states.aria-busy), not what — announcing the specific activity ("Uploading…") is a natural fit for a future per-file status label.🤖 Generated with Claude Code
Summary by CodeRabbit
loadingsupport to the file component, including busy behavior with a spinner.invalidandhelperTexthandling to improve messaging in chip UI.aria-busy) and adjusted UI while busy.