fix(ios): patch AudioSet.swift for Release build (#741) #746
Conversation
|
Warning Rate limit exceeded@hyochan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds documentation describing a Swift optional-access bug and a TypeScript patch script that replaces unsafe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Summary of ChangesHello @hyochan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a significant issue affecting iOS Release builds where audio configuration properties were not correctly interpreted, leading to unexpected behavior. The core of the solution involves a targeted patch to the generated Swift code to ensure safe handling of optional values when bridging between Swift and C++, preventing data corruption. Additionally, it automates this patching process and updates key dependencies to their latest versions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical bug on iOS where audio codec properties were being corrupted in Release builds. The root cause was an unsafe accessor (.value) on std::optional types in the Swift-C++ bridge, which was sensitive to compiler optimizations. The fix involves patching the generated AudioSet.swift file to use a safer pattern (.has_value() ? .pointee : nil) for all optional properties. A new script, scripts/patch-audioset.ts, has been added to automate this patching process after code generation. The changes are well-implemented and directly address the reported issue. I've included one suggestion to make the patch script more robust by extending its scope to other generated files that suffer from the same bug, and to improve how the file header is updated to preserve the original warnings.
scripts/patch-audioset.ts
Outdated
| #!/usr/bin/env npx tsx | ||
| /** | ||
| * Patches the generated AudioSet.swift file to fix Release build optimization issues. | ||
| * See: https://github.com/hyochan/react-native-nitro-sound/issues/741 | ||
| * | ||
| * The nitrogen code generator uses `.value` accessor for std::optional which causes | ||
| * corruption in Release builds. This script replaces it with the safe pattern: | ||
| * `.has_value() ? .pointee : nil` | ||
| */ | ||
|
|
||
| import * as fs from 'fs'; | ||
| import * as path from 'path'; | ||
|
|
||
| const AUDIOSET_PATH = path.join( | ||
| __dirname, | ||
| '../nitrogen/generated/ios/swift/AudioSet.swift' | ||
| ); | ||
|
|
||
| // Properties that need to be patched (using unsafe .value pattern) | ||
| const PROPERTIES_TO_PATCH = [ | ||
| 'AVModeIOS', | ||
| 'AVEncodingOptionIOS', | ||
| 'AVFormatIDKeyIOS', | ||
| 'AVNumberOfChannelsKeyIOS', | ||
| 'AVSampleRateKeyIOS', | ||
| 'AudioQuality', | ||
| 'AudioChannels', | ||
| 'AudioSamplingRate', | ||
| 'AudioEncodingBitRate', | ||
| ]; | ||
|
|
||
| function patchAudioSet(): void { | ||
| console.log('🔧 Patching AudioSet.swift for Release build compatibility...'); | ||
|
|
||
| if (!fs.existsSync(AUDIOSET_PATH)) { | ||
| console.error('❌ AudioSet.swift not found at:', AUDIOSET_PATH); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| let content = fs.readFileSync(AUDIOSET_PATH, 'utf-8'); | ||
| let patchCount = 0; | ||
|
|
||
| for (const prop of PROPERTIES_TO_PATCH) { | ||
| // Match the unsafe pattern: return self.__PropertyName.value | ||
| const unsafePattern = new RegExp(`return self\\.__${prop}\\.value`, 'g'); | ||
| const safeReplacement = `return self.__${prop}.has_value() ? self.__${prop}.pointee : nil`; | ||
|
|
||
| if (unsafePattern.test(content)) { | ||
| content = content.replace(unsafePattern, safeReplacement); | ||
| patchCount++; | ||
| console.log(` ✅ Patched: ${prop}`); | ||
| } | ||
| } | ||
|
|
||
| // Update the header comment to indicate it's been patched | ||
| const originalHeader = | ||
| '/// This file was generated by nitrogen. DO NOT MODIFY THIS FILE.'; | ||
| const patchedHeader = `/// This file was generated by nitrogen. | ||
| /// PATCHED: Fixed .value accessors to use .has_value() ? .pointee : nil pattern | ||
| /// to fix Release build optimization issues (see #741)`; | ||
|
|
||
| if (content.includes(originalHeader)) { | ||
| content = content.replace(originalHeader, patchedHeader); | ||
| } | ||
|
|
||
| fs.writeFileSync(AUDIOSET_PATH, content, 'utf-8'); | ||
|
|
||
| if (patchCount > 0) { | ||
| console.log( | ||
| `\n✅ Successfully patched ${patchCount} properties in AudioSet.swift` | ||
| ); | ||
| } else { | ||
| console.log( | ||
| '\n⚠️ No properties needed patching (already patched or pattern changed)' | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| patchAudioSet(); |
There was a problem hiding this comment.
This patch script is a great ad-hoc solution to fix the code generation bug. To make it more robust and maintainable, I have a couple of suggestions:
-
Extend the scope: The same bug (
.valueaccessor on optional properties) exists in other generated files. For example, innitrogen/generated/ios/swift/RecordBackType.swift, thecurrentMeteringandrecordSecsproperties are also affected. Consider renaming this script to something more generic (e.g.,patch-swift-optionals.ts) and extending it to patch all affected files. This will ensure all instances of this bug are fixed. -
Preserve header warning: The current implementation replaces the "DO NOT MODIFY" warning with the patch information. It would be better to preserve the original warning and append the patch details. This ensures developers are still warned that the file is auto-generated. You could adjust the header replacement logic like this:
// In patchAudioSet() const originalHeader = '/// This file was generated by nitrogen. DO NOT MODIFY THIS FILE.'; const patchedHeader = `/// This file was generated by nitrogen. DO NOT MODIFY THIS FILE.\n/// PATCHED: Fixed .value accessors to use .has_value() ? .pointee : nil pattern\n/// to fix Release build optimization issues (see #741)`; if (content.includes(originalHeader)) { content = content.replace(originalHeader, patchedHeader); }
There was a problem hiding this comment.
Please submit a PR to nitro instead of posting patches :)
There was a problem hiding this comment.
Ok I just created a PR since I was already working on Nitro right now, but in the future please prefer PRs over patches. Patches are never sustainable for OSS.
I still need your help tho, in Nitro we have an example for an optional double, and it builds just fine in Release.
So I need your help to add an example to the Nitro repo that demonstrates exactly how this can break if this fix is not applied. mrousavy/nitro#1065
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ISSUE_741_RESPONSE.md (1)
22-83: Doc is technically aligned; only minor markdownlint nits if you careThe analysis, patterns A/B, and the list of affected properties all match the behavior of
scripts/patch-audioset.ts, so the doc is consistent with the actual fix and gives users clear manual steps.If you want to fully satisfy
markdownlint:
- Add blank lines before and after the fenced Swift code blocks for Patterns A/B and the replacement snippet (MD031).
- Optionally wrap the bare NitroModules URL in a markdown link (
[NitroModules](https://...)) to avoid the bare-URL warning (MD034).These are purely cosmetic; content-wise this file looks solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (25)
nitrogen/generated/android/c++/JFunc_void_PlayBackType.hppis excluded by!**/generated/**nitrogen/generated/android/c++/JFunc_void_PlaybackEndType.hppis excluded by!**/generated/**nitrogen/generated/android/c++/JFunc_void_RecordBackType.hppis excluded by!**/generated/**nitrogen/generated/android/c++/JHybridSoundSpec.cppis excluded by!**/generated/**nitrogen/generated/android/c++/JHybridSoundSpec.hppis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/sound/AudioSet.ktis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/sound/HybridSoundSpec.ktis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/sound/PlayBackType.ktis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/sound/PlaybackEndType.ktis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/sound/RecordBackType.ktis excluded by!**/generated/**nitrogen/generated/ios/NitroSound-Swift-Cxx-Bridge.cppis excluded by!**/generated/**nitrogen/generated/ios/NitroSound-Swift-Cxx-Bridge.hppis excluded by!**/generated/**nitrogen/generated/ios/c++/HybridSoundSpecSwift.hppis excluded by!**/generated/**nitrogen/generated/ios/swift/AudioSet.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/Func_void_PlayBackType.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/Func_void_PlaybackEndType.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/Func_void_RecordBackType.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/Func_void_std__exception_ptr.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/Func_void_std__string.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/HybridSoundSpec.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/HybridSoundSpec_cxx.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/PlayBackType.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/PlaybackEndType.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/RecordBackType.swiftis excluded by!**/generated/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
ISSUE_741_RESPONSE.md(1 hunks)example/package.json(1 hunks)package.json(2 hunks)scripts/patch-audioset.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use 2-space indentation, single quotes, and es5 trailing commas in TypeScript code (enforced by Prettier/ESLint)
Use camelCase for variables and functions in TypeScript
Use PascalCase for classes and type names in TypeScript
Files:
scripts/patch-audioset.ts
🧠 Learnings (3)
📚 Learning: 2025-09-07T05:13:03.063Z
Learnt from: CR
Repo: hyochan/react-native-nitro-sound PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T05:13:03.063Z
Learning: Use Jest with the react-native preset for testing
Applied to files:
example/package.jsonpackage.json
📚 Learning: 2025-09-07T05:13:03.063Z
Learnt from: CR
Repo: hyochan/react-native-nitro-sound PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T05:13:03.063Z
Learning: Applies to nitrogen/generated/** : Never hand-edit generated native bindings under nitrogen/generated/**
Applied to files:
package.json
📚 Learning: 2025-09-07T05:13:03.063Z
Learnt from: CR
Repo: hyochan/react-native-nitro-sound PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T05:13:03.063Z
Learning: Modify behavior via Nitro specs (src/specs/*.nitro.ts) and native implementations, not by editing generated bindings
Applied to files:
package.json
🪛 ast-grep (0.40.0)
scripts/patch-audioset.ts
[warning] 44-44: 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(return self\\.__${prop}\\.value, 'g')
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)
🪛 LanguageTool
ISSUE_741_RESPONSE.md
[style] ~12-~12: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1976 characters long)
Context: ...Optional(2.0) | Optional(5.0) (wrong!) | | AudioSamplingRate | `Optional(22...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
ISSUE_741_RESPONSE.md
23-23: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
32-32: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
82-82: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: ci-web / build-web
- GitHub Check: ci-android / build-android
- GitHub Check: ci-ios / ios-dependencies
- GitHub Check: build-android
🔇 Additional comments (3)
example/package.json (1)
21-21: Example app nitro-modules bump is consistent; just re-run buildsAligning
react-native-nitro-modulesin the example with^0.31.10keeps the workspace coherent and avoids mismatched generator/runtime combos. Please re-runyarn installand the example iOS/Android builds to confirm there are no version-related regressions.package.json (1)
47-47: Nitrogen + patch script wiring and version bumps look coherentChaining
npx nitrogen && npx tsx scripts/patch-audioset.tsin thenitrogenscript correctly ensures the Swift bindings are generated before the patcher runs, and bumping bothnitrogenandreact-native-nitro-modulesto^0.31.10keeps the tooling stack aligned. Please verify that:
yarn nitrogen(andbob build) succeed on a clean checkout, and- the resulting iOS Release build for the example app passes without the AudioSet issue.
Also applies to: 96-101
scripts/patch-audioset.ts (1)
1-79: Patch script logic is sound and idempotent; regex warning is benign hereThe overall flow looks good: you guard on
AUDIOSET_PATHexistence, patch only a known set of properties via a precisereturn self.__${prop}.valuepattern, update the header once, and log how many properties were touched. The script is idempotent (second run finds no matches and leaves the already-patched header alone), which plays nicely with it being wired into thenitrogenscript.Re the static-analysis warning about
new RegExp(\return self\.__${prop}\.value`, 'g'): sincepropcomes from the fixedPROPERTIES_TO_PATCH` array (not from user input), there’s no practical ReDoS risk here, and the pattern is narrow enough that accidental over-matching is unlikely.The only operational thing to keep an eye on is that both the unsafe pattern and
originalHeadermust stay in sync with nitrogen’s generator output; if those change, the patch script will silently become a no-op apart from the “no properties needed patching” log. That’s probably acceptable for a stopgap like this, but worth remembering once the upstream generator is fixed so this script can eventually be retired. Based on learnings, this automated patcher is a reasonable compromise versus hand-editing generated Swift.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/patch-audioset.ts (1)
43-53: Minor efficiency improvement opportunity.The current pattern calls
test()followed byreplace()on the same regex. This runs the pattern matching twice. Consider simplifying to just perform the replacement and check if the content changed:Apply this diff:
for (const prop of properties) { // Match the unsafe pattern: return self.__PropertyName.value const unsafePattern = new RegExp(`return self\\.__${prop}\\.value`, 'g'); const safeReplacement = `return self.__${prop}.has_value() ? self.__${prop}.pointee : nil`; - if (unsafePattern.test(content)) { - content = content.replace(unsafePattern, safeReplacement); + const newContent = content.replace(unsafePattern, safeReplacement); + if (newContent !== content) { + content = newContent; patchCount++; console.log(` ✅ Patched: ${prop}`); } }Note on the ReDoS warning: The static analysis tool flagged the regex construction from a variable (line 45). This is a false positive—the property names are hardcoded in the script configuration and are simple identifiers, so there's no ReDoS risk in this build-time script.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (25)
nitrogen/generated/android/c++/JFunc_void_PlayBackType.hppis excluded by!**/generated/**nitrogen/generated/android/c++/JFunc_void_PlaybackEndType.hppis excluded by!**/generated/**nitrogen/generated/android/c++/JFunc_void_RecordBackType.hppis excluded by!**/generated/**nitrogen/generated/android/c++/JHybridSoundSpec.cppis excluded by!**/generated/**nitrogen/generated/android/c++/JHybridSoundSpec.hppis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/sound/AudioSet.ktis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/sound/HybridSoundSpec.ktis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/sound/PlayBackType.ktis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/sound/PlaybackEndType.ktis excluded by!**/generated/**nitrogen/generated/android/kotlin/com/margelo/nitro/sound/RecordBackType.ktis excluded by!**/generated/**nitrogen/generated/ios/NitroSound-Swift-Cxx-Bridge.cppis excluded by!**/generated/**nitrogen/generated/ios/NitroSound-Swift-Cxx-Bridge.hppis excluded by!**/generated/**nitrogen/generated/ios/c++/HybridSoundSpecSwift.hppis excluded by!**/generated/**nitrogen/generated/ios/swift/AudioSet.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/Func_void_PlayBackType.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/Func_void_PlaybackEndType.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/Func_void_RecordBackType.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/Func_void_std__exception_ptr.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/Func_void_std__string.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/HybridSoundSpec.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/HybridSoundSpec_cxx.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/PlayBackType.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/PlaybackEndType.swiftis excluded by!**/generated/**nitrogen/generated/ios/swift/RecordBackType.swiftis excluded by!**/generated/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
ISSUE_741_RESPONSE.md(1 hunks)example/package.json(1 hunks)package.json(2 hunks)scripts/patch-audioset.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- example/package.json
- package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use 2-space indentation, single quotes, and es5 trailing commas in TypeScript code (enforced by Prettier/ESLint)
Use camelCase for variables and functions in TypeScript
Use PascalCase for classes and type names in TypeScript
Files:
scripts/patch-audioset.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: hyochan/react-native-nitro-sound PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T05:13:03.063Z
Learning: Modify behavior via Nitro specs (src/specs/*.nitro.ts) and native implementations, not by editing generated bindings
📚 Learning: 2025-09-07T05:13:03.063Z
Learnt from: CR
Repo: hyochan/react-native-nitro-sound PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T05:13:03.063Z
Learning: Modify behavior via Nitro specs (src/specs/*.nitro.ts) and native implementations, not by editing generated bindings
Applied to files:
scripts/patch-audioset.ts
🪛 ast-grep (0.40.0)
scripts/patch-audioset.ts
[warning] 44-44: 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(return self\\.__${prop}\\.value, 'g')
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)
🪛 LanguageTool
ISSUE_741_RESPONSE.md
[style] ~12-~12: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1976 characters long)
Context: ...Optional(2.0) | Optional(5.0) (wrong!) | | AudioSamplingRate | `Optional(22...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
ISSUE_741_RESPONSE.md
23-23: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
32-32: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
82-82: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ci-web / build-web
- GitHub Check: ci-android / build-android
- GitHub Check: ci-ios / ios-dependencies
🔇 Additional comments (6)
ISSUE_741_RESPONSE.md (1)
1-83: Excellent documentation of the bug and fix!The documentation clearly explains the root cause, provides technical analysis with code examples, and offers both automated and manual patch instructions. The static analysis formatting hints (exclamation marks, blank lines around code blocks, bare URLs) are minor stylistic issues that don't impact the clarity or usefulness of the documentation.
scripts/patch-audioset.ts (5)
1-14: Clear script header and setup.The shebang, header documentation, and imports are well-structured. The comment clearly explains the purpose and references the related issue.
17-30: Previous feedback successfully incorporated!The script now patches both
AudioSet.swiftandRecordBackType.swift, addressing the previous review's suggestion to extend the scope. The configuration is clean and easy to maintain.
55-64: Header patching correctly preserves original warning.The implementation properly preserves the "DO NOT MODIFY" warning while adding the patch notice, addressing previous review feedback. The idempotency check on line 62 ensures the header won't be patched multiple times.
71-94: Clean orchestration and reporting.The main function provides clear feedback and properly aggregates patch counts across files. The logging with emojis makes the output easy to scan.
1-96: Context: Temporary workaround for upstream code generator bug.This script patches generated bindings, which normally contradicts the practice of modifying behavior via Nitro specs rather than generated code. However, this is an acceptable temporary workaround because:
- It addresses a critical Swift/C++ bridge optimization bug in the nitrogen code generator
- The documentation (ISSUE_741_RESPONSE.md) indicates plans to file an upstream issue
- The patch is automated and applied consistently post-generation
- Once the upstream fix is merged, this script can be removed
Based on learnings: This is a pragmatic exception to the general rule.
|
@hyochan do you want me to check this out before merging? :) |
I tested myself but it'd be welcome if you do! |
|
@felixspitzer I'll wait for your review approve then! Please let me know |
|
|
This crashes for me. Is that only me? It also looks like these values are still nil - still see the LLDB error |
|
@felixspitzer Did you test with the example project from this repo? The example project already has the patched If you tested example project did you run |
|
@hyochan yes - still using the example from this repo. Did run yarn clean and yarn prepare beforehand. 
The release build still ignores my settings. It has 5 channels and 8kHz The LLDB error only happens if I run in Release mode and with "Debug executable" checked in Xcode. When I disable debug executable it does not crash there. But the settings are still ignored / nil. |
|
I don't know if that might help but I am happy to jump on a FaceTime call (or whatever) with you guys and check this. Did you run a release build with my config and did you get the expected results? @hyochan |
|
@felixspitzer Thank you for testing. I've verified the recorded audio file in release build - the AudioSet values are still not being passed correctly (got 8000Hz/1ch instead of expected 22050Hz/2ch). I've posted details in mrousavy/nitro#1065 (comment). Need further investigation. However, I've not faced the build error in release build like your screenshot. Your crash is actually the same bug with a different symptom. In our Sound.swift: When AVSampleRateKeyIOS is corrupted to nil, it falls back to AudioSamplingRate which is corrupted to -nan. Then Int(-nan) causes the Swift runtime crash. Same bug, different manifestation depending on which property gets corrupted first. |
|
@hyochan thank you for verifying. Regarding the screenshot - that only happens when you have a questionable Xcode Config. So maybe it can be ignored. 
|
|
@felixspitzer Thanks for the clarification! That makes sense - the "Debug executable" option in Xcode catches the runtime error that would otherwise silently fail. So the core issue remains: in Release builds, the std::optional values are corrupted (becoming nil, -nan, or wrong values), causing either: The bug is confirmed, just manifests differently based on debug settings. I'm not very familiar with NitroModules core, so I'll wait for mrousavy/nitro#1065 to be resolved. |
|
Waiting for swiftlang/swift#85735 |
|
This goes deeper than expected :D Thank you for making the effort 🙏 |
3 participants
Summary by CodeRabbit
Bug Fixes
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.