Feat/aab #18
Feat/aab #18
Conversation
# Conflicts: # src/package.ts
… messages with localized strings for better user feedback.
📝 WalkthroughWalkthroughThis PR migrates many app-parser utilities from JavaScript to TypeScript, adds AAB handling (AabParser with extractApk and uploadAab), updates CLI/docs/locales for new commands, bumps package version to 2.6.0, and updates GitHub Actions to use actions/checkout v5 and actions/setup-node v6. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Cmd as extractApk / uploadAab
participant Aab as AabParser
participant Bundle as bundletool (external)
participant FS as File System
participant I18n as i18n
User->>Cmd: run command with AAB path + options
Cmd->>Aab: new AabParser(aabPath)
Cmd->>Aab: call extractApk(outputPath, opts)
rect rgb(230,245,230)
Aab->>Bundle: execute bundletool build-apks (may use npx fallback)
Bundle-->>FS: write .apks archive
FS-->>Aab: return .apks archive
Aab->>FS: extract universal.apk from .apks
FS-->>Aab: universal.apk buffer
Aab->>FS: write universal.apk to outputPath (or return buffer)
end
Aab-->>Cmd: resolve success / throw localized error
Cmd->>I18n: request apkExtracted message
I18n-->>Cmd: localized string
Cmd->>User: log message or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api.ts (1)
61-61: Potential bug: Missing function call parentheses.Line 61 uses
await getBaseUrlwithout parentheses. IfgetBaseUrlis a function, this will await the function object itself (which resolves immediately) rather than calling it. This should likely beawait getBaseUrl().🔎 Proposed fix
- const baseUrl = await getBaseUrl; + const baseUrl = await getBaseUrl();
🧹 Nitpick comments (6)
src/package.ts (1)
218-253: Review the newextractApkcommand implementation.The implementation is generally solid with good input handling:
Strengths:
- Proper validation of source file extension
- Sensible default for output path
- Robust parsing of
includeAllSplits(handles both boolean and string 'true')- Safe parsing of comma-separated
splitswith trim and filterConsiderations:
- The command doesn't validate that the output directory exists or is writable before calling
parser.extractApk(). If the directory doesn't exist, the error will come from the parser rather than providing early feedback.- No explicit try-catch block, so errors propagate to the caller. This is acceptable if the CLI framework handles errors consistently.
Optional: Add output directory validation
+ const outputDir = path.dirname(output); + if (!fs.existsSync(outputDir)) { + throw new Error(t('outputDirectoryNotFound', { dir: outputDir })); + } + const parser = new AabParser(source); await parser.extractApk(output, { includeAllSplits, splits, });Note: This would require adding
fsimport and a new localization key.src/utils/app-info-parser/apk.js (2)
38-46: Consider logging when falling back to proto manifest parsing.The try-catch silently falls back to proto manifest parsing without any diagnostic output. Adding a debug log or comment would help troubleshoot parsing issues in production.
🔎 Proposed improvement
try { apkInfo = this._parseManifest(manifestBuffer); } catch (e) { - // 尝试解析 proto manifest（来自 AAB） + // Fallback: try parsing proto manifest (from AAB) + console.debug?.('[ApkParser] XML manifest parse failed, trying proto format'); apkInfo = this._parseProtoManifest( manifestBuffer, buffers[ResourceProtoName], ); }
208-243: Silent error swallowing may hide issues.
_resolveResourceFromProtocatches all exceptions and returnsnull, which could mask legitimate errors (e.g., corrupted proto data or schema mismatches). Consider logging a warning when resolution fails unexpectedly.🔎 Proposed improvement
} catch (e) { + console.warn('[Warning] Failed to resolve resource from proto:', e.message || e); return null; }src/utils/app-info-parser/aab.ts (3)
23-25: Consider adding a timeout to prevent indefinite hangs.The
execAsynccall has no timeout. Ifbundletoolhangs (e.g., waiting for input or a stalled process), the CLI will block indefinitely.🔎 Proposed improvement
- const execAsync = util.promisify(exec); + const execAsync = util.promisify(exec); + const TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes + const execWithTimeout = (cmd: string) => execAsync(cmd, { timeout: TIMEOUT_MS });Then use
execWithTimeout(cmd)instead ofexecAsync(cmd).
138-167: Consider hoistingrequirestatements to top-level imports.Using
require()inside methods (lines 140, 162) is unconventional in TypeScript and can complicate tree-shaking and static analysis. These could be top-level imports.🔎 Proposed improvement
+import ManifestXmlParser from './xml-parser/manifest'; +import ResourceFinder from './resource-finder'; // Then in _parseManifest: - const ManifestXmlParser = require('./xml-parser/manifest'); const parser = new ManifestXmlParser(buffer, { // And in _parseResourceMap: - const ResourceFinder = require('./resource-finder'); return new ResourceFinder().processResourceTable(buffer);
8-14: Optional: Remove redundantfileproperty assignment.The base
Zipclass already stores thefileproperty in its constructor. The assignmentthis.file = file;in theAabParserconstructor (line 13) is redundant and can be safely removed. The type declaration at the class level can remain for TypeScript typing purposes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/publish.ymlREADME.mdREADME.zh-CN.mdcli.jsonpackage.jsonsrc/api.tssrc/locales/en.tssrc/locales/zh.tssrc/modules/version-module.tssrc/package.tssrc/utils/app-info-parser/aab.jssrc/utils/app-info-parser/aab.tssrc/utils/app-info-parser/apk.jssrc/utils/app-info-parser/zip.jssrc/utils/dep-versions.tssrc/utils/http-helper.tssrc/utils/latest-version/index.tssrc/versions.ts
💤 Files with no reviewable changes (1)
- src/utils/app-info-parser/aab.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/utils/app-info-parser/zip.js (1)
test-modules.js (5)
require(9-9)require(20-20)require(31-31)require(42-42)require(53-53)
src/utils/app-info-parser/aab.ts (2)
src/utils/app-info-parser/apk.js (2)
require(4-8)ResourceName(10-10)src/utils/i18n.ts (1)
t(37-39)
src/package.ts (2)
src/api.ts (1)
getAllPackages(178-181)src/utils/i18n.ts (1)
t(37-39)
🪛 ast-grep (0.40.3)
src/utils/app-info-parser/aab.ts
[warning] 108-108: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapeRegExp(manifestPath)}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (29)
src/modules/version-module.ts (1)
1-1: Minor formatting improvement.The spacing before the closing brace in the type import statement now aligns with standard TypeScript/ESLint conventions.
package.json (1)
3-3: LGTM! Version bump aligns with new feature addition.The minor version bump from 2.5.0 to 2.6.0 follows semantic versioning conventions for the new
extractApkcommand feature.src/utils/dep-versions.ts (1)
32-38: LGTM! Formatting improvement enhances readability.The multi-line formatting of the reduce call with explicit trailing comma improves code readability while preserving identical behavior.
src/versions.ts (1)
9-9: LGTM! Type-only import follows TypeScript best practices.Using
import typefor types that are only used at the type level improves tree-shaking and helps avoid potential circular dependency issues.src/utils/app-info-parser/zip.js (1)
3-3: LGTM! Lazy loading optimization for bundle utilities.The lazy-loading pattern defers the
require('../../bundle')untilgetEntryFromHarmonyAppis actually invoked, improving startup performance when Harmony app functionality isn't needed. The cached module-scoped variable ensures the module is only loaded once.Also applies to: 53-54
.github/workflows/publish.yml (1)
17-17: Verify GitHub Actions runner compatibility before upgrading to actions/checkout v5.The upgrade from v4 to v5 includes a breaking change: the runtime is bumped to Node.js 24, which requires GitHub Actions Runner v2.327.1 or newer. Ensure your runner version meets this minimum requirement before merging.
README.zh-CN.md (1)
228-228: Documentation update is accurate and well-placed.The new
extractApkcommand is correctly documented in the Package 模块 section with appropriate parameter descriptions.src/utils/latest-version/index.ts (1)
230-231: Formatting improvement for readability.Splitting the authorization header assignment across lines improves readability without any functional impact.
src/utils/http-helper.ts (1)
1-2: Clean import organization with explicit dependency.Explicit import of
node-fetchis good practice and properly used in theping()function. Import ordering (external then local) is well-organized.README.md (1)
230-230: LGTM! Clear documentation for the new command.The
extractApkcommand is well-documented with its three options, following the same pattern as other commands in the Package Module section.cli.json (1)
59-71: LGTM! Command definition is well-structured.The
extractApkcommand options are properly defined:
output: accepts a value for the output pathincludeAllSplits: boolean flag defaulting to falsesplits: accepts comma-separated split namessrc/package.ts (3)
1-1: LGTM! Necessary imports for new functionality.The added imports support the new
extractApkcommand and improved error handling.
15-15: Good defensive programming with safe fallback.The
|| []fallback prevents potential errors ifgetAllPackagesreturnsnullorundefined, making the function more resilient.
53-53: Good use of optional chaining.The optional chaining
list?.findprevents runtime errors iflistisnullorundefined, improving code safety.src/locales/zh.ts (3)
5-12: LGTM! Comprehensive error messages for AAB parsing.The new localization strings provide clear Chinese translations for various AAB parsing error scenarios, enabling good user feedback when issues occur.
121-122: LGTM! Clear usage message for the new command.The usage message clearly documents the command syntax and available options in Chinese.
151-151: LGTM! Informative success message.The success message provides clear feedback with the output path, helping users locate the extracted APK.
src/api.ts (1)
9-9: No action required.getBaseUrlis correctly defined as a Promise object (an async IIFE that is immediately invoked with the trailing()), and the usage at line 61 asawait getBaseUrlis correct. No parentheses should be added.Likely an incorrect or invalid review comment.
src/utils/app-info-parser/apk.js (4)
2-3: LGTM!The new imports for
path,protobufjs, and theResourceProtoNameregex pattern are correctly added to support proto-based manifest parsing for AAB.Also applies to: 11-11
129-180: LGTM!The
_parseProtoManifestmethod correctly extractsversionName,versionCode,package, andmetaDatafrom the proto-encoded manifest. The structure navigation and attribute resolution logic is sound.
182-206: LGTM!The
_resolveProtoValuehelper properly handles resource references, primitive values, and direct attribute values with appropriate null checks and fallback logic.
113-127: TheResources.protofile exists at the expected path.The synchronous
protobuf.loadSync()is appropriate here since this is a CLI tool. No refactoring to async is necessary.src/locales/en.ts (2)
5-13: LGTM!The AAB-related localization keys are clear, consistently formatted with
{{error}}placeholders, and follow the existing naming convention (aab*prefix for AAB-related messages).
128-129: LGTM!The usage string for
extractApkclearly documents the command syntax with optional flags, and the success message uses the{{output}}placeholder correctly.Also applies to: 161-161
src/utils/app-info-parser/aab.ts (5)
108-110: Static analysis ReDoS warning is a false positive.The
escapeRegExpfunction at line 170-171 properly escapes all regex metacharacters, andmanifestPathis a hardcoded constant ('base/manifest/AndroidManifest.xml'), not user input. This is safe.
55-90: LGTM!The zip extraction logic correctly handles the async yauzl API with proper error handling, streaming, and cleanup. The
foundflag and event-driven approach are appropriate for extracting a single entry.
91-96: LGTM!Good use of
finallyto ensure cleanup of the temporary.apksfile even when extraction fails.
99-133: LGTM!The
parse()method correctly reads the AAB manifest frombase/manifest/AndroidManifest.xml, handles resource resolution gracefully with a warning, and provides clear error messages.
170-171: LGTM!The
escapeRegExputility correctly escapes all regex special characters, making it safe to construct regexes from literal paths.
| - uses: oven-sh/setup-bun@v2 | ||
| # Setup .npmrc file to publish to npm | ||
| - uses: actions/setup-node@v4 | ||
| - uses: actions/setup-node@v6 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
actions/setup-node v6 release breaking changes
💡 Result:
Short summary — breaking changes in actions/setup-node v6:
-
Automatic caching behavior narrowed: automatic cache is now enabled only for npm projects when package.json sets packageManager (or devEngines.packageManager); Yarn and pnpm caching is disabled by default and must be opted into via the cache input. [1][2]
-
The always-auth input was removed (deprecated; handling was removed in v6/v6.1.x). Remove any references to always-auth in your workflows. [1][2]
Sources:
[1] actions/setup-node README — “Breaking changes in V6”.
[2] actions/setup-node GitHub releases (v6.0.0 / v6.1.0 release notes).
Verify workflow compatibility with setup-node v6 breaking changes.
The upgrade to actions/setup-node@v6 introduces breaking changes:
- Automatic caching narrowed: Automatic cache is now enabled only for npm projects with
packageManager(ordevEngines.packageManager) specified inpackage.json. Yarn and pnpm caching are disabled by default and must be explicitly opted into via thecacheinput. - always-auth input removed: This deprecated input was removed in v6/v6.1.x. Ensure the workflow does not reference it.
Check the workflow and package.json to confirm caching behavior is still correct and that always-auth is not used.
🤖 Prompt for AI Agents
.github/workflows/publish.yml around line 20: the workflow was updated to
actions/setup-node@v6 which changed caching and removed the always-auth input;
to fix, inspect package.json for a packageManager (or devEngines.packageManager)
entry and add one if missing so automatic npm caching works, or explicitly set
the setup-node cache input (cache: 'yarn' or 'pnpm' or 'npm') to enable caching
for non-npm managers; also search the workflow for any use of always-auth and
remove it or replace with the appropriate npmrc/auth configuration supported by
v6; finally run the workflow locally or in a test branch to confirm caching and
auth behavior are correct.
…pt, remove deprecated apk.js and app.js files, and update import statements for consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/app-info-parser/aab.ts (1)
44-52: Command injection vulnerability via filename.This issue was already identified in a previous review. The
this.filepath is directly interpolated into shell commands without sanitization, allowing potential arbitrary command execution through malicious filenames.
🧹 Nitpick comments (6)
src/utils/app-info-parser/ipa.js (1)
5-5: Consider completing the ES module migration.While the ES module import for
Zipworks correctly, this file mixes ES imports (line 5) with CommonJS exports (line 94:module.exports). For consistency with the newly added parsers (apk.ts,aab.ts,app.ts) and to avoid potential module system conflicts, consider converting this file to TypeScript with full ES module syntax.🔎 Suggested conversion approach
- Rename
ipa.js→ipa.ts- Add TypeScript type annotations
- Replace
module.exports = IpaParserwithexport class IpaParser extends Zip- Convert remaining
require()calls toimportstatementssrc/package.ts (1)
230-235: Consider checking for existing output files (optional).The output path defaults to replacing the
.aabextension with.apkin the same directory. If the output file already exists, it will be silently overwritten. For better user experience, consider adding a warning or confirmation prompt when the output file exists, unless--forceor similar option is provided.src/utils/app-info-parser/apk.ts (2)
11-11: Consider a more specific return type (optional).The
parse()method returnsPromise<any>, which sacrifices type safety. Consider defining an interface for the parsed APK information (e.g.,ApkInfo) to provide better type checking and IDE support for consumers of this API.Example interface
interface ApkInfo { package: string; versionName: string; versionCode: number; icon?: string | null; // ... other fields } parse(): Promise<ApkInfo> { // ... }
83-89: Consider consistent error message formatting (nitpick).For consistency with
_parseManifest(line 75), consider usinge.message || einstead of just${e}in the error message. This ensures more readable error messages when the error object has amessageproperty.Suggested change
- throw new Error(`Parser resources.arsc error: ${e}`); + throw new Error(`Parser resources.arsc error: ${e.message || e}`);src/utils/app-info-parser/aab.ts (2)
23-25: Prefer top-level imports for Node.js built-in modules.The dynamic
requirestatements forchild_processandutilwork correctly but reduce code maintainability. Moving these to top-level imports aligns with TypeScript best practices and improves static analysis.🔎 Proposed refactor
At the top of the file, add:
import fs from 'fs-extra'; import path from 'path'; import os from 'os'; +import { exec as execCallback } from 'child_process'; +import { promisify } from 'util'; import { open as openZipFile } from 'yauzl';Then in the method:
- const { exec } = require('child_process'); - const util = require('util'); - const execAsync = util.promisify(exec); + const execAsync = promisify(execCallback);
122-122: Consider moving dynamic requires to top-level imports.Dynamic
requirestatements on lines 122, 140, and 162 reduce code clarity and prevent optimal tree-shaking. While functional, top-level imports would improve maintainability.🔎 Optional refactor
At the top of the file:
import { open as openZipFile } from 'yauzl'; import { Zip } from './zip'; import { t } from '../i18n'; +import ManifestXmlParser from './xml-parser/manifest'; +import ResourceFinder from './resource-finder'; +import { mapInfoResource } from './utils';Then update the methods:
- const { mapInfoResource } = require('./utils'); apkInfo = mapInfoResource(apkInfo, resourceMap);private _parseManifest(buffer: Buffer) { try { - const ManifestXmlParser = require('./xml-parser/manifest'); const parser = new ManifestXmlParser(buffer, {private _parseResourceMap(buffer: Buffer) { try { - const ResourceFinder = require('./resource-finder'); return new ResourceFinder().processResourceTable(buffer);Also applies to: 140-140, 162-162
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/package.tssrc/utils/app-info-parser/aab.tssrc/utils/app-info-parser/apk.tssrc/utils/app-info-parser/app.jssrc/utils/app-info-parser/app.tssrc/utils/app-info-parser/index.tssrc/utils/app-info-parser/ipa.jssrc/utils/app-info-parser/zip.js
💤 Files with no reviewable changes (1)
- src/utils/app-info-parser/app.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/app-info-parser/zip.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/utils/app-info-parser/app.ts (1)
src/utils/app-info-parser/zip.js (1)
Zip(5-66)
src/utils/app-info-parser/apk.ts (1)
src/utils/app-info-parser/zip.js (2)
require(2-2)Zip(5-66)
src/package.ts (5)
src/api.ts (1)
getAllPackages(178-181)src/utils/i18n.ts (1)
t(37-39)src/utils/app-info-parser/aab.ts (1)
AabParser(8-168)src/types.ts (1)
Platform(10-10)src/exports.ts (1)
Platform(15-15)
🪛 ast-grep (0.40.3)
src/utils/app-info-parser/aab.ts
[warning] 108-108: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapeRegExp(manifestPath)}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (9)
src/utils/app-info-parser/app.ts (1)
1-3: LGTM! Consistent with the parser architecture.The
AppParserclass follows the same inheritance pattern asApkParser,IpaParser, andAabParser, providing a consistent API surface even though no additional methods are currently defined. This serves as an appropriate extension point for future .app-specific functionality.src/utils/app-info-parser/index.ts (1)
1-4: LGTM! Named imports align with the ES module migration.The named imports for
ApkParser,AppParser, andAabParserare consistent with their ES module exports. TheIpaParsercontinues to userequire()becauseipa.jshas not yet been fully converted to ES modules (as noted in the review of that file).src/package.ts (3)
15-15: Good defensive coding.Using
|| []ensuresallPkgsis always an array, preventing potential runtime errors when iterating over the packages.
53-53: Good use of optional chaining.The optional chaining operator prevents a potential null reference error if
listis undefined.
264-269: LGTM! Improved type annotation formatting.The multi-line formatting and explicit
Platformtype annotation improve readability and maintain consistency with other command option types in this file.src/utils/app-info-parser/apk.ts (1)
62-77: LGTM! Well-structured manifest parsing.The private
_parseManifestmethod properly encapsulates manifest parsing logic with appropriate error handling. The ignore list for application components is reasonable for reducing parsing overhead when those details aren't needed.src/utils/app-info-parser/aab.ts (3)
108-110: Static analysis warning is a false positive.The static analysis tool flags a potential ReDoS risk from constructing a regex with variable input. However, this is safe because
manifestPathis a compile-time constant string (line 104), not user input, andescapeRegExpproperly sanitizes it.
118-127: Excellent graceful degradation for optional resource parsing.The try-catch block appropriately treats resource map parsing as optional, allowing the parse operation to succeed even if resource enrichment fails. This is good defensive coding for non-critical enhancements.
91-96: Proper temporary file cleanup.The finally block correctly ensures cleanup of temporary
.apksfiles regardless of success or failure, preventing resource leaks.
| extractApk: async ({ | ||
| args, | ||
| options, | ||
| }: { | ||
| args: string[]; | ||
| options: Record<string, any>; | ||
| }) => { | ||
| const source = args[0]; | ||
| if (!source || !source.endsWith('.aab')) { | ||
| throw new Error(t('usageExtractApk')); | ||
| } | ||
|
|
||
| const output = | ||
| options.output || | ||
| path.join( | ||
| path.dirname(source), | ||
| `${path.basename(source, path.extname(source))}.apk`, | ||
| ); | ||
|
|
||
| const includeAllSplits = | ||
| options.includeAllSplits === true || options.includeAllSplits === 'true'; | ||
| const splits = options.splits | ||
| ? String(options.splits) | ||
| .split(',') | ||
| .map((item) => item.trim()) | ||
| .filter(Boolean) | ||
| : null; | ||
|
|
||
| const parser = new AabParser(source); | ||
| await parser.extractApk(output, { | ||
| includeAllSplits, | ||
| splits, | ||
| }); | ||
|
|
||
| console.log(t('apkExtracted', { output })); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check README for bundletool mention
echo "=== Checking README for bundletool ==="
rg -i "bundletool" README.md || echo "No bundletool found in README.md"
# Check for any documentation of extractApk command
echo -e "\n=== Checking for extractApk documentation ==="
rg -i "extractapk|extract-apk" README.md docs/ 2>/dev/null || echo "No extractApk docs found"
# List documentation files
echo -e "\n=== Available documentation files ==="
git ls-files | grep -E "(README|docs/|\.md$)" | head -20Repository: reactnativecn/react-native-update-cli
Length of output: 511
🏁 Script executed:
#!/bin/bash
# Find and examine the AabParser class to understand bundletool usage
echo "=== Finding AabParser implementation ==="
fd -t f "AabParser" --type f
# Search for AabParser in the codebase
echo -e "\n=== Searching for AabParser class definition ==="
rg "class AabParser|export.*AabParser" -A 5
# Search for extractApk method in AabParser
echo -e "\n=== Searching for extractApk method ==="
rg "extractApk" -B 2 -A 10Repository: reactnativecn/react-native-update-cli
Length of output: 5520
🏁 Script executed:
#!/bin/bash
# Check how bundletool is invoked
echo "=== Checking for bundletool invocations ==="
rg "bundletool|node-bundletool" -A 3 -B 3
# Check for error messages related to bundletool or missing dependencies
echo -e "\n=== Checking for dependency error messages ==="
rg -i "bundletool.*not.*found|bundletool.*install|bundletool.*require" -A 2Repository: reactnativecn/react-native-update-cli
Length of output: 1580
🏁 Script executed:
#!/bin/bash
# Get the full AabParser.extractApk method
cat -n src/utils/app-info-parser/aab.ts | sed -n '1,200p'Repository: reactnativecn/react-native-update-cli
Length of output: 6837
🏁 Script executed:
#!/bin/bash
# Find the exact line numbers for extractApk method
rg -n "async extractApk" src/utils/app-info-parser/aab.ts
# Then read the full method
wc -l src/utils/app-info-parser/aab.tsRepository: reactnativecn/react-native-update-cli
Length of output: 140
🏁 Script executed:
#!/bin/bash
# Check the localization strings for bundletool/dependency error messages
echo "=== English error messages ==="
rg "aabOpenApksFailed|aabReadUniversalApkFailed|aabUniversalApkNotFound" src/locales/en.ts -A 1
echo -e "\n=== Chinese error messages ==="
rg "aabOpenApksFailed|aabReadUniversalApkFailed|aabUniversalApkNotFound" src/locales/zh.ts -A 1
# Check if there are any other error message keys for bundletool
echo -e "\n=== All aab-related error messages ==="
rg "aab[A-Z]" src/locales/en.ts | grep -E ":\s*['\"]"Repository: reactnativecn/react-native-update-cli
Length of output: 1094
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about bundletool in package.json or setup guides
echo "=== Checking package.json for bundletool reference ==="
rg "bundletool" package.json
# Check if there's a setup guide or install guide
echo -e "\n=== Looking for setup/install documentation ==="
fd -t f -e md | xargs grep -l -i "install\|setup\|depend" 2>/dev/null | head -10
# Check cli.json for extractApk full definition
echo -e "\n=== Checking cli.json extractApk definition ==="
rg -A 10 '"extractApk"' cli.jsonRepository: reactnativecn/react-native-update-cli
Length of output: 499
Document bundletool dependency and improve error messages for missing binary.
The extractApk command depends on bundletool being available, with a fallback to npx node-bundletool, but this dependency is not documented in the README or CLI help. Additionally, if both the direct bundletool command and the npx fallback fail, users see a raw shell error without guidance on installing bundletool.
Add bundletool to the README installation/dependencies section, and improve the error handling in AabParser.extractApk to catch bundletool failures and provide clear installation instructions.
🤖 Prompt for AI Agents
In src/package.ts around lines 218-253, the extractApk command currently assumes
bundletool is available but does not document that dependency or provide a
friendly error when the bundletool binary (or the npx fallback) fails; update
the README installation/dependencies section and the CLI help text to explicitly
state that bundletool (or node-bundletool via npx) is required and how to
install it, and modify the AabParser.extractApk implementation (the file
implementing AabParser, e.g., src/aab-parser.ts) to catch failures when invoking
bundletool or the npx fallback, surface a clear, user-friendly error that
includes the stderr/exception details and actionable install commands (e.g.,
install via Homebrew, download jar, or run `npx node-bundletool`), and ensure
the code first attempts the direct bundletool command, then the npx fallback,
and logs the underlying error when both fail.
…move deprecated utils.js, and enhance AAB and APK parsing with improved error handling and resource mapping.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/app-info-parser/apk.ts (1)
33-44: Type mismatch withgetEntryreturn type.Based on the
Zipclass context,getEntryreturnsPromise<Buffer | Blob | undefined>, but the callback parameter is typed asBuffer | null. This could cause type errors or unexpected behavior.🔎 Suggested fix
this.getEntry(iconPath) - .then((iconBuffer: Buffer | null) => { + .then((iconBuffer) => { apkInfo.icon = iconBuffer - ? getBase64FromBuffer(iconBuffer) + ? getBase64FromBuffer(iconBuffer as Buffer) : null; resolve(apkInfo); })src/utils/app-info-parser/xml-parser/binary.ts (1)
585-599: Inconsistent debug log message.The
readXmlElementEndmethod logs "readXmlCData" on line 586 instead of "readXmlElementEnd".🔎 Suggested fix
readXmlElementEnd() { - this.debug && console.group('readXmlCData'); + this.debug && console.group('readXmlElementEnd');
🧹 Nitpick comments (8)
src/utils/app-info-parser/index.ts (1)
7-46: Consider a union type for the parser property.The
parser: anyon line 9 could be typed asApkParser | IpaParser | AppParser | AabParserfor improved type safety and IDE support.🔎 Proposed improvement
class AppInfoParser { file: string | File; - parser: any; + parser: ApkParser | IpaParser | AppParser | AabParser;src/utils/app-info-parser/resource-finder.ts (2)
7-7: Consider using ESM import for consistency.The file uses CommonJS
requirefor ByteBuffer while other imports in the codebase use ESM. This works but is inconsistent with the migration pattern.
74-86: Silent catch may hide parsing errors.The
try/catchblock on lines 79-86 silently breaks on any error. While this may be intentional for graceful handling of truncated data, consider logging in debug mode or distinguishing between expected EOF and unexpected errors.🔎 Proposed improvement
try { pos = bb.offset; t = bb.readShort(); hs = bb.readShort(); s = bb.readInt(); } catch (e) { + if (DEBUG) { + console.log('Reached end of resource table or encountered read error'); + } break; }src/utils/app-info-parser/zip.ts (1)
67-85: Error swallowed silently in getEntryFromHarmonyApp.The method logs the error but returns
undefinedwithout propagating the failure. Callers cannot distinguish between "entry not found" and "error occurred". Consider re-throwing after logging or returning a result type.🔎 Proposed improvement
} catch (error) { console.error('Error in getEntryFromHarmonyApp:', error); + throw error; } }src/utils/app-info-parser/utils.ts (2)
74-76: Non-idiomatic array check using.splice.Line 75 checks for an array by testing if
.spliceexists. Consider usingArray.isArray()for clarity and correctness.🔎 Proposed fix
const findApkIconPath = (info: any) => { - if (!info.application.icon || !info.application.icon.splice) { + if (!info.application.icon || !Array.isArray(info.application.icon)) { return ''; }
88-95: Using.some()for side effects is non-idiomatic.The
.some()method is intended for finding elements, but here it's used to populateresultMapas a side effect. Consider using.find()or a simple loop.🔎 Proposed improvement
for (const i in rulesMap) { - info.application.icon.some((icon: string) => { - if (icon && icon.indexOf(i) !== -1) { - resultMap[`application-icon-${rulesMap[i]}`] = icon; - return true; - } - return false; - }); + const matchingIcon = info.application.icon.find( + (icon: string) => icon && icon.indexOf(i) !== -1 + ); + if (matchingIcon) { + resultMap[`application-icon-${rulesMap[i]}`] = matchingIcon; + }src/utils/app-info-parser/aab.ts (1)
12-18: Redundantfileproperty declaration.Line 13 declares
file: string | Filewhich is already defined in the parentZipclass (asfile: string | File | Blob). The assignment on line 17 is also redundant sincesuper(file)already assigns it.🔎 Proposed fix
export class AabParser extends Zip { - file: string | File; - constructor(file: string | File) { super(file); - this.file = file; }src/utils/app-info-parser/apk.ts (1)
9-55: Consider refactoring to async/await for improved readability.The nested promise chains with
.then()could be simplified using async/await syntax, making the control flow easier to follow. This is optional but would improve maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/utils/app-info-parser/aab.tssrc/utils/app-info-parser/apk.tssrc/utils/app-info-parser/index.tssrc/utils/app-info-parser/ipa.tssrc/utils/app-info-parser/resource-finder.jssrc/utils/app-info-parser/resource-finder.tssrc/utils/app-info-parser/utils.jssrc/utils/app-info-parser/utils.tssrc/utils/app-info-parser/xml-parser/binary.tssrc/utils/app-info-parser/xml-parser/manifest.tssrc/utils/app-info-parser/zip.jssrc/utils/app-info-parser/zip.ts
💤 Files with no reviewable changes (3)
- src/utils/app-info-parser/zip.js
- src/utils/app-info-parser/resource-finder.js
- src/utils/app-info-parser/utils.js
🧰 Additional context used
🧬 Code graph analysis (4)
src/utils/app-info-parser/xml-parser/manifest.ts (1)
src/utils/app-info-parser/xml-parser/binary.ts (1)
BinaryXmlParser(77-694)
src/utils/app-info-parser/zip.ts (2)
src/utils/app-info-parser/utils.ts (2)
isBrowser(156-156)decodeNullUnicode(161-161)src/bundle.ts (2)
enumZipEntries(777-842)readEntry(530-548)
src/utils/app-info-parser/ipa.ts (1)
src/utils/app-info-parser/zip.ts (1)
Zip(7-86)
src/utils/app-info-parser/apk.ts (4)
src/utils/app-info-parser/zip.ts (1)
Zip(7-86)src/utils/app-info-parser/utils.ts (3)
mapInfoResource(157-157)findApkIconPath(158-158)getBase64FromBuffer(160-160)src/utils/app-info-parser/xml-parser/manifest.ts (1)
ManifestParser(7-223)src/utils/app-info-parser/resource-finder.ts (1)
ResourceFinder(20-508)
🪛 ast-grep (0.40.3)
src/utils/app-info-parser/aab.ts
[warning] 146-146: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapeRegExp(manifestPath)}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (17)
src/utils/app-info-parser/ipa.ts (3)
14-22: Consider using a typed approach for buffer indexing.The
as anycasts on lines 16, 19, 21 work around TypeScript's inability to use RegExp as a Record key. This is acceptable given the underlying library's callback signature, but consider defining a local type alias if this pattern is used elsewhere.
61-72: Plist parsing logic looks correct.The method correctly handles three plist formats based on the first byte: XML (
<or UTF-8 BOM239), and binary (b). The fallback throws an appropriate error for unknown formats.
77-89: Safe handling of optional provision buffer.The method gracefully handles
undefinedbuffer input by returning an empty object, which prevents downstream errors.src/utils/app-info-parser/index.ts (1)
1-4: ESModule imports are correctly aligned with the new class exports.The named imports match the exported class names from each module.
src/utils/app-info-parser/xml-parser/manifest.ts (3)
7-14: Clean class structure with proper encapsulation.The ManifestParser class properly encapsulates the buffer and XML parser as private fields. The constructor initialization is straightforward.
16-22: Consider removing unnecessary Array.from wrapper.If
element.attributesis already iterable (NodeList-like),Array.from()may be unnecessary. However, this is safe and ensures compatibility.
143-155: Launcher activity detection logic is correct.The method properly checks for both
android.intent.action.MAINandandroid.intent.category.LAUNCHERto identify launcher activities.src/utils/app-info-parser/aab.ts (4)
35-56: Command execution properly uses spawn with argument arrays.The
runCommandhelper correctly usesspawnwith separate command and args, preventing shell injection. This addresses the previously flagged security concern.
146-148: Static analysis ReDoS warning is a false positive.The
escapeRegExpfunction on line 205 properly escapes all regex metacharacters before constructing the RegExp. SincemanifestPathis a hardcoded constant ('base/manifest/AndroidManifest.xml'), there's no user input that could cause ReDoS.
93-128: ZIP extraction handles edge cases well.The code properly handles:
- Missing zipfile or errors on open
- Entry-by-entry lazy reading
- Stream piping with error handling
- "universal.apk not found" case on
endevent
129-134: Good cleanup pattern with finally block.The temp file cleanup in
finallyensures resources are freed even if extraction fails.src/utils/app-info-parser/apk.ts (1)
1-5: Clean ES module imports.The migration to ES module imports looks good. All dependencies are correctly imported.
src/utils/app-info-parser/xml-parser/binary.ts (5)
73-96: Clean TypeScript class definition with explicit field types.Good migration to TypeScript with properly typed class fields and constructor options.
207-238:readFractionimplementation is correct.Unlike
readDimension, this method correctly uses the localvaluevariable to extract the type.
254-356: Well-structured typed value parsing with comprehensive type handling.The switch statement covers all the Android TypedValue types properly, and the cursor drift detection at the end is a good defensive measure.
365-399: String encoding handling looks correct.Both UTF-8 and UCS-2 encodings are properly handled with their respective length reading strategies.
636-693: Parse method handles chunk types comprehensively.The main parse loop correctly handles all XML chunk types and includes good error handling for unsupported types and cursor drift detection.
| private _parseResourceMap(buffer: Buffer) { | ||
| try { | ||
| return new ResourceFinder().processResourceTable(buffer); | ||
| } catch (e) { | ||
| throw new Error('Parser resources.arsc error: ' + e); | ||
| } catch (e: any) { | ||
| throw new Error(`Parser resources.arsc error: ${e}`); | ||
| } |
There was a problem hiding this comment.
Typo in error message: "Parser" should be "Parse".
The error message says "Parser resources.arsc error" but should be "Parse resources.arsc error" for consistency with the manifest error message on line 74.
🔎 Suggested fix
} catch (e: any) {
- throw new Error(`Parser resources.arsc error: ${e}`);
+ throw new Error(`Parse resources.arsc error: ${e}`);
}📝 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.
| private _parseResourceMap(buffer: Buffer) { | |
| try { | |
| return new ResourceFinder().processResourceTable(buffer); | |
| } catch (e) { | |
| throw new Error('Parser resources.arsc error: ' + e); | |
| } catch (e: any) { | |
| throw new Error(`Parser resources.arsc error: ${e}`); | |
| } | |
| private _parseResourceMap(buffer: Buffer) { | |
| try { | |
| return new ResourceFinder().processResourceTable(buffer); | |
| } catch (e: any) { | |
| throw new Error(`Parse resources.arsc error: ${e}`); | |
| } |
🤖 Prompt for AI Agents
In src/utils/app-info-parser/apk.ts around lines 82 to 87, the thrown error
message contains a typo ("Parser resources.arsc error"); change the message to
"Parse resources.arsc error" so it matches the manifest error wording and keep
the existing interpolation of the caught error (e) unchanged.
| for (const refKey in refKeys) { | ||
| const values = | ||
| this.responseMap[ | ||
| `@${Number(refKeys[refKey]).toString(16).toUpperCase()}` | ||
| ]; | ||
| if (values != null && Object.keys(values).length < 1000) { | ||
| for (const value of values) { | ||
| this.putIntoMap(`@${refKey}`, value); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Incorrect length check on array.
Line 372 uses Object.keys(values).length on an array. While this works, values.length is more idiomatic and performant for arrays.
🔎 Proposed fix
const values =
this.responseMap[
`@${Number(refKeys[refKey]).toString(16).toUpperCase()}`
];
- if (values != null && Object.keys(values).length < 1000) {
+ if (values != null && values.length < 1000) {
for (const value of values) {
this.putIntoMap(`@${refKey}`, value);
}
}📝 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.
| for (const refKey in refKeys) { | |
| const values = | |
| this.responseMap[ | |
| `@${Number(refKeys[refKey]).toString(16).toUpperCase()}` | |
| ]; | |
| if (values != null && Object.keys(values).length < 1000) { | |
| for (const value of values) { | |
| this.putIntoMap(`@${refKey}`, value); | |
| } | |
| } | |
| } | |
| for (const refKey in refKeys) { | |
| const values = | |
| this.responseMap[ | |
| `@${Number(refKeys[refKey]).toString(16).toUpperCase()}` | |
| ]; | |
| if (values != null && values.length < 1000) { | |
| for (const value of values) { | |
| this.putIntoMap(`@${refKey}`, value); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/utils/app-info-parser/resource-finder.ts around lines 367 to 377, the
code checks array size using Object.keys(values).length which is non-idiomatic
and slower; change the check to use values.length (and optionally guard that
values is an array) so the condition becomes values != null && values.length <
1000, preserving behavior but improving clarity and performance.
| const rulesMap: Record<string, number> = { | ||
| mdpi: 48, | ||
| hdpi: 72, | ||
| xhdpi: 96, | ||
| xxdpi: 144, | ||
| xxxhdpi: 192, | ||
| }; |
There was a problem hiding this comment.
Typo in DPI key: "xxdpi" should be "xxhdpi".
Line 83 has xxdpi: 144 which should be xxhdpi: 144 to match Android's density naming convention.
🔎 Proposed fix
const rulesMap: Record<string, number> = {
mdpi: 48,
hdpi: 72,
xhdpi: 96,
- xxdpi: 144,
+ xxhdpi: 144,
xxxhdpi: 192,
};📝 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.
| const rulesMap: Record<string, number> = { | |
| mdpi: 48, | |
| hdpi: 72, | |
| xhdpi: 96, | |
| xxdpi: 144, | |
| xxxhdpi: 192, | |
| }; | |
| const rulesMap: Record<string, number> = { | |
| mdpi: 48, | |
| hdpi: 72, | |
| xhdpi: 96, | |
| xxhdpi: 144, | |
| xxxhdpi: 192, | |
| }; |
🤖 Prompt for AI Agents
In src/utils/app-info-parser/utils.ts around lines 78 to 84, the density key has
a typo: "xxdpi" is used but should be "xxhdpi"; update the rulesMap key from
"xxdpi" to "xxhdpi" so it matches Android naming convention (mdpi, hdpi, xhdpi,
xxhdpi, xxxhdpi) and keep the corresponding value 144 unchanged.
| if (Object.keys(resultMap).length === 0 || !maxDpiIcon.icon) { | ||
| maxDpiIcon.dpi = 120; | ||
| maxDpiIcon.icon = info.application.icon[0] || ''; | ||
| resultMap['applicataion-icon-120'] = maxDpiIcon.icon; | ||
| } |
There was a problem hiding this comment.
Typo in fallback key: "applicataion" should be "application".
Line 109 has a typo that could cause issues if this key is used elsewhere for lookups.
🔎 Proposed fix
if (Object.keys(resultMap).length === 0 || !maxDpiIcon.icon) {
maxDpiIcon.dpi = 120;
maxDpiIcon.icon = info.application.icon[0] || '';
- resultMap['applicataion-icon-120'] = maxDpiIcon.icon;
+ resultMap['application-icon-120'] = maxDpiIcon.icon;
}📝 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.
| if (Object.keys(resultMap).length === 0 || !maxDpiIcon.icon) { | |
| maxDpiIcon.dpi = 120; | |
| maxDpiIcon.icon = info.application.icon[0] || ''; | |
| resultMap['applicataion-icon-120'] = maxDpiIcon.icon; | |
| } | |
| if (Object.keys(resultMap).length === 0 || !maxDpiIcon.icon) { | |
| maxDpiIcon.dpi = 120; | |
| maxDpiIcon.icon = info.application.icon[0] || ''; | |
| resultMap['application-icon-120'] = maxDpiIcon.icon; | |
| } |
🤖 Prompt for AI Agents
In src/utils/app-info-parser/utils.ts around lines 106 to 110, the fallback map
key is misspelled as "applicataion-icon-120"; change it to
"application-icon-120" so the resultMap uses the correct key, and update any
related occurrences to the correct spelling to ensure lookups use the same key.
| const dimension: { | ||
| value: number | null; | ||
| unit: string | null; | ||
| rawUnit: number | null; | ||
| } = { | ||
| value: null, | ||
| unit: null, | ||
| rawUnit: null, | ||
| }; | ||
|
|
||
| const value = this.readU32(); | ||
| const unit = dimension.value & 0xff; | ||
| const unit = (dimension.value ?? 0) & 0xff; | ||
|
|
||
| dimension.value = value >> 8; | ||
| dimension.rawUnit = unit; |
There was a problem hiding this comment.
Critical bug: unit calculation uses wrong variable.
Line 176 uses dimension.value ?? 0 to calculate the unit, but at this point dimension.value is still null (initialized on line 170). This means unit will always be 0, causing incorrect dimension parsing. It should use the value variable read from the buffer on line 175.
🔎 Proposed fix
const value = this.readU32();
- const unit = (dimension.value ?? 0) & 0xff;
+ const unit = value & 0xff;
dimension.value = value >> 8;📝 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.
| const dimension: { | |
| value: number | null; | |
| unit: string | null; | |
| rawUnit: number | null; | |
| } = { | |
| value: null, | |
| unit: null, | |
| rawUnit: null, | |
| }; | |
| const value = this.readU32(); | |
| const unit = dimension.value & 0xff; | |
| const unit = (dimension.value ?? 0) & 0xff; | |
| dimension.value = value >> 8; | |
| dimension.rawUnit = unit; | |
| const dimension: { | |
| value: number | null; | |
| unit: string | null; | |
| rawUnit: number | null; | |
| } = { | |
| value: null, | |
| unit: null, | |
| rawUnit: null, | |
| }; | |
| const value = this.readU32(); | |
| const unit = value & 0xff; | |
| dimension.value = value >> 8; | |
| dimension.rawUnit = unit; |
🤖 Prompt for AI Agents
In src/utils/app-info-parser/xml-parser/binary.ts around lines 165 to 179, the
unit is computed from dimension.value which is still null; change the
computation to derive unit from the local variable value (read from the buffer)
instead. Read value first, compute unit as value & 0xff, then set
dimension.value = value >> 8 and dimension.rawUnit = unit so the parsed
dimension uses the correct unit byte.
| constructor(file: string | File | Blob) { | ||
| if (isBrowser()) { | ||
| if (!(file instanceof window.Blob || typeof file.size !== 'undefined')) { | ||
| throw new Error( | ||
| 'Param error: [file] must be an instance of Blob or File in browser.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Browser file validation condition may be incorrect.
Line 13's condition !(file instanceof window.Blob || typeof file.size !== 'undefined') has a logic issue. The typeof file.size !== 'undefined' check is likely meant to validate File objects, but it would pass for any object with a size property. Consider using instanceof File instead.
🔎 Proposed fix
constructor(file: string | File | Blob) {
if (isBrowser()) {
- if (!(file instanceof window.Blob || typeof file.size !== 'undefined')) {
+ if (!(file instanceof window.Blob || file instanceof window.File)) {
throw new Error(
'Param error: [file] must be an instance of Blob or File in browser.',
);
}📝 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.
| constructor(file: string | File | Blob) { | |
| if (isBrowser()) { | |
| if (!(file instanceof window.Blob || typeof file.size !== 'undefined')) { | |
| throw new Error( | |
| 'Param error: [file] must be an instance of Blob or File in browser.', | |
| ); | |
| } | |
| constructor(file: string | File | Blob) { | |
| if (isBrowser()) { | |
| if (!(file instanceof window.Blob || file instanceof window.File)) { | |
| throw new Error( | |
| 'Param error: [file] must be an instance of Blob or File in browser.', | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/utils/app-info-parser/zip.ts around lines 11 to 17, the browser
validation uses the condition !(file instanceof window.Blob || typeof file.size
!== 'undefined') which is too permissive because typeof file.size !==
'undefined' will accept any object with a size property; replace that check with
a proper instanceof File check (e.g., file instanceof File) so the condition
becomes something like !(file instanceof Blob || file instanceof File) and throw
the same error when the check fails; ensure you reference the global Blob/File
correctly in the browser environment.
…AB file uploads, including options for split management and update documentation in README files. Remove deprecated .babelrc configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/package.ts (1)
267-302: Acknowledge past review: bundletool dependency documentation needed.As noted in the previous review comment, the
extractApkcommand relies onbundletool(ornpx node-bundletoolas a fallback), but this dependency is not documented in user-facing documentation. The past review recommendation to add bundletool installation instructions to the README and improve error messages when bundletool is missing remains valid and should be addressed.
🧹 Nitpick comments (1)
src/package.ts (1)
171-178: Consider extracting duplicated option parsing logic.The parsing logic for
includeAllSplitsandsplitsis duplicated betweenuploadAab(lines 171-178) andextractApk(lines 286-293). Consider extracting this into a shared helper function to improve maintainability.💡 Suggested refactor
function parseAabExtractOptions(options: Record<string, any>) { const includeAllSplits = options.includeAllSplits === true || options.includeAllSplits === 'true'; const splits = options.splits ? String(options.splits) .split(',') .map((item) => item.trim()) .filter(Boolean) : null; return { includeAllSplits, splits }; }Then use it in both commands:
const { includeAllSplits, splits } = parseAabExtractOptions(options);Also applies to: 286-293
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.babelrcREADME.mdREADME.zh-CN.mdcli.jsonsrc/locales/en.tssrc/locales/zh.tssrc/package.tssrc/provider.ts
💤 Files with no reviewable changes (1)
- .babelrc
🚧 Files skipped from review as they are similar to previous changes (2)
- README.zh-CN.md
- README.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/provider.ts (1)
src/package.ts (1)
packageCommands(67-364)
src/package.ts (3)
src/api.ts (1)
getAllPackages(178-181)src/utils/i18n.ts (1)
t(37-39)src/utils/app-info-parser/aab.ts (1)
AabParser(12-203)
🔇 Additional comments (8)
cli.json (1)
48-84: LGTM! Well-structured command definitions.The new
uploadAabandextractApkcommands are properly configured with consistent option structures that align with their implementations.src/locales/zh.ts (1)
5-12: LGTM! Complete localization coverage.All AAB-related localization strings are properly defined and consistent with the English locale.
Also applies to: 121-122, 126-127, 153-153
src/locales/en.ts (1)
5-13: LGTM! Comprehensive localization.English localization strings properly mirror the Chinese locale and provide clear messaging for AAB operations.
Also applies to: 128-129, 134-135, 163-163
src/provider.ts (1)
125-127: LGTM! Consistent integration.The AAB upload routing follows the established pattern for other file types and properly delegates to the new
uploadAabcommand.src/package.ts (4)
1-19: LGTM! Clean import organization.The imports are well-organized with the addition of necessary dependencies (os, path, fs-extra, AabParser) for AAB handling.
22-22: LGTM! Good defensive coding improvements.The defensive default
|| []on Line 22 prevents potential null/undefined issues, and the optional chaining on Line 60 adds safety when accessing the list.Also applies to: 60-60
154-195: LGTM! Well-implemented AAB upload flow.The
uploadAabimplementation correctly:
- Validates input
- Extracts APK to a temporary location
- Delegates to
uploadApk- Cleans up temporary files in a finally block
The option parsing for
includeAllSplitshandles both boolean and string values, which is appropriate for CLI compatibility.
313-318: LGTM! Type signature appropriately expanded.The explicit typing of
deletePackageoptions to includeplatform?: Platformimproves type safety and clarity.
…mprove error handling in runCommand function. Refactor command execution to support stdio options.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.