feat: return installed app metadata from apps install#274
Open
gmegidish wants to merge 1 commit into
Open
Conversation
WalkthroughThis PR adds cross-platform app metadata extraction by introducing a new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
utils/appmeta_test.go (1)
77-89: 💤 Low valueConsider verifying all metadata fields for consistency.
The test correctly validates that nested framework plists don't override the app plist. For consistency with other tests, consider also asserting
VersionandVersionCodefields.✨ Optional enhancement
meta, err := ParseAppMetadata(ipaPath) require.NoError(t, err) assert.Equal(t, "com.mobilenext.playground", meta.PackageName) + assert.Equal(t, "1.4.0", meta.Version) + assert.Equal(t, "42", meta.VersionCode) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/appmeta_test.go` around lines 77 - 89, Update the test TestParseAppMetadataIgnoresNestedBundlePlists to assert the parsed Version and VersionCode as well as PackageName: after calling ParseAppMetadata(ipaPath) and obtaining meta, add assertions that meta.Version equals the expected version string from sampleInfoPlist and that meta.VersionCode (or meta.VersionCodeInt if that’s the actual field name) equals the expected numeric build/version code; use require.NoError and assert.Equal like the existing PackageName check so the test verifies all primary metadata fields are consistent with sampleInfoPlist.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@commands/apps.go`:
- Line 167: The code calls utils.ParseAppMetadata(req.Path) even when a resigned
IPA was installed; update the call to parse metadata from the actual installed
file by using installPath when req.ForceResign is true (fall back to req.Path
otherwise) — i.e., replace utils.ParseAppMetadata(req.Path) with
utils.ParseAppMetadata(installPathOrOriginal) where installPathOrOriginal =
installPath if req.ForceResign else req.Path (resignedPath is still available
due to the deferred removal).
In `@utils/appmeta_test.go`:
- Around line 111-114: The test TestParseAppMetadataRejectsUnknownExtension uses
a Unix-only path "/tmp/whatever.txt"; change the argument to ParseAppMetadata to
a simple relative filename (e.g., "whatever.txt" or "whatever.unknown") so the
test dispatches by extension without relying on an OS-specific temp directory;
update the call in TestParseAppMetadataRejectsUnknownExtension accordingly to
use the relative path.
---
Nitpick comments:
In `@utils/appmeta_test.go`:
- Around line 77-89: Update the test
TestParseAppMetadataIgnoresNestedBundlePlists to assert the parsed Version and
VersionCode as well as PackageName: after calling ParseAppMetadata(ipaPath) and
obtaining meta, add assertions that meta.Version equals the expected version
string from sampleInfoPlist and that meta.VersionCode (or meta.VersionCodeInt if
that’s the actual field name) equals the expected numeric build/version code;
use require.NoError and assert.Equal like the existing PackageName check so the
test verifies all primary metadata fields are consistent with sampleInfoPlist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8690475-503d-4218-bd9c-8126e91932ac
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
commands/apps.gogo.modutils/appmeta.goutils/appmeta_test.goutils/testdata/helloworld.apk
Comment on lines
+111
to
+114
| func TestParseAppMetadataRejectsUnknownExtension(t *testing.T) { | ||
| _, err := ParseAppMetadata("/tmp/whatever.txt") | ||
| assert.Error(t, err) | ||
| } |
There was a problem hiding this comment.
Use a cross-platform path for the test.
The hardcoded /tmp/whatever.txt path is Unix-specific and will fail on Windows where /tmp/ doesn't exist in the same form. Since ParseAppMetadata dispatches by extension before attempting to open the file, use a simple relative path instead.
🔧 Proposed fix
func TestParseAppMetadataRejectsUnknownExtension(t *testing.T) {
- _, err := ParseAppMetadata("/tmp/whatever.txt")
+ _, err := ParseAppMetadata("whatever.txt")
assert.Error(t, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestParseAppMetadataRejectsUnknownExtension(t *testing.T) { | |
| _, err := ParseAppMetadata("/tmp/whatever.txt") | |
| assert.Error(t, err) | |
| } | |
| func TestParseAppMetadataRejectsUnknownExtension(t *testing.T) { | |
| _, err := ParseAppMetadata("whatever.txt") | |
| assert.Error(t, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utils/appmeta_test.go` around lines 111 - 114, The test
TestParseAppMetadataRejectsUnknownExtension uses a Unix-only path
"/tmp/whatever.txt"; change the argument to ParseAppMetadata to a simple
relative filename (e.g., "whatever.txt" or "whatever.unknown") so the test
dispatches by extension without relying on an OS-specific temp directory; update
the call in TestParseAppMetadataRejectsUnknownExtension accordingly to use the
relative path.
gmegidish
force-pushed
the
feat/install-app-metadata
branch
from
9216e24 to
a500d86
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utils/appmeta.go (1)
66-96: 💤 Low valueOptional: Document first-match behavior for multiple app bundles.
The function returns the first matching top-level
*.app/Info.plistfound in the archive. For standard IPA/simulator-zip layouts, this is correct since only one app bundle should exist. If you want to make this explicit, consider adding a comment.📝 Suggested documentation addition
// parseIpaMetadata reads the top-level app's Info.plist from an .ipa or .zip // archive. It works for both .ipa (Payload/Foo.app/Info.plist) and simulator -// .zip (Foo.app/Info.plist) layouts. +// .zip (Foo.app/Info.plist) layouts. If multiple top-level app bundles exist, +// the first match is returned. func parseIpaMetadata(path string) (*AppMetadata, error) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/appmeta.go` around lines 66 - 96, Add a short explanatory comment to the parseIpaMetadata function documenting that it returns the first matching top-level "*.app/Info.plist" found in the archive (first-match behavior) and that this is appropriate for standard IPA/simulator-zip layouts where only one app bundle is expected; also note that if multiple .app bundles exist the function will ignore subsequent matches (and call out that changing this behavior would require iterating all matches and merging/choosing among them). Reference function parseIpaMetadata and place the comment immediately above its declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@utils/appmeta.go`:
- Around line 44-64: parseApkMetadata currently returns success even when
pkg.PackageName() is empty, producing degenerate AppMetadata; update
parseApkMetadata to validate that pkg.PackageName() is non-empty after opening
the APK and return a descriptive error (e.g., fmt.Errorf("apk has empty package
name")) if it is empty (ensure pkg is closed via the existing defer), so
AppMetadata.PackageName is never "" on success; keep the existing population of
Version/VersionCode logic unchanged.
---
Nitpick comments:
In `@utils/appmeta.go`:
- Around line 66-96: Add a short explanatory comment to the parseIpaMetadata
function documenting that it returns the first matching top-level
"*.app/Info.plist" found in the archive (first-match behavior) and that this is
appropriate for standard IPA/simulator-zip layouts where only one app bundle is
expected; also note that if multiple .app bundles exist the function will ignore
subsequent matches (and call out that changing this behavior would require
iterating all matches and merging/choosing among them). Reference function
parseIpaMetadata and place the comment immediately above its declaration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 674d1c9b-6147-4e08-8283-f3d95d056227
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
commands/apps.gogo.modutils/appmeta.goutils/appmeta_test.goutils/testdata/sample.apk
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- commands/apps.go
- utils/appmeta_test.go
Comment on lines
+44
to
+64
| func parseApkMetadata(path string) (*AppMetadata, error) { | ||
| pkg, err := apk.OpenFile(path) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to open apk: %w", err) | ||
| } | ||
| defer func() { _ = pkg.Close() }() | ||
|
|
||
| meta := &AppMetadata{ | ||
| PackageName: pkg.PackageName(), | ||
| } | ||
|
|
||
| manifest := pkg.Manifest() | ||
| if versionName, err := manifest.VersionName.String(); err == nil { | ||
| meta.Version = versionName | ||
| } | ||
| if versionCode, err := manifest.VersionCode.Int32(); err == nil { | ||
| meta.VersionCode = strconv.FormatInt(int64(versionCode), 10) | ||
| } | ||
|
|
||
| return meta, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify androidbinary API compatibility
# Check the API exported by the library at v1.0.5
echo "=== Fetching androidbinary v1.0.5 API ==="
rg -A 3 'func.*PackageName|func.*Manifest|func.*VersionName|func.*VersionCode' \
--iglob '**/androidbinary/**/*.go' 2>/dev/null || \
echo "Note: Library not in current workspace; manual verification needed"
# Search Go package documentation
echo -e "\n=== Manual verification needed ==="
echo "Please verify the following at https://pkg.go.dev/github.com/shogo82148/androidbinary@v1.0.5/apk:"
echo " 1. apk.OpenFile(path) returns *Pkg with PackageName() method"
echo " 2. Pkg.Manifest() returns manifest with VersionName and VersionCode fields"
echo " 3. VersionName has String() method, VersionCode has Int32() method"Repository: mobile-next/mobilecli
Length of output: 517
Validate non-empty APK package name
parseApkMetadata always returns success and may set AppMetadata.PackageName to "" when pkg.PackageName() is empty; treat that as an error so callers don’t receive degenerate metadata.
🛡️ Suggested validation
meta := &AppMetadata{
PackageName: pkg.PackageName(),
}
+ if meta.PackageName == "" {
+ return nil, fmt.Errorf("APK manifest missing package name")
+ }
manifest := pkg.Manifest()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func parseApkMetadata(path string) (*AppMetadata, error) { | |
| pkg, err := apk.OpenFile(path) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to open apk: %w", err) | |
| } | |
| defer func() { _ = pkg.Close() }() | |
| meta := &AppMetadata{ | |
| PackageName: pkg.PackageName(), | |
| } | |
| manifest := pkg.Manifest() | |
| if versionName, err := manifest.VersionName.String(); err == nil { | |
| meta.Version = versionName | |
| } | |
| if versionCode, err := manifest.VersionCode.Int32(); err == nil { | |
| meta.VersionCode = strconv.FormatInt(int64(versionCode), 10) | |
| } | |
| return meta, nil | |
| } | |
| func parseApkMetadata(path string) (*AppMetadata, error) { | |
| pkg, err := apk.OpenFile(path) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to open apk: %w", err) | |
| } | |
| defer func() { _ = pkg.Close() }() | |
| meta := &AppMetadata{ | |
| PackageName: pkg.PackageName(), | |
| } | |
| if meta.PackageName == "" { | |
| return nil, fmt.Errorf("APK manifest missing package name") | |
| } | |
| manifest := pkg.Manifest() | |
| if versionName, err := manifest.VersionName.String(); err == nil { | |
| meta.Version = versionName | |
| } | |
| if versionCode, err := manifest.VersionCode.Int32(); err == nil { | |
| meta.VersionCode = strconv.FormatInt(int64(versionCode), 10) | |
| } | |
| return meta, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utils/appmeta.go` around lines 44 - 64, parseApkMetadata currently returns
success even when pkg.PackageName() is empty, producing degenerate AppMetadata;
update parseApkMetadata to validate that pkg.PackageName() is non-empty after
opening the APK and return a descriptive error (e.g., fmt.Errorf("apk has empty
package name")) if it is empty (ensure pkg is closed via the existing defer), so
AppMetadata.PackageName is never "" on success; keep the existing population of
Version/VersionCode logic unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
apps installnow returns the installed app's identity and version metadata alongside the success message, parsed locally from the file (no device round-trips, noexec).{ "status": "ok", "data": { "message": "Installed app from '…' on device …", "app": { "packageName": "com.mobilenext.playground", "version": "1.4.0", "versionCode": "42" } }}How
New
utils.ParseAppMetadata(path)dispatches by extension:.apk→ pure-Go AXML parse via new depgithub.com/shogo82148/androidbinary.ipa/.zip→ reads the top-level*.app/Info.plistfrom the archive via the existinghowett.net/plist(handles bothPayload/Foo.app/…and simulator-.zipFoo.app/…layouts; ignores nested framework/plugin plists).appdir → readsInfo.plistdirectlyUnified field mapping across platforms:
packageNamepackageCFBundleIdentifierversionversionNameCFBundleShortVersionStringversionCodeversionCodeCFBundleVersionMetadata extraction is best-effort: a parse failure logs via
utils.Verboseand omitsapprather than failing a successful install. Parsing the originalpathworks for all device types including remote (the file is local).Tests
utils/appmeta_test.go(+testdata/helloworld.apk) covers APK, IPA layout, simulator-zip layout,.appdirectory, nested-bundle exclusion, and unknown extension.go build ./...,go vet,go test ./utils/ ./commands/— pass