yl-branch-53 #1877
yl-branch-53 #1877
Conversation
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds Swift–Rust transcription bindings and public APIs (init, transcribe), switches Swift package dependencies to WhisperKit, updates Swift toolchain metadata, sets TOOLCHAINS=XcodeDefault for builds, and implements an automated macOS build step that extracts and links ArgmaxSDK.xcframework during cargo build. Changes
Sequence Diagram(s)sequenceDiagram
participant Rust as Rust API
participant FFI as SRString Interop
participant Swift as Swift library
participant Whisper as WhisperKit
participant Argmax as ArgmaxSDK
rect rgb(200, 230, 255)
note over Rust, Swift: Initialization (sync via semaphore)
Rust->>FFI: init(api_key: &str)
FFI->>FFI: convert to SRString
FFI->>Swift: initialize_am2_sdk(apiKey: SRString)
Swift->>Argmax: ArgmaxSDK.with(apiKey) (Task)
Argmax-->>Swift: setup complete
Swift-->>FFI: return (unblock semaphore)
end
rect rgb(220, 200, 255)
note over Rust, Swift: Transcription (sync via semaphore)
Rust->>FFI: transcribe(audio_path: &str)
FFI->>FFI: convert to SRString
FFI->>Swift: transcribe_audio_file(path: SRString)
Swift->>Whisper: init/load model (WhisperKitPro)
Swift->>Whisper: transcribe(path) (async)
Whisper-->>Swift: transcript/chunks
Swift->>Swift: merge results, handle errors
Swift-->>FFI: return SRString transcript (unblock semaphore)
FFI->>Rust: convert to String and return
end
sequenceDiagram
participant Cargo as cargo build
participant BuildRs as crates/am2/build.rs
participant SPM as swift package manager
participant FS as filesystem
rect rgb(240, 240, 220)
note over Cargo, BuildRs: macOS build-time xcframework extraction & linking
Cargo->>BuildRs: run build script
BuildRs->>FS: check for swift-lib/frameworks/ArgmaxSDK.xcframework
alt xcframework missing
BuildRs->>FS: create temp-resolver project
BuildRs->>SPM: run `swift package resolve`
SPM-->>BuildRs: resolved artifacts
BuildRs->>FS: locate & copy ArgmaxSDK.xcframework -> swift-lib/frameworks/
BuildRs->>FS: cleanup temp-resolver
end
BuildRs->>FS: compute link paths
BuildRs-->>Cargo: emit cargo:rustc-link-* directives
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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: 3
🧹 Nitpick comments (3)
crates/am2/src/lib.rs (1)
20-23: Consider returningResult<String, String>for better error handling.Currently, errors are embedded in the return string with an "Error:" prefix. A
Resulttype would provide more idiomatic Rust error handling and avoid callers needing to parse the string.-pub fn transcribe(audio_path: &str) -> String { +pub fn transcribe(audio_path: &str) -> Result<String, String> { let path = SRString::from(audio_path); - unsafe { transcribe_audio_file(&path).to_string() } + let result = unsafe { transcribe_audio_file(&path).to_string() }; + if result.starts_with("Error:") { + Err(result) + } else { + Ok(result) + } }crates/am2/swift-lib/src/lib.swift (2)
37-54: GlobalwhisperKitProis reassigned on every call.The global
whisperKitProvariable is overwritten with a new instance on eachtranscribe_audio_filecall (line 41), but never reused. This creates a newWhisperKitProinstance per transcription, which may be intentional but:
- The previous instance is orphaned (potential resource leak)
- If reuse is intended, initialize once and check for existing instance
Consider either removing the global variable if single-use is intended, or reusing the initialized instance:
Task { do { - let config = WhisperKitProConfig(model: "large-v3-v20240930_626MB") - let kit = try await WhisperKitPro(config) - whisperKitPro = kit + let kit: WhisperKitPro + if let existing = whisperKitPro { + kit = existing + } else { + let config = WhisperKitProConfig(model: "large-v3-v20240930_626MB") + kit = try await WhisperKitPro(config) + whisperKitPro = kit + }
39-39: Model name is hardcoded.The model
"large-v3-v20240930_626MB"is hardcoded. Consider making this configurable via parameter or environment variable if different models may be needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.cargo/config.toml(1 hunks).swift-version(0 hunks).vscode/settings.json(1 hunks)crates/am2/.gitignore(1 hunks)crates/am2/src/lib.rs(1 hunks)crates/am2/swift-lib/.gitignore(1 hunks)crates/am2/swift-lib/Package.resolved(3 hunks)crates/am2/swift-lib/Package.swift(2 hunks)crates/am2/swift-lib/src/lib.swift(1 hunks)
💤 Files with no reviewable changes (1)
- .swift-version
🧰 Additional context used
🧬 Code graph analysis (2)
crates/am2/src/lib.rs (1)
crates/am2/swift-lib/src/lib.swift (3)
initialize_am2_sdk(8-21)check_am2_ready(23-26)transcribe_audio_file(28-58)
crates/am2/swift-lib/src/lib.swift (2)
crates/analytics/src/lib.rs (1)
with(147-150)crates/am2/src/lib.rs (1)
transcribe(20-23)
⏰ 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 (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: ci (macos, depot-macos-14)
🔇 Additional comments (11)
crates/am2/swift-lib/.gitignore (1)
1-2: LGTM!Standard ignore patterns for Swift Package Manager build artifacts and binary frameworks directory.
crates/am2/.gitignore (1)
1-1: LGTM!Clean separation with nested
.gitignorehandling Swift-specific build artifacts..cargo/config.toml (1)
3-3: LGTM!Setting
TOOLCHAINSensures consistent Swift toolchain usage during Rust builds that invoke Swift compilation. This aligns with the corresponding VSCode settings update.crates/am2/swift-lib/Package.resolved (1)
44-45: Verify the swift-transformers version downgrade.The
swift-transformersdependency was downgraded from0.1.24to0.1.15. Ensure this is intentional and compatible with WhisperKit requirements..vscode/settings.json (1)
25-28: LGTM!Good alignment with
.cargo/config.tomlto ensure consistent toolchain environment between terminal and VSCode builds.crates/am2/src/lib.rs (2)
1-14: LGTM!Clean FFI wrapper with proper SRString conversion for the API key initialization.
36-52: Audio file path is correctly configured; review comment's premise is incorrect.The audio file the test references (
crates/data/src/english_1/audio.wav) exists in the repository. The relative path construction is valid:CARGO_MANIFEST_DIR(crates/am2/) +/../data/src/english_1/audio.wavcorrectly resolves to the existing file atcrates/data/src/english_1/audio.wav.The legitimate concern is the
AM_API_KEYenvironment variable dependency, which remains valid. However, presenting the audio file path as a potential missing resource is inaccurate.Likely an incorrect or invalid review comment.
crates/am2/swift-lib/src/lib.swift (2)
1-6: LGTM!Appropriate imports and global state declarations for SDK lifecycle management.
8-21: Potential deadlock with semaphore blocking pattern.Using
semaphore.wait()to block while aTaskexecutes async work can deadlock if called from a context that shares the cooperative thread pool (e.g., Swift's main actor or certain dispatch queues). This pattern works when called from a dedicated thread but is fragile.Since this is called via FFI from Rust, it's likely safe, but document this constraint or consider using a dedicated dispatch queue for the async work.
crates/am2/swift-lib/Package.swift (2)
1-1: Swift tools version downgrade from 6.0 to 5.10.This is likely intentional to avoid Swift 6's stricter concurrency checking defaults, which can cause build issues with dependencies like WhisperKit. Please confirm this was intentional and consider documenting the rationale.
25-33: Target configuration looks correct.The target properly declares dependencies on SwiftRs (for Rust interop), WhisperKit (for transcription), and the ArgmaxSDK binary target. The explicit path "src" follows the expected swift-rs convention.
| .package( | ||
| url: "https://github.com/huggingface/swift-transformers.git", | ||
| exact: "0.1.24") | ||
| .package(url: "https://github.com/argmaxinc/WhisperKit.git", exact: "0.14.1") |
There was a problem hiding this comment.
🧩 Analysis chain
Verify WhisperKit version 0.14.1 is current and stable.
Using exact version constraint ensures reproducibility but may cause dependency resolution issues if WhisperKit's transitive dependencies conflict. Consider using from: "0.14.1" if flexibility is acceptable.
🌐 Web query:
WhisperKit 0.14.1 release argmaxinc latest version
💡 Result:
WhisperKit v0.14.1 was released on Oct 17, 2025. The latest release is v0.15.0 (marked "Latest") published on Nov 7, 2025. [1]
[1] argmaxinc/WhisperKit releases on GitHub.
WhisperKit 0.14.1 is outdated; consider upgrading to 0.15.0.
WhisperKit v0.14.1 was released on Oct 17, 2025, but the latest release is v0.15.0 (published Nov 7, 2025). Using exact version pinning on 0.14.1 locks the dependency to an older release. Unless there's a specific reason to avoid 0.15.0, consider updating to the latest stable version or switching to from: "0.14.1" to allow flexibility for security patches and bug fixes.
🤖 Prompt for AI Agents
In crates/am2/swift-lib/Package.swift around line 18, the dependency is pinned
to WhisperKit exact "0.14.1" which is outdated; update the package declaration
to either use the new exact version "0.15.0" (replace exact: "0.14.1" with
exact: "0.15.0") or relax the pin to allow patch updates (replace exact:
"0.14.1" with from: "0.14.1") depending on whether you want a strict upgrade to
0.15.0 or flexible updates for future patches.
| .binaryTarget( | ||
| name: "ArgmaxSDK", | ||
| path: "frameworks/ArgmaxSDK.xcframework" | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
Verify ArgmaxSDK.xcframework exists at the specified path.
Binary targets require the xcframework to be present at build time. Ensure frameworks/ArgmaxSDK.xcframework is either committed to the repository or downloaded as part of the build process.
🏁 Script executed:
#!/bin/bash
# Description: Verify ArgmaxSDK.xcframework exists at the expected location
# Check if the xcframework directory exists
fd -t d "ArgmaxSDK.xcframework" crates/am2/swift-lib
# Also check if there's a download script or documentation for obtaining the framework
fd -e sh -e md . crates/am2/swift-lib | xargs -I {} sh -c 'echo "=== {} ===" && head -30 {}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Description: Comprehensive verification of ArgmaxSDK.xcframework and related files
echo "=== Checking directory structure ==="
ls -la crates/am2/swift-lib/ 2>/dev/null || echo "crates/am2/swift-lib does not exist"
echo -e "\n=== Searching for ArgmaxSDK.xcframework anywhere in repo ==="
find . -name "*ArgmaxSDK*" -o -name "*.xcframework" 2>/dev/null | head -20
echo -e "\n=== Checking .gitignore for binary/framework patterns ==="
cat .gitignore 2>/dev/null | grep -E "(\.xcframework|frameworks|binary|\.xcarchive)" || echo "No matching patterns in .gitignore"
echo -e "\n=== Checking Package.swift file context ==="
cat crates/am2/swift-lib/Package.swift 2>/dev/null || echo "Package.swift not found"
echo -e "\n=== Looking for build/download scripts in swift-lib ==="
find crates/am2/swift-lib -type f \( -name "*.sh" -o -name "Makefile" -o -name "*.yaml" -o -name "*.yml" \) 2>/dev/null
echo -e "\n=== Searching for ArgmaxSDK mentions in repo ==="
rg -i "ArgmaxSDK" --type swift --type text -B2 -A2 | head -40Length of output: 1548
🏁 Script executed:
#!/bin/bash
# Description: Comprehensive verification of ArgmaxSDK.xcframework and related files
echo "=== Checking directory structure ==="
ls -la crates/am2/swift-lib/ 2>/dev/null || echo "crates/am2/swift-lib does not exist"
echo -e "\n=== Searching for ArgmaxSDK.xcframework anywhere in repo ==="
find . -name "*ArgmaxSDK*" -o -name "*.xcframework" 2>/dev/null | head -20
echo -e "\n=== Checking .gitignore for binary/framework patterns ==="
cat .gitignore 2>/dev/null | grep -E "(\.xcframework|frameworks|binary|\.xcarchive)" || echo "No matching patterns in .gitignore"
echo -e "\n=== Checking Package.swift file context ==="
cat crates/am2/swift-lib/Package.swift 2>/dev/null || echo "Package.swift not found"
echo -e "\n=== Looking for build/download scripts in swift-lib ==="
find crates/am2/swift-lib -type f \( -name "*.sh" -o -name "Makefile" -o -name "*.yaml" -o -name "*.yml" \) 2>/dev/null
echo -e "\n=== Searching for ArgmaxSDK mentions in repo ==="
rg -i "ArgmaxSDK" --type swift --type text -B2 -A2 | head -40Length of output: 1548
Add frameworks directory with ArgmaxSDK.xcframework or remove the binary target dependency.
The frameworks/ArgmaxSDK.xcframework referenced in Package.swift (lines 21-24) does not exist in the repository. The frameworks/ directory is missing entirely, and no build scripts or download mechanisms are present. When developers attempt to build this package, the binary target resolution will fail. Either:
- Commit the xcframework to the repository under
frameworks/ArgmaxSDK.xcframework, or - Remove the binaryTarget from Package.swift and the dependency from the swift-lib target
🤖 Prompt for AI Agents
In crates/am2/swift-lib/Package.swift around lines 21 to 24, Package.swift
declares a binaryTarget pointing to frameworks/ArgmaxSDK.xcframework which is
missing from the repo; fix by either adding the missing
frameworks/ArgmaxSDK.xcframework into the repository (create the frameworks/
directory, commit the ArgmaxSDK.xcframework bundle, and verify the path and
xcframework structure) or remove the binaryTarget entry and any references to
"ArgmaxSDK" from the package's target dependencies and targets list so the
package no longer expects that binary; ensure Package.swift and any swift-lib
target dependency lists are updated accordingly and run swift build to verify
resolution succeeds.
| Task { | ||
| await ArgmaxSDK.with(ArgmaxConfig(apiKey: key)) | ||
| isAM2Ready = true | ||
| print("AM2 SDK initialized successfully with API key: \(key.prefix(10))...") |
There was a problem hiding this comment.
Avoid logging API key prefixes.
Logging even the first 10 characters of an API key could expose sensitive information in logs. Consider removing or masking this entirely.
- print("AM2 SDK initialized successfully with API key: \(key.prefix(10))...")
+ print("AM2 SDK initialized successfully")📝 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.
| print("AM2 SDK initialized successfully with API key: \(key.prefix(10))...") | |
| print("AM2 SDK initialized successfully") |
🤖 Prompt for AI Agents
In crates/am2/swift-lib/src/lib.swift around line 16, the initialization log
prints the first 10 characters of the API key which risks leaking sensitive
data; remove the key prefix from the log and replace it with a non-sensitive
message (e.g., "AM2 SDK initialized successfully") or a fully masked token
(e.g., "<redacted>") so no part of the API key is written to logs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/am2/build.rs (3)
56-67: Consider using Rust's filesystem APIs instead of externalcpcommand.While
cp -Rworks on macOS, using Rust-native filesystem operations would be more robust. Thefs_extracrate or a recursive copy function would avoid relying on external commands.Additionally,
.to_str().unwrap()on lines 59-60 will panic if paths contain non-UTF8 characters.- let status = Command::new("cp") - .args([ - "-R", - artifacts_path.to_str().unwrap(), - xcframework_dest.to_str().unwrap(), - ]) - .status() - .expect("Failed to copy xcframework"); - - if !status.success() { - panic!("Failed to copy ArgmaxSDK.xcframework"); - } + fn copy_dir_recursive(src: &std::path::Path, dst: &std::path::Path) -> std::io::Result<()> { + std::fs::create_dir_all(dst)?; + for entry in std::fs::read_dir(src)? { + let entry = entry?; + let ty = entry.file_type()?; + let dst_path = dst.join(entry.file_name()); + if ty.is_dir() { + copy_dir_recursive(&entry.path(), &dst_path)?; + } else { + std::fs::copy(entry.path(), dst_path)?; + } + } + Ok(()) + } + + copy_dir_recursive(&artifacts_path, &xcframework_dest) + .expect("Failed to copy ArgmaxSDK.xcframework");
1-11: Addcargo:rerun-if-changeddirectives for proper incremental builds.The build script lacks
cargo:rerun-if-changeddirectives. Without them, Cargo may not re-run the build script when Swift source files or the Package.swift change, leading to stale builds.Add after the path setup (around line 11):
println!("cargo:rerun-if-changed=swift-lib/"); println!("cargo:rerun-if-changed=swift-lib/Package.swift"); println!("cargo:rerun-if-changed=swift-lib/src/lib.swift");
74-79: Add a comment explaining the workspace-state.json removal.This cleanup step would benefit from a brief comment explaining why it's necessary, helping future maintainers understand the workaround.
let out_dir = std::env::var("OUT_DIR").unwrap(); let swift_build_dir = Path::new(&out_dir).join("swift-rs/swift-lib"); let workspace_state = swift_build_dir.join("workspace-state.json"); + // Remove stale workspace state to force swift-rs to re-resolve packages if workspace_state.exists() { std::fs::remove_file(&workspace_state).ok(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/am2/build.rs(1 hunks)
⏰ 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 (linux, depot-ubuntu-24.04-8)
- GitHub Check: ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: fmt
- GitHub Check: ci (macos, depot-macos-14)
| let framework_path = xcframework_dest.join("macos-arm64"); | ||
| println!( | ||
| "cargo:rustc-link-search=framework={}", | ||
| framework_path.display() | ||
| ); | ||
| println!("cargo:rustc-link-lib=framework=ArgmaxSDK"); | ||
| println!( | ||
| "cargo:rustc-link-arg=-Wl,-rpath,{}", | ||
| framework_path.display() | ||
| ); |
There was a problem hiding this comment.
Hardcoded macos-arm64 breaks Intel Mac builds.
The xcframework path hardcodes macos-arm64, which will fail on Intel-based Macs. XCFrameworks contain architecture-specific slices that must be selected based on the build target.
- let framework_path = xcframework_dest.join("macos-arm64");
+ let arch = std::env::var("CARGO_CFG_TARGET_ARCH").unwrap_or_else(|_| "arm64".to_string());
+ let framework_slice = match arch.as_str() {
+ "x86_64" => "macos-x86_64",
+ "aarch64" | "arm64" => "macos-arm64",
+ _ => panic!("Unsupported architecture: {}", arch),
+ };
+ let framework_path = xcframework_dest.join(framework_slice);📝 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.
| let framework_path = xcframework_dest.join("macos-arm64"); | |
| println!( | |
| "cargo:rustc-link-search=framework={}", | |
| framework_path.display() | |
| ); | |
| println!("cargo:rustc-link-lib=framework=ArgmaxSDK"); | |
| println!( | |
| "cargo:rustc-link-arg=-Wl,-rpath,{}", | |
| framework_path.display() | |
| ); | |
| let arch = std::env::var("CARGO_CFG_TARGET_ARCH").unwrap_or_else(|_| "arm64".to_string()); | |
| let framework_slice = match arch.as_str() { | |
| "x86_64" => "macos-x86_64", | |
| "aarch64" | "arm64" => "macos-arm64", | |
| _ => panic!("Unsupported architecture: {}", arch), | |
| }; | |
| let framework_path = xcframework_dest.join(framework_slice); | |
| println!( | |
| "cargo:rustc-link-search=framework={}", | |
| framework_path.display() | |
| ); | |
| println!("cargo:rustc-link-lib=framework=ArgmaxSDK"); | |
| println!( | |
| "cargo:rustc-link-arg=-Wl,-rpath,{}", | |
| framework_path.display() | |
| ); |
🤖 Prompt for AI Agents
In crates/am2/build.rs around lines 85 to 94, the code hardcodes "macos-arm64"
which breaks Intel Mac builds; change it to select the correct xcframework slice
based on the build TARGET environment variable (std::env::var("TARGET")) —
detect whether the target contains "aarch64" (use "macos-arm64") or otherwise
use "macos-x86_64", build framework_path = xcframework_dest.join(slice), and
then use that framework_path for cargo:rustc-link-search, cargo:rustc-link-arg
rpath, and the link-lib statement so the correct architecture slice is used at
build time.
No description provided.