Skip to content

Conversation

NicoHinderling
Copy link
Contributor

I thought I pushed these changes to #2675 on sunday but I must have only committed them to my local

@NicoHinderling NicoHinderling requested review from a team and szokeasaurusrex as code owners August 11, 2025 16:05
@NicoHinderling NicoHinderling force-pushed the one-more-tweak-mobile-app branch from 8c4cf18 to ebd41c0 Compare August 11, 2025 16:06
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit: I would rename the PR to ref(preprod): Rename variables to indicate they store URLs.

From what I can tell, this PR is a refactor, and would not actually change any behavior. The current title makes it seem otherwise.

Edit: just realized I was wrong about this

pub missing_chunks: Vec<Digest>,
pub detail: Option<String>,
pub artifact_id: Option<String>,
pub artifact_url: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wait, this produces a behavior change. We now are trying to deserialize a field named artifact_url instead of artifact_id

@szokeasaurusrex
Copy link
Member

CI is failing, likely because you have to update the mocks to return the artifact_url in addition to, or instead of the artifact_id

@szokeasaurusrex
Copy link
Member

Closed in favor of #2698

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants