Studio: fix download card alignment and surface the stop button - #7848
Conversation
In a downloading card the progress bar sat 12px left of the quant label and ran past the percentage on the right, because the row's trigger and action button both inset their text and the bar did not. It now shares those bounds, so the bar, the byte counts and the row read as one block. The quant label group also stretched to fill the trigger, which pushed the chevron to the far edge of a wide card and left a gap the width of the row between the tags and the affordance they belong to. It sizes to its content now, and still shrinks when the row is tight.
Stopping a download meant hovering the percentage, where a spinner cross-faded
into a cancel glyph. Nothing on screen said the number was a button, so the
only way to stop a download was to guess. The glyph is always visible now.
It also says what stopping costs. An HTTP download leaves a partial to
continue from, so it shows a pause; Xet has to start over, so it shows a
cancel. That matches the resume labels these cards already use ("Continue"
for HTTP, "Redownload" for Xet). An unknown transport shows the cancel,
rather than promising a resume that may not exist.
fd4cd2c to
c9d479b
Compare
shimmyshimmer
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9d479b816
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| starting, | ||
| variant, | ||
| ]); | ||
| const stopMode = downloadStopMode(partialTransport); |
There was a problem hiding this comment.
When an HTTP download starts without an existing partial, partialTransport is null, so this reports cancel even though stopping preserves resumable data; it can also remain the old transport when a conflict is restarted using the other transport. Because this value describes the pre-download partial rather than the running job, the new icon and aria-label misrepresent common fresh and transport-switch downloads. Propagate the active transport from the download manager instead.
Useful? React with 👍 / 👎.
The review is right that `partialTransport` was the wrong source. It describes a partial that existed before the download started, so a fresh HTTP download had none and showed a cancel even though stopping keeps resumable data, and a conflict restarted on the other transport kept reporting the old one. Both are ordinary cases, so the icon was usually wrong for HTTP. The transport a run resolves to is now recorded on the job and read from there, falling back to the partial only for an adopted job that never resolved one here. Unknown still shows a cancel rather than promising a resume.
|
This one is right, and it undoes the reasoning I used when I wrote it.
The transport a run resolves to is now recorded on the job at start, where Seven tests cover the mapping, including the two cases that were wrong: a fresh HTTP download with no partial, and a restarted conflict that switched transport. Frontend 351/351, typecheck and build clean. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5dfee883b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...(opts.adopt | ||
| ? existing?.transport | ||
| ? { transport: existing.transport } | ||
| : {} |
There was a problem hiding this comment.
When an HTTP download survives a page reload, toPersistedJob and sanitizePersistedJob discard the newly added transport, so this adopted branch leaves it unset; hydration also ignores the transport already returned by ActiveModelDownload and ActiveDatasetDownload. downloadStopMode consequently falls back to stale or unavailable inventory data—and the GGUF loading fallback has no partial-transport fallback at all—so a resumable HTTP transfer is presented as Cancel rather than Pause. Fresh evidence beyond the earlier comment is that the new active-transport implementation explicitly loses the value along this reload/adoption path.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 406b2d0f37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const activeTransport = opts.adopt | ||
| ? (opts.transport ?? existing?.transport) | ||
| : mode; |
There was a problem hiding this comment.
When another client has already started the same download key before this client's POST reaches the backend, the backend claim rejects the new transport but the model/dataset start handlers still return accepted: true so this client can poll the existing job. This non-opts.adopt path nevertheless records the locally requested mode, so an existing Xet job requested locally as HTTP is shown and persisted as pausable, and the inverse is shown as cancel-only. Fresh evidence beyond the earlier adoption comment is that accepted-start adoption is not marked as opts.adopt; fetch the live job's transport when the response adopts an existing run instead of assuming the requested mode became active.
Useful? React with 👍 / 👎.
Right, and it is the one adoption path the previous commit missed. When another client has already started the same key, `claim` refuses the new transport but `adoptable` still returns accepted, so this client polls the existing run. It then recorded the transport it asked for, not the one that run is on, and the button offered Pause for a Xet job or Cancel for a resumable HTTP one. The start response now carries the live job's transport, taken from the same registry metadata `active-downloads` already publishes, and it is only set on that adopting branch. A start of its own reports nothing and keeps what it resolved. Backend 3 tests over the registry, frontend 3 over the mapping, both covering the two swapped cases.
|
Right, and it is the one adoption path the previous commit missed. When another client has already started the same key, The start response now carries the live job's transport, read from the same registry metadata Testing: three backend tests over the registry (adopted, unknown, and a job claimed without metadata) and three frontend tests over the mapping, covering both swapped cases. Frontend 356/356, typecheck and build clean. The four |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08cec392e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (runtimeRegistry.runtimes.get(key)?.pollingStarted) { | ||
| // Persistence hydration and the backend-active probe run concurrently. A | ||
| // late backend response must still replace a missing or stale stored value. | ||
| if (transport) patchJob(key, { transport }); |
There was a problem hiding this comment.
When a backend-active/hydration probe returns after the user has cancelled and restarted the same download key, this branch patches the new runtime with the old response's transport without checking the generation argument against the current job. In that race, an old HTTP adoption response can overwrite a freshly restarted Xet job (or vice versa), and the later fresh-start response may not repair it because startJob compares against its precomputed local activeTransport, leaving the stop button advertising Pause/Cancel for the wrong transport.
Useful? React with 👍 / 👎.
| const activeTransport = opts.adopt | ||
| ? (opts.transport ?? existing?.transport) | ||
| : mode; |
There was a problem hiding this comment.
When the capabilities request cannot verify Xet support, effectiveTransportMode can still let an Xet start go through, but the backend's resolve_requested_use_xet('xet') may downgrade that fresh start to HTTP when hf_xet is unavailable. Because successful fresh-start responses do not report the resolved transport, storing mode here leaves the job persisted as Xet and the stop button shows Cancel even though the worker is actually on resumable HTTP.
Useful? React with 👍 / 👎.
| starting, | ||
| variant, | ||
| ]); | ||
| const stopMode = downloadStopMode(job.transport, partialTransport); |
There was a problem hiding this comment.
For dataset cards that auto-adopt a download started elsewhere during the same session, the adoption path still calls getDatasetDownloadStatus, whose response has no transport, so the adopted job reaches this new stop-mode calculation with job.transport unset. A fresh HTTP dataset download with no existing partial therefore falls through to cancel until a full hydration path happens to patch it, making the visible glyph and aria-label disagree with the resumable HTTP behavior.
Useful? React with 👍 / 👎.
Three findings, all confirmed, all the same underlying gap: the client decided the transport locally and the backend never contradicted it. A fresh start can be downgraded. `resolve_requested_use_xet` honours an explicit "xet" only when hf_xet is present; `resolve_effective_use_xet` falls back to HTTP otherwise. The success response said nothing, so the job stayed recorded as Xet and offered Cancel for a transfer that would have resumed byte-exact. Both start handlers now report the resolved transport, which the client already reads through `transportAfterStart`. Dataset auto-adoption went through `download-status`, which does not carry a transport, while the model path used `active-downloads`, which does. Datasets use the same list now, narrowed by repo id. A late probe reply could also land on the wrong run. `adoptJob` patched an already-polling job without checking the generation it described, so a response from before a cancel and restart overwrote the new job's transport, and the fresh start's own reply could not repair it because it compares against a value it computed before the request. Patching is gated on the generation, and a job with none recorded yet takes the probe's and records it. Backend 5 tests, frontend 4 more over the gate. Frontend 360/360, typecheck, locale parity and build clean.
|
All three right, and they share one root: the client decided the transport locally and the backend never contradicted it. Keep the backend fallback transport for fresh starts. Preserve transport for dataset auto-adoption. The model path probes Gate late transport patches by generation. Backend 5 tests, frontend 4 more over the gate. The four |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36962e0d5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (signal.aborted) return; | ||
| if (status.state === "running" || status.state === "cancelling") { | ||
| for (const active of datasets) { | ||
| if (active.repo_id !== repoId) continue; |
There was a problem hiding this comment.
When the dataset card probes a repo whose displayed casing differs from the backend's canonical/cache casing, the /datasets/active-downloads endpoint has already canonicalized and filtered the requested repo_id, but this exact comparison drops the returned active row. In that case another client's running dataset download is not adopted, so this card misses its progress/transport and stop control until a full hydration path runs; compare normalized repo ids or remove the redundant filter.
Useful? React with 👍 / 👎.
| const transport = activeTransport ?? partialTransport; | ||
| return transport === "http" ? "pause" : "cancel"; |
There was a problem hiding this comment.
When a Xet worker falls back to HTTP, the backend reclaims the job as HTTP but keeps cancel_marker_transport as the original Xet transport (studio/backend/hub/services/download_lifecycle.py:506-517, :899-904), so cancelling that retry still creates the Redownload/non-resumable partial state. Because this chooses Pause solely from activeTransport === "http", adopted or reloaded clients that learn the retry's active HTTP transport will advertise Pause even though stopping produces a Redownload state; the stop mode needs the backend's cancel-marker/stop semantics, not only the live transport.
Useful? React with 👍 / 👎.
…transport Two findings, both confirmed. A Xet worker that falls back to HTTP has the job reclaimed as HTTP but keeps `cancel_marker_transport` set to Xet, and terminating swaps the recorded transport back to that marker. So the partial such a run leaves behind is restart-only even though the worker is on resumable HTTP, and a client that adopted the run and learned "http" offered Pause for something that would in fact have to start over. The marker is published on active downloads now, and the stop mode prefers it. It is set only on a fallback run, so nothing else changes. The dataset probe also compared repo ids exactly, which the previous commit added. `/datasets/active-downloads` resolves the cached casing before it filters, so a card whose spelling differs from the cache's dropped the very row it had just asked for and never adopted the running download. The compare is gone; the endpoint's own filter is the one that counts. Backend 7 tests, frontend 3 more over the marker. 363/363 frontend, typecheck, locale parity and build clean. The four `test_cached_models_scan_*` failures are on main as well.
for more information, see https://pre-commit.ci
|
Both right. Preserve retry cancel semantics. Confirmed in The marker is published on active downloads now as Normalize dataset ids before dropping active rows. Right, and the redundant compare was added by the previous commit. Backend 7 tests, frontend 3 more over the marker, including the fallback case and an ordinary HTTP job that still pauses. Frontend 363/363, typecheck, locale parity and build clean. The four |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc863be3b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # The transport the job is really on: the one the backend resolved for a | ||
| # fresh start, or the running job's own when this start adopted one. | ||
| # Either can differ from what the client asked for. | ||
| transport: Optional[str] = None |
There was a problem hiding this comment.
When another client starts the same key while the live job has already fallen back from Xet to HTTP, this start response is the only data source for the non-opts.adopt adoption path. Fresh evidence beyond the earlier retry-semantics concern is that active downloads now expose cancel_transport, but the start response only exposes transport, so the frontend records http with no cancelTransport and offers Pause even though cancelling still writes the Xet marker and leaves a Redownload-only partial; include the marker in both model and dataset accepted-start responses.
Useful? React with 👍 / 👎.
| }, | ||
| safeGeneration(download.generation), | ||
| download.state, | ||
| isResolvedTransport(download.transport) ? download.transport : undefined, |
There was a problem hiding this comment.
On a page reload during an Xet-to-HTTP fallback run, /active-downloads returns both transport: "http" and cancel_transport: "xet", but hydration passes only the live transport into adoptJob here (and does the same for datasets below). Fresh evidence is that this newly marker-aware adoption API now receives the field from the backend type but drops it, so the reloaded card loses job.cancelTransport and advertises Pause for a stop that produces a restart-only partial.
Useful? React with 👍 / 👎.
| downloading | ||
| cancelling={cancelling} | ||
| progressPercent={Math.round(Math.min(progress.fraction, 1) * 100)} | ||
| stopMode={downloadStopMode(job.transport)} |
There was a problem hiding this comment.
When the GGUF variant list cannot load while a fallback run is active, this fallback card bypasses useDownloadCardState and derives the stop mode from job.transport alone. For an adopted Xet-to-HTTP retry that has job.transport === "http" and job.cancelTransport === "xet", this renders the Pause glyph/aria-label even though stopping leaves a Redownload-only partial; pass job.cancelTransport into downloadStopMode here too.
Useful? React with 👍 / 👎.
…sport Three findings, all correct, and all the same omission: the previous commit published the marker on active downloads and then only read it in one place. An accepted start that adopts a running job is the sole data source for that adoption path, so it reports the marker alongside the live transport now. A fresh start still reports neither, since a fallback happens long after one. Reload hydration passed only the live transport into `adoptJob`, for models and datasets both, so a reloaded card lost the marker and advertised Pause for a stop that leaves a restart-only partial. The GGUF fallback card bypasses `useDownloadCardState` and built its stop mode straight from `job.transport`, so it had the same wrong glyph and label. It reads the marker too. Backend 9 tests, frontend 363/363, typecheck, locale parity and build clean.
|
All three right, and all the same omission: the previous commit published the cancel marker on active downloads and then only read it in one place. Include cancel markers in adopted start responses. That response is the only data source for the non- Pass cancel markers through reload adoption. Hydration read Keep the GGUF fallback card marker-aware. It bypasses Backend 9 tests over the registry, frontend 363/363, typecheck, locale parity and build clean. The four |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fa9f75192
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? { serverGeneration: job.serverGeneration } | ||
| : {}), | ||
| ...(job.scopedFiles !== undefined ? { scopedFiles: job.scopedFiles } : {}), | ||
| ...(job.transport !== undefined ? { transport: job.transport } : {}), |
There was a problem hiding this comment.
When the page is reloaded during an Xet→HTTP fallback run, the in-memory job has cancelTransport: "xet" so downloadStopMode correctly shows Cancel, but the persisted job written here keeps only transport. Fresh evidence is that the new persisted shape includes the active transport but omits the sibling cancel marker; after localStorage hydration (and until or unless backend active adoption patches it), the restored job becomes plain HTTP and the stop button advertises Pause even though stopping still leaves a restart-only partial. Persist and sanitize cancelTransport alongside transport.
Useful? React with 👍 / 👎.
Right, and the last omission of the same kind. `toPersistedJob` wrote `transport` and `sanitizePersistedJob` restored it, but neither touched `cancelTransport`, so a page reload during a Xet-to-HTTP fallback run brought the job back as plain HTTP. Until backend adoption happened to patch it, the card offered Pause for a stop that still leaves a restart-only partial. Both directions handle it now, and the restore drops an unresolved value rather than trusting whatever was in storage, exactly as it does for the transport. Frontend 366/366, typecheck, locale parity and build clean.
|
Right, and the last place the marker was being dropped. Both directions handle it now, and the restore drops an unresolved value rather than trusting whatever was in storage, the same way it already treats the transport. Two hydration cases and one write case are covered, including a stored marker of "auto" being discarded. Frontend 366/366, typecheck, locale parity and build clean. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55e8841ccb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...(opts.adopt && opts.cancelTransport | ||
| ? { cancelTransport: opts.cancelTransport } | ||
| : {}), |
There was a problem hiding this comment.
When a page reload restores an Xet→HTTP fallback job, the status probe can win the hydration race (or /active-downloads can fail) and call adoptJob(req, generation, state) without a cancelTransport, because /download-status does not expose that field. Fresh evidence is that this adopt path rebuilds the job while only copying opts.cancelTransport, so it erases the already-sanitized existing.cancelTransport; the card then treats the active HTTP retry as pausable even though stopping still writes the Xet marker and leaves a restart-only partial. Preserve existing?.cancelTransport when opts.adopt has no marker.
Useful? React with 👍 / 👎.
Right. The transport already fell back to the persisted value on adopt, but the cancel marker only took what the probe reported, and `putJob` replaces the record. `/download-status` carries no marker and can win the hydration race against `/active-downloads`, which does, so a reloaded fallback run lost the marker that had just been restored from storage and offered Pause again. Both fields go through one helper now rather than two lines that can drift, so a probe carrying only one of them cannot erase the other. Frontend 370/370, typecheck, locale parity and build clean.
|
Correct, fixed in 2579c4e.
Both fields now go through one helper, |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2579c4e913
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| isResolvedTransport(download.cancel_transport) | ||
| ? download.cancel_transport | ||
| : undefined, |
There was a problem hiding this comment.
When a reload restores an old Xet→HTTP fallback job from localStorage, existing.cancelTransport is preserved by adoptJob; if the backend now reports the same key as an ordinary HTTP active download with cancel_transport: null (for example after another client started a fresh generation), this conversion turns that explicit “no marker” into undefined, so the old Xet marker is kept and the card continues to show Cancel instead of Pause for a resumable HTTP run. Preserve the distinction between an omitted field and a reported null marker, or drop the persisted marker when the reported generation/run is different and markerless.
Useful? React with 👍 / 👎.
adoptedTransports() kept a persisted marker whenever the probe reported none, which is right for /download-status (it cannot report one) but wrong for /active-downloads, which always carries the field. A reload that restored an old Xet-to-HTTP job kept its marker even when the backend described a fresh markerless HTTP run, so the card showed Cancel for a transfer that would have resumed. The marker is tri-state through adoption now: undefined means the source has nothing to say and the stored value stands, null is the backend saying this run has none.
|
Right, and it is the mirror image of the case fixed just before it, so worth being exact about which source is which.
The marker is tri-state through adoption now: Fixed in 9726ce9. Frontend 372/372 with two new cases, plus typecheck, locale parity and build. |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…alignment # Conflicts: # studio/frontend/src/features/hub/download-manager/download-manager-types.ts
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Two problems with a downloading card, one layout and one that made a control unfindable.
The progress bar did not line up with its row
.hub-download-cardpads its children, then the row's popover trigger and action button each inset their own text by another 12px. The bar was a direct child of the card, so it started 12px left of the quant label and ran past the percentage on the right. It now sits in the same inset, so the bar, the byte counts and the row read as one block.The quant label group also carried
flex-1inside the trigger. On a wide card that stretched it across the whole row and stranded the chevron at the far edge, a gap the width of the card away from the tags it belongs to. Dropping the grow makes the group size to its content, so the chevron follows the tags. It keepsmin-w-0and its default shrink, so a long quant label still truncates when the row is tight, and the trigger keeps its full-width hover and click target.Stopping a download required guessing
The stop control was the percentage itself: hovering it cross-faded a spinner into a cancel glyph. Nothing on screen suggested the number was a button, so a user who wanted to stop a download had no visible way to do it. The glyph now sits beside the percentage at all times.
It also reflects what stopping costs, which is not the same on both transports:
That mapping reuses
partialResumeLabel, so the button and the label a stopped download shows cannot disagree.Testing
Frontend typecheck, tests, locale parity and build all pass. The dead hover-swap rules in
hub.cssare removed with the behaviour they drove. Both changes apply to every card on the shared shell (safetensors, GGUF and dataset).