fix: show contextual artifact unavailability message in preview dialog#2181
fix: show contextual artifact unavailability message in preview dialog#2181morgan-wowk wants to merge 1 commit intomasterfrom
Conversation
🎩 PreviewA preview build has been created at: |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
morgan-wowk
force-pushed
the
artifact-retention-msgs
branch
9 times, most recently
from
d6fbf9f to
5fe2f3f
Compare
| <ArtifactRetentionNotice | ||
| title="Failed to load artifact" | ||
| variant="error" | ||
| preamble={`An unexpected error occurred${statusDetail}.`} | ||
| specific | ||
| /> |
There was a problem hiding this comment.
Semantics here are a little confusing: given you're using this new component for both a Retention Notice and general errors you may want to consider a bit of renaming and/or splitting responsibilities between multiple components.
There was a problem hiding this comment.
It is split into multiple components per your suggestion.
morgan-wowk
force-pushed
the
artifact-retention-msgs
branch
from
5fe2f3f to
8c77875
Compare
| {isOlderThanRetentionPeriod ? ( | ||
| <ArtifactRetentionNotice title="Artifact Storage" /> | ||
| ) : expiryDate ? ( | ||
| <InfoBox title="Artifact Storage" variant="info" width="full"> | ||
| Artifacts from this run expire on {expiryDate}. | ||
| </InfoBox> | ||
| )} | ||
| ) : null} |
There was a problem hiding this comment.
I don't love the nested ternary here, especially with it fallingback to null. My gut tells me there's a better strategy to how the logic is implemented here
| const expiryDate = | ||
| retentionDays !== null && | ||
| metadata?.created_at && | ||
| !isOlderThanRetentionPeriod |
There was a problem hiding this comment.
in the ternary below, expiryDate only applies when isOlderThanRetentionPeriod is false, making the check for isOlderThanRetentionPeriod here redundant
| const isOlderThanRetentionPeriod = | ||
| retentionDays !== null && | ||
| metadata?.created_at && | ||
| isOlderThanDays(metadata.created_at, retentionDays); | ||
|
|
||
| const expiryDate = | ||
| retentionDays !== null && | ||
| metadata?.created_at && | ||
| !isOlderThanRetentionPeriod |
There was a problem hiding this comment.
The logic here seems confusing given retentionDays !== null and metadata?.created_at are duplicated across isOlderThanRetentionPeriod and expiryDate, yet expiryDate expects isOlderThanRetentionPeriod to be false, meaning the duplicated logic statements are potentially conflicting each other.
I strongly feel that it can be simplified or made more readable here
There was a problem hiding this comment.
:robot_face: Replaced the isOlderThanRetentionPeriod / expiryDate pair with a single storageStatus discriminated union ({ expired: true } | { expired: false; expiryDate: string } | null). The redundant retentionDays !== null && metadata?.created_at guards now happen once in a single if block, with clear branching on isOlderThanDays. The nested ternary in JSX is replaced with two flat && expressions keyed on storageStatus.expired.
--------
It's doing its best to clean things up. Another iteration has been pushed.
morgan-wowk
force-pushed
the
artifact-retention-msgs
branch
from
8c77875 to
ac789ac
Compare
2 participants
Description
Introduces a typed
ArtifactFetchErrorclass that carries HTTPstatusandstatusText, replacing genericErrorthrows in bothgetArtifactSignedUrlanduseArtifactFetch. This allows the artifact visualizer to distinguish between 404 (not found) and other failure states and render appropriate inline notices.Adds a new
ArtifactRetentionNoticecomponent that displays a configurable warning or errorInfoBoxwith a message driven by theVITE_ARTIFACT_RETENTION_DAYSenvironment variable. When the variable is set, the message includes the specific number of days; otherwise it falls back to a generic expiry message.The existing hardcoded 30-day retention warning in
IOSectionis replaced with this new component, now respectingVITE_ARTIFACT_RETENTION_DAYSinstead of a fixed threshold. The artifact visualizer'sSuspenseWrappernow uses anerrorFallbackthat renders a retention notice on 404 and an error-variant notice for all other failures.Related Issue and Pull requests
Type of Change
Checklist
Before screenshots
After screenshots
Test Instructions
ArtifactRetentionNoticewarning appears in the IO section.VITE_ARTIFACT_RETENTION_DAYSto a specific value and verify the expiry message includes that number of days.VITE_ARTIFACT_RETENTION_DAYSand verify the generic expiry message is shown instead.Additional Comments
The
Object.setPrototypeOfcall inArtifactFetchErrorensuresinstanceofchecks work correctly when targeting ES5 in TypeScript.