Skip to content

Add default vcs head_repo_name and provider parsing for mobile-app subcommand #2699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 12, 2025
Merged

Add default vcs head_repo_name and provider parsing for mobile-app subcommand #2699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ab1842c
Add default vcs head_repo_name and provider parsing for mobile-app su…
rbro112 Aug 11, 2025
9504dc3
Tweaks
rbro112 Aug 11, 2025
ecb6f02
Fixes
rbro112 Aug 11, 2025
87fc789
FF
rbro112 Aug 11, 2025
5c19bf7
Remove matchers
rbro112 Aug 11, 2025
d320c06
Cleanup based on feedback
rbro112 Aug 12, 2025
5d52df7
fmt
rbro112 Aug 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 40 additions & 6 deletions src/commands/mobile_app/upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ use crate::utils::mobile_app::{
};
use crate::utils::mobile_app::{is_aab_file, is_apk_file, is_zip_file, normalize_directory};
use crate::utils::progress::ProgressBar;
use crate::utils::vcs;
use crate::utils::vcs::{
self, get_provider_from_remote, get_repo_from_remote, git_repo_remote_url,
};

pub fn make_command(command: Command) -> Command {
#[cfg(all(target_os = "macos", target_arch = "aarch64"))]
Expand Down Expand Up @@ -92,6 +94,7 @@ pub fn make_command(command: Command) -> Command {
}

pub fn execute(matches: &ArgMatches) -> Result<()> {
let config = Config::current();
let path_strings = matches
.get_many::<String>("paths")
.expect("paths argument is required");
Expand All @@ -102,10 +105,41 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
.map(Cow::Borrowed)
.or_else(|| vcs::find_head().ok().map(Cow::Owned));

let base_sha = matches.get_one("base_sha").map(String::as_str);
let vcs_provider = matches.get_one("vcs_provider").map(String::as_str);
let head_repo_name = matches.get_one("head_repo_name").map(String::as_str);
let cached_remote = config.get_cached_vcs_remote();
// Try to open the git repository and find the remote, but handle errors gracefully.
let (vcs_provider, head_repo_name) = {
// Try to open the repo and get the remote URL, but don't fail if not in a repo.
let repo = git2::Repository::open_from_env().ok();
let remote_url = repo.and_then(|repo| git_repo_remote_url(&repo, &cached_remote).ok());

let vcs_provider: Option<Cow<'_, str>> = matches
.get_one("vcs_provider")
.map(String::as_str)
.map(Cow::Borrowed)
.or_else(|| {
remote_url
.as_ref()
.map(|url| get_provider_from_remote(url))
.map(Cow::Owned)
});

let head_repo_name: Option<Cow<'_, str>> = matches
.get_one("head_repo_name")
.map(String::as_str)
.map(Cow::Borrowed)
.or_else(|| {
remote_url
.as_ref()
.map(|url| get_repo_from_remote(url))
.map(Cow::Owned)
});

(vcs_provider, head_repo_name)
};
Copy link

Choose a reason for hiding this comment

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

Bug: Mobile App Upload: Feature Guard and URL Parsing Errors

The mobile_app upload command introduces two issues:

  1. The get_provider_from_remote and get_repo_from_remote functions are imported and used without the #[cfg(feature = "unstable-mobile-app")] guard, despite their definitions being guarded. This causes compilation errors when the feature is disabled.
  2. These functions can return empty strings for unparseable remote URLs. These empty strings are then incorrectly passed as Some("") to the API instead of None, leading to potentially incorrect VCS information being sent.
Fix in Cursor Fix in Web


let base_repo_name = matches.get_one("base_repo_name").map(String::as_str);

let base_sha = matches.get_one("base_sha").map(String::as_str);
let head_ref = matches.get_one("head_ref").map(String::as_str);
let base_ref = matches.get_one("base_ref").map(String::as_str);
let pr_number = matches.get_one::<u32>("pr_number");
Expand Down Expand Up @@ -170,8 +204,8 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
let vcs_info = VcsInfo {
head_sha: head_sha.as_deref(),
base_sha,
vcs_provider,
head_repo_name,
vcs_provider: vcs_provider.as_deref(),
head_repo_name: head_repo_name.as_deref(),
base_repo_name,
head_ref,
base_ref,
Expand Down
18 changes: 18 additions & 0 deletions src/utils/vcs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,24 @@ pub fn get_repo_from_remote(repo: &str) -> String {
obj.id
}

#[cfg(feature = "unstable-mobile-app")]
pub fn get_provider_from_remote(remote: &str) -> String {
let obj = VcsUrl::parse(remote);
obj.provider
}

#[cfg(feature = "unstable-mobile-app")]
pub fn git_repo_remote_url(
repo: &git2::Repository,
cached_remote: &str,
) -> Result<String, git2::Error> {
let remote = repo.find_remote(cached_remote)?;
remote
.url()
.map(|url| url.to_owned())
.ok_or_else(|| git2::Error::from_str("No remote URL found"))
}

fn find_reference_url(repo: &str, repos: &[Repo]) -> Result<Option<String>> {
let mut non_git = false;
for configured_repo in repos {
Expand Down
2 changes: 0 additions & 2 deletions tests/integration/mobile_app/upload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ fn command_mobile_app_upload_apk_chunked() {
"/api/0/projects/wat-org/wat-project/files/preprodartifacts/assemble/",
)
.with_header_matcher("content-type", "application/json")
.with_matcher(r#"{"checksum":"18e40e6e932d0b622d631e887be454cc2003dbb5","chunks":["18e40e6e932d0b622d631e887be454cc2003dbb5"],"head_sha":"test_head_sha"}"#)
Copy link
Member Author

Choose a reason for hiding this comment

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

With additional params, we don't want a 1:1 body matcher anymore, so removing should make this not fail when making default value changes.

.with_response_fn(move |_| {
if is_first_assemble_call.swap(false, Ordering::Relaxed) {
r#"{
Expand Down Expand Up @@ -214,7 +213,6 @@ fn command_mobile_app_upload_ipa_chunked() {
"/api/0/projects/wat-org/wat-project/files/preprodartifacts/assemble/",
)
.with_header_matcher("content-type", "application/json")
.with_matcher(r#"{"checksum":"ed9da71e3688261875db21b266da84ffe004a8a4","chunks":["ed9da71e3688261875db21b266da84ffe004a8a4"],"head_sha":"test_head_sha"}"#)
.with_response_fn(move |_| {
if is_first_assemble_call.swap(false, Ordering::Relaxed) {
r#"{
Expand Down