Maple 3.0: Native Rust/SwiftUI/Compose/iced app#462
Maple 3.0: Native Rust/SwiftUI/Compose/iced app#462AnthonyRonning wants to merge 9 commits intomasterfrom
Conversation
Introduces native/ directory alongside existing frontend/ (2.0 Tauri). All bundle IDs aligned to cloud.opensecret.maple for App Store continuity. SDK dependency switched from submodule to crates.io opensecret 3.0.0-alpha.0. Version set to 3.0.0 across all platforms. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full "native" Maple 3.0 platform: a Rust FFI core with UniFFI bindings, Android/iOS/desktop frontends and UIs, an rmp-cli for binding/build/run workflows, Nix dev shell and Justfile recipes, platform projects/resources, and CI packaging/build scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Native UI
participant AM as AppManager (platform)
participant Rust as Rust Core (FfiApp)
participant API as OpenSecret API
participant KS as Secure Storage
rect rgba(100,150,200,0.5)
UI->>AM: initialize / getInstance()
AM->>KS: load persisted tokens
AM->>Rust: FfiApp::new(api_url, client_id)
Rust->>API: initialize client
AM->>Rust: listen_for_updates(self)
Rust-->>AM: AppUpdate events
AM-->>UI: update state
end
rect rgba(150,100,200,0.5)
UI->>AM: dispatch(LoginWithEmail)
AM->>Rust: dispatch(LoginWithEmail)
Rust->>API: auth.login_with_email()
API-->>Rust: tokens
Rust-->>AM: AppUpdate::SessionTokens
AM->>KS: persist tokens
AM-->>UI: state becomes LoggedIn
end
rect rgba(200,150,100,0.5)
UI->>AM: dispatch(SendMessage)
AM->>Rust: dispatch(SendMessage)
Rust->>API: send message / receive stream
API-->>Rust: AgentMessageChunk (stream)
Rust-->>AM: AppUpdate::FullState (streaming updates)
AM-->>UI: update messages / typing indicators
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
native/rust/src/lib.rs
Outdated
| Ok(opensecret::types::AgentSseEvent::Done(_)) => { | ||
| InternalEvent::AgentDone | ||
| } | ||
| Ok(opensecret::types::AgentSseEvent::Error(err)) => { | ||
| InternalEvent::AgentError(err.error) | ||
| } | ||
| Err(e) => { | ||
| InternalEvent::AgentError(e.to_string()) | ||
| } | ||
| }; | ||
| let _ = tx.send(CoreMsg::Internal(Box::new(internal))); | ||
| } | ||
| // If stream ends without a Done event, finalize | ||
| let _ = tx.send(CoreMsg::Internal(Box::new(InternalEvent::AgentDone))); |
There was a problem hiding this comment.
🟡 Duplicate AgentDone event sent unconditionally after SSE stream ends
In the agent chat streaming task at native/rust/src/lib.rs:866-869, an InternalEvent::AgentDone is sent unconditionally after the while let loop over the SSE stream finishes. However, when the stream contains a Done SSE event (line 856-857), AgentDone is already sent inside the loop. When the stream then ends naturally, a second AgentDone is sent. This causes a redundant state emission cycle (rev increment + full state clone + FFI callback), and could confuse platform-side logic that tracks typing state transitions.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (24)
native/rmp-cli/README.md-7-9 (1)
7-9:⚠️ Potential issue | 🟠 MajorThe referenced documentation files are missing from the repository.
The README references three documentation files that do not exist:
docs/rmp.mddocs/rmp-ci.mddocs/architecture.mdEither include these documentation files in the PR or remove the references from the README.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rmp-cli/README.md` around lines 7 - 9, The README references missing documentation files—update the PR so the links resolve: either add the three files (`docs/rmp.md`, `docs/rmp-ci.md`, `docs/architecture.md`) with appropriate content and commit them, or remove/update the three link entries in native/rmp-cli/README.md that point to those filenames; ensure the README no longer contains broken links to `docs/rmp.md`, `docs/rmp-ci.md`, or `docs/architecture.md`.native/android/app/src/main/AndroidManifest.xml-6-12 (1)
6-12:⚠️ Potential issue | 🟠 MajorDisable Android backups for persisted chat data, or add explicit exclusions.
This app persists sessions/settings, so
android:allowBackup="true"opts that data into device or cloud backup unless backup rules exclude it. For a chat client, that is an avoidable privacy leak.Suggested change
- android:allowBackup="true" + android:allowBackup="false"If you need backup for non-sensitive preferences, add
fullBackupContent/dataExtractionRulesand explicitly exclude conversations, session state, and tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/android/app/src/main/AndroidManifest.xml` around lines 6 - 12, The AndroidManifest currently sets android:allowBackup="true" on the <application> tag which allows app data (including persisted chat sessions/settings) to be backed up; change this to disable backups or add explicit backup rules: either set android:allowBackup="false" on the <application> element, or keep allowBackup but add android:fullBackupContent or android:dataExtractionRules pointing to an XML resource that explicitly excludes conversation storage, session tokens, and other sensitive files; update/create the corresponding full_backup_content or data_extraction_rules XML to list exclusions (e.g., exclude databases, prefs holding tokens/conversations).native/.gitignore-15-17 (1)
15-17:⚠️ Potential issue | 🟠 MajorDon’t ignore the Gradle wrapper.
For a new Android app,
gradlew*andgradle/wrapper/*are what pin the Gradle distribution for every contributor and CI job. Keeping them ignored makes native Android builds depend on local setup.Suggested change
-android/gradle/ -android/gradlew -android/gradlew.bat🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/.gitignore` around lines 15 - 17, Remove the Gradle wrapper from being ignored: delete the entries that ignore gradlew and the android/gradle/ directory in .gitignore, and instead ensure the Gradle wrapper files (gradlew, gradlew.bat, and the gradle/wrapper/* files such as gradle-wrapper.properties and gradle-wrapper.jar) are tracked in git so the project pins the Gradle distribution for all contributors and CI; update .gitignore to only ignore build artifacts, not the wrapper files.native/android/build.gradle.kts-1-4 (1)
1-4:⚠️ Potential issue | 🟠 MajorUpdate to latest stable plugin versions.
The code uses AGP 8.5.1 and Kotlin 1.9.24, which are outdated. As of March 2026, the latest stable versions are AGP 9.1.0 (released this month) and Kotlin 2.3.10 (released February 2026). Update both plugins to maintain compatibility with current tooling and benefit from recent fixes and features:
plugins { id("com.android.application") version "9.1.0" apply false id("org.jetbrains.kotlin.android") version "2.3.10" apply false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/android/build.gradle.kts` around lines 1 - 4, Update the plugin version numbers in the plugins block: change the com.android.application plugin version used with id("com.android.application") to "9.1.0" and change the Kotlin Android plugin version used with id("org.jetbrains.kotlin.android") to "2.3.10"; ensure both apply false remain unchanged and run a Gradle sync/build to verify compatibility after the update.native/ios/Sources/ContentView.swift-122-127 (1)
122-127:⚠️ Potential issue | 🟠 MajorGuard
submit()against repeat keyboard submits.The button is disabled while
isLoading, but Line 127 still callssubmit()from the password field. Repeated Return presses can dispatch duplicate auth requests unlesssubmit()enforces the same guard itself.🔐 Keep the handler consistent with the button state
private func submit() { + guard !isLoading, !email.isEmpty, !password.isEmpty else { return } if isSignUp { manager.dispatch(.signUpWithEmail(email: email, password: password, name: name)) } else {Also applies to: 134-145, 200-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/ios/Sources/ContentView.swift` around lines 122 - 127, The submit() handler must early-return when an auth request is already in progress; add a guard at the top of submit() that checks the same isLoading flag used to disable the button and returns immediately if true, so keyboard Return presses cannot dispatch duplicate requests. Update the submit() method referenced by the SecureField .onSubmit(submit) (and other .onSubmit usages) to atomically set isLoading before starting async work and clear it on all exit paths (success, failure, cancellation) to match the button state.native/rmp-cli/src/init.rs-593-653 (1)
593-653:⚠️ Potential issue | 🟠 MajorThe generated iOS recipes assume Xcode lives at one fixed path.
Hard-coding
/Applications/Xcode.app/Contents/Developerbreaksjust ios-*on common setups likeXcode-beta.app, renamed installs, orxcode-select/DEVELOPER_DIRoverrides. The scaffold should discover or inherit the active developer dir instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rmp-cli/src/init.rs` around lines 593 - 653, The template hard-codes XCODE_DEV and TOOLCHAIN_BIN; change it to detect or inherit the active developer dir (use DEVELOPER_DIR if set, otherwise fall back to xcode-select -p) and derive TOOLCHAIN_BIN from that so ios-rust, ios-build, ios-xcframework and ios-full work for Xcode-beta/renamed installs and xcode-select overrides; update the XCODE_DEV/TOOLCHAIN_BIN definitions in the generated string (and any uses like DEVELOPER_DIR="{{XCODE_DEV}}" and SDK paths) to compute the path dynamically from DEVELOPER_DIR or xcode-select.native/ios/Sources/ContentView.swift-332-347 (1)
332-347:⚠️ Potential issue | 🟠 MajorLoading older messages snaps the list back to the bottom.
Pagination from Line 333 and Line 347 prepends older history, but this hook still sees a higher message count and scrolls to
last.id. That makes upward history browsing effectively unusable.🧭 Auto-scroll on tail changes, not any count increase
- .onChange(of: manager.state.messages.count) { oldCount, newCount in - if newCount > oldCount { - if manager.state.isAgentTyping { - withAnimation { proxy.scrollTo("typing-indicator", anchor: .bottom) } - } else if let last = manager.state.messages.last { - withAnimation { proxy.scrollTo(last.id, anchor: .bottom) } - } - } + .onChange(of: manager.state.messages.last?.id) { oldLastId, newLastId in + guard newLastId != oldLastId else { return } + if manager.state.isAgentTyping { + withAnimation { proxy.scrollTo("typing-indicator", anchor: .bottom) } + } else if let last = manager.state.messages.last { + withAnimation { proxy.scrollTo(last.id, anchor: .bottom) } + } }Also applies to: 365-380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/ios/Sources/ContentView.swift` around lines 332 - 347, The list is auto-scrolling whenever the message array length changes (including when older messages are prepended), so stop treating any count increase as a tail append: only trigger the scroll-to-tail behavior when the last message at the end actually changes. Locate the logic that scrolls to last.id (the code observing manager.state.messages and calling scrollTo(last.id)), and change it to compare the previous last message id with the new last message id (or track a boolean like isLoadingHistory) and only call scrollTo(last.id) when the last message id differs (i.e., new messages appended at the tail), not when older messages are loaded via manager.dispatch(.loadOlderMessages) / the prefetched branch in MessageBubble onAppear.native/rmp-cli/src/util.rs-33-65 (1)
33-65:⚠️ Potential issue | 🟠 MajorHonor
xcode-select -pbefore scanning/Applications.Many valid setups point
xcode-selectatXcode-beta.appor a non-default install location. This helper skips that source entirely, sodoctorandruncan fail even though/usr/bin/xcrunwould work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rmp-cli/src/util.rs` around lines 33 - 65, discover_xcode_dev_dir currently ignores the system xcode-select setting; update it to call xcode-select -p (e.g., via std::process::Command) and, if that returns a valid path, construct a PathBuf pointing at its Developer directory and validate it the same way as DEVELOPER_DIR by checking for usr/bin/xcrun or usr/bin/simctl and by calling developer_dir_supports_iphoneos(&path); only fall back to scanning /Applications if xcode-select either fails or yields a path that does not pass those checks. Ensure you reuse the existing developer_dir_supports_iphoneos function and the same validation logic used for DEVELOPER_DIR to keep behavior consistent.native/desktop/iced/src/main.rs-334-362 (1)
334-362:⚠️ Potential issue | 🟠 MajorMirror the disabled-state checks in
update().Line 618 and Line 793 still submit on Enter, but
Message::SubmitAuthandMessage::SendMessageonly trust the button state. That lets keyboard submit send duplicate auth requests or another chat turn whilestate.is_agent_typingis true.🛡️ Re-check the same guards in the handler
Message::SubmitAuth => { + if matches!(state.auth, AuthState::LoggingIn | AuthState::SigningUp) { + return Task::none(); + } if let ScreenState::Login(s) = screen { if s.is_sign_up { manager.dispatch(AppAction::SignUpWithEmail { @@ Message::SendMessage => { if let ScreenState::Chat { compose } = screen { - if !compose.trim().is_empty() { + if !state.is_agent_typing && !compose.trim().is_empty() { manager.dispatch(AppAction::SendMessage { content: compose.clone() }); *compose = String::new(); } } }Also applies to: 615-633, 782-795
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/desktop/iced/src/main.rs` around lines 334 - 362, The handlers for Message::SubmitAuth and Message::SendMessage must mirror the UI disabled-state checks currently used for the buttons: before calling manager.dispatch in the Message::SubmitAuth branch (when matching ScreenState::Login(s)), check the same "disabled" condition you use for the submit button (e.g., s.is_disabled or validation flags) and early-return if disabled; similarly, in Message::SendMessage (when matching ScreenState::Chat { compose }), ensure you check the agent-typing guard (e.g., state.is_agent_typing or the same flag used to disable the send button) and that compose.trim().is_empty() is still checked, and only call manager.dispatch(AppAction::SendMessage { .. }) when those guards allow it; update both branches where manager.dispatch is invoked so keyboard Enter submits cannot bypass the button-enabled logic.native/rmp-cli/src/doctor.rs-41-63 (1)
41-63:⚠️ Potential issue | 🟠 MajorDo not hard-fail iOS checks on non-macOS hosts.
init()enables iOS by default, so a fresh scaffold on Linux/Windows will failrmp doctorhere trying to discover Xcode under/Applications. That blocks health checks for projects that can still build Android or desktop on those hosts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rmp-cli/src/doctor.rs` around lines 41 - 63, The iOS check currently hard-fails by calling discover_xcode_dev_dir() and running xcrun on non-macOS hosts; wrap the whole iOS block with a runtime host check (e.g. use cfg!(target_os = "macos") or std::env::consts::OS == "macos") so that if the host is not macOS you skip the Xcode/simctl checks (or return a non-fatal warning) instead of returning CliError; specifically modify the block guarded by cfg.ios.is_some() to first verify the host is macOS before calling discover_xcode_dev_dir(), run_capture, or setting xcode_developer_dir, and ensure discover_xcode_dev_dir() errors are treated as non-fatal on non-macOS hosts.native/rmp-cli/src/init.rs-63-64 (1)
63-64:⚠️ Potential issue | 🟠 MajorValidate Android
app_idwith package-name rules, not bundle-id rules.The
validate_bundle_likefunction accepts hyphens in package segments, but the generated Android templates emitapp_iddirectly intonamespaceandpackagedeclarations. Kotlin package names and Android'snamespacevalue require valid Java identifiers—hyphens are not permitted. Anapp_idlikecom.my-app.foowill pass validation but produce uncompilable Kotlin sources.Use Java identifier rules (alphanumeric,
_,$; no hyphens) for Android package validation, or transform hyphens to underscores before usingapp_idin Android templates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rmp-cli/src/init.rs` around lines 63 - 64, The current flow uses validate_bundle_like (which allows hyphens) and then writes app_id into Kotlin/Android namespace/package via kotlin_pkg and kotlin_pkg_path, producing invalid Java/Kotlin identifiers; either update validation to enforce Java identifier rules (no hyphens, only letters/digits/underscore/$ per segment) or, if you want to accept hyphens in input, normalize app_id before templating by converting hyphens to underscores (e.g., replace '-' with '_' when computing kotlin_pkg / kotlin_pkg_path) so the values used in namespace/package and Kotlin sources are valid identifiers; apply the change where app_id is read/validated and where kotlin_pkg/kotlin_pkg_path are derived.native/ios/Sources/AppManager.swift-10-21 (1)
10-21:⚠️ Potential issue | 🟠 MajorHandle keychain write/delete failures instead of dropping them.
SecItemAddandSecItemDeletereturnOSStatuscodes, but both calls ignore them. This causes silent failures—if Keychain access fails, login appears to work and session restore simply stops working on next launch. Apple's own documentation recommends checking these return values.Affects both
save()(lines 10–21) anddelete()(lines 39–45).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/ios/Sources/AppManager.swift` around lines 10 - 21, In save(key:value:) and delete(key:) capture the OSStatus return values from SecItemAdd and SecItemDelete instead of ignoring them, check for non-success statuses, and handle failures (e.g., log the error with the status code and a clear message or propagate/throw an error) so Keychain write/delete failures aren’t silent; specifically modify the SecItemDelete(query as CFDictionary) and SecItemAdd(item as CFDictionary, nil) calls to assign their results to variables, compare against errSecSuccess, and handle/log non-success using your existing logging/error propagation patterns.native/rmp-cli/src/devices.rs-233-239 (1)
233-239:⚠️ Potential issue | 🟠 Major
devices listshould still work when Xcode is unavailable.iOS discovery is unconditional here, so Linux/Windows hosts—or Macs without Xcode—fail before
adbenumeration runs. That makes Android-only setups unusable even thoughandroid_targets()is independent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rmp-cli/src/devices.rs` around lines 233 - 239, The devices_list function currently fails early because discover_xcode_dev_dir() is called unconditionally; change the code to attempt Xcode/device discovery only if discover_xcode_dev_dir() succeeds and otherwise continue to enumerate Android targets. Specifically, call discover_xcode_dev_dir() and match on its Result (or convert to Option) so that ios_connected_devices(&dev_dir) and ios_simulators(&dev_dir) are only invoked on Ok(dev_dir); on Err just skip/empty the iOS lists (optionally log/debug the skip) and then call android_targets() as before so Android-only environments still work. Ensure you update any variable bindings (ios_devs, ios_sims) accordingly to valid empty defaults when Xcode is unavailable.native/flake.nix-97-103 (1)
97-103:⚠️ Potential issue | 🟠 MajorResolve the native workspace before using relative paths in
shellHook.This assumes the shell was entered from the
native/directory. Withnix develop ./nativefrom the repo root,RMP_REPOstays unset andandroid/local.propertiesis created in the wrong place. Resolve the native workspace first, then write through that absolute path.💡 Suggested hardening
- # Auto-detect rmp-cli in parent directory - if [ -z "''${RMP_REPO:-}" ]; then - _parent="$(cd .. 2>/dev/null && pwd)" - if [ -f "$_parent/rmp-cli/Cargo.toml" ]; then - export RMP_REPO="$_parent" - fi - fi + # Resolve the writable native workspace independently of the entry cwd. + if [ -z "''${RMP_REPO:-}" ]; then + _repo_root="$(git rev-parse --show-toplevel 2>/dev/null || true)" + if [ -n "$_repo_root" ] && [ -f "$_repo_root/native/Cargo.toml" ] && [ -d "$_repo_root/native/rmp-cli" ]; then + export RMP_REPO="$_repo_root/native" + elif [ -f "$PWD/Cargo.toml" ] && [ -d "$PWD/rmp-cli" ]; then + export RMP_REPO="$PWD" + fi + fi @@ - mkdir -p android - cat > android/local.properties <<EOF - sdk.dir=$ANDROID_HOME -EOF + if [ -n "''${RMP_REPO:-}" ]; then + mkdir -p "$RMP_REPO/android" + cat > "$RMP_REPO/android/local.properties" <<EOF + sdk.dir=$ANDROID_HOME +EOF + fiAlso applies to: 121-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/flake.nix` around lines 97 - 103, The shellHook currently uses a relative parent lookup to set RMP_REPO which fails when nix develop is run from the repo root; replace the relative lookup with code that resolves the absolute path of the native workspace (compute the repository root first and then resolve/realpath the native directory) and export that absolute path into RMP_REPO before any writes to android/local.properties in shellHook; make the same change to the other occurrence mentioned (the block around the second if at lines 121-124) so both use the resolved native workspace instead of a relative parent path.native/android/app/src/main/java/cloud/opensecret/maple/ui/MainApp.kt-359-367 (1)
359-367:⚠️ Potential issue | 🟠 MajorGate auto-scroll on whether the user is already near the bottom.
This effect fires for any message-list growth, so loading older pages or scrolling up while the last assistant message is streaming immediately snaps the list back to the bottom. That makes pagination and reading history effectively unusable.
💡 Suggested guard
+ val isNearBottom by remember { + derivedStateOf { + val lastVisible = listState.layoutInfo.visibleItemsInfo.lastOrNull()?.index ?: -1 + lastVisible >= listState.layoutInfo.totalItemsCount - 3 + } + } + var prevMessageCount by remember { mutableIntStateOf(state.messages.size) } LaunchedEffect(state.messages.size, state.messages.lastOrNull()?.content, state.isAgentTyping) { val totalItems = listState.layoutInfo.totalItemsCount - if (totalItems > 0 && state.messages.size >= prevMessageCount) { + if (isNearBottom && totalItems > 0 && state.messages.size >= prevMessageCount) { listState.animateScrollToItem(totalItems - 1) } prevMessageCount = state.messages.size }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/android/app/src/main/java/cloud/opensecret/maple/ui/MainApp.kt` around lines 359 - 367, The auto-scroll LaunchedEffect should only run when the user is already near the bottom; modify the effect that uses prevMessageCount, LaunchedEffect, and listState.animateScrollToItem so it checks the current last visible item before animating. Specifically, inside the LaunchedEffect that watches state.messages.size/state.isAgentTyping, get the last visible index from listState.layoutInfo.visibleItemsInfo (or compute from listState.firstVisibleItemIndex + visibleCount) and only call listState.animateScrollToItem(totalItems - 1) if totalItems > 0 AND state.messages.size >= prevMessageCount AND (totalItems - 1 - lastVisible) <= N (e.g., N = 1 or 2) to treat “near bottom” as true; still update prevMessageCount = state.messages.size after the guard. This keeps existing symbols (prevMessageCount, LaunchedEffect, listState.animateScrollToItem, state.messages, state.isAgentTyping) but prevents jumping when the user is reading history or paginating.native/android/app/src/main/java/cloud/opensecret/maple/AppManager.kt-84-101 (1)
84-101:⚠️ Potential issue | 🟠 MajorAlign
SessionTokenshandling with the architecture bible's safe pattern for side-effect updates.The code persists credentials correctly before checking the revision, but then omits the early return guard that the bible prescribes. Line 94 should check
if (update.rev <= lastRevApplied) return@postafter persistence, matching the pattern shown in Bible 6.6 for side-effect variants likeAccountCreated.Current pattern (unsafe assumption):
// persist if (update.rev > lastRevApplied) { lastRevApplied = update.rev }Safe pattern (per bible):
// persist if (update.rev <= lastRevApplied) return@post lastRevApplied = update.revWhile the current producer increments
revindependently for each event (soSessionTokensandFullStatenever share a revision), the code should be defensive. If the producer's semantics change or ifSessionTokensandFullStateever share a revision from the same logical transition, the current code would silently drop the full-state update and leave Compose on stale auth/router state. Align Android with the documented safe pattern used on iOS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/android/app/src/main/java/cloud/opensecret/maple/AppManager.kt` around lines 84 - 101, The SessionTokens branch persists credentials to securePrefs but lacks the early-return rev guard; change the handler for AppUpdate.SessionTokens so that after persisting (in the block handling update.accessToken) you check if (update.rev <= lastRevApplied) return@post and only then set lastRevApplied = update.rev, mirroring the safe pattern used in AppUpdate.FullState; update the logic around lastRevApplied, AppUpdate.SessionTokens, update.rev and the return@post guard to be defensive against replayed/duplicate revisions.native/rust/src/lib.rs-305-312 (1)
305-312:⚠️ Potential issue | 🟠 MajorThe core loop never sees channel shutdown.
core_tx_asyncstays alive inside the worker thread for the lifetime of the loop. After the lastFfiApphandle is dropped,self.core_txis gone butcore_rx.recv()at Line 391 still blocks forever, so the Tokio runtime and update pipeline leak. Add an explicit shutdown message inDrop, or drop the long-lived sender and keep only short-lived clones for spawned tasks.Also applies to: 391-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rust/src/lib.rs` around lines 305 - 312, The core loop blocks forever because a long-lived sender (core_tx_async / self.core_tx) stays alive preventing core_rx.recv() from returning; modify FfiApp so that Drop either sends an explicit shutdown message into core_tx (a dedicated CoreMsg::Shutdown) or ensure the persistent sender is removed and only short-lived clones are created for tasks (so when all FfiApp handles are dropped receivers observe closed channel), update the core loop to handle the shutdown variant and exit cleanly; search for core_tx_async, core_rx.recv(), self.core_tx, and the FfiApp Drop impl to make these changes.native/rust/src/lib.rs-313-325 (1)
313-325:⚠️ Potential issue | 🟠 MajorDon't panic on bad bootstrap config.
These
expects kill the core thread on an invalidclient_idorapi_urlbefore it can emit any recoverable state. Becausedispatch()later ignores send failures, the app just silently stops responding. Return an error fromnewor translate bootstrap failures into an initial error state instead.Also applies to: 884-886
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rust/src/lib.rs` around lines 313 - 325, Replace the panics in the bootstrap path by returning an error or setting an initial error state instead of calling expect: change the tokio runtime creation, uuid::Uuid::parse_str(&client_id), and opensecret::OpenSecretClient::new(&api_url) calls (seen around rt, client_uuid, and os_client) to propagate failures (e.g., return Result::Err from new) or map them into an initial error message that the rest of the system can handle (so dispatch() won’t silently stop); ensure the constructor/new function signature returns a Result or stores a recoverable error state and use ? or map_err to forward detailed errors rather than panicking.native/rmp-cli/src/bindings.rs-233-253 (1)
233-253:⚠️ Potential issue | 🟠 MajorHash the generator, not just the Rust inputs.
The cache key only covers
rust/src,uniffi.toml, and a few manifests, but generation is executed viacargo run -p uniffi-bindgen. Source edits under the localuniffi-bindgencrate can change emitted Swift/Kotlin while this hash still returns a hit, so stale bindings get reused. Include the generator crate sources/manifests in the cache inputs or key off the built binary fingerprint.Also applies to: 281-302, 905-943
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rmp-cli/src/bindings.rs` around lines 233 - 253, The cache key currently omits the uniffi-bindgen generator sources so edits under the local uniffi-bindgen crate produce stale hits; update the cache input computation to include the generator crate (e.g., its Cargo.toml and src tree) or, better, key off the actual built generator binary fingerprint. Concretely, when computing the hash used before running Command::new("cargo") ... arg("run") ... arg("-p").arg("uniffi-bindgen"), include the uniffi-bindgen crate files/manifests or the built artifact's checksum (target/.../uniffi-bindgen) in the inputs (also apply same change to the other occurrences noted around the other blocks at 281-302 and 905-943) so generator changes invalidate the cache.native/rmp-cli/src/bindings.rs-82-161 (1)
82-161:⚠️ Potential issue | 🟠 MajorKeep
--jsonstdout clean.
native/rmp-cli/src/cli.rs, Lines 294-296 prints the success payload to stdout, but this flow still delegates to helpers that inherit child stdout. Any output fromcargo,uniffi-bindgen, orxcodebuildwill corrupt the JSON stream for callers. Pass thejsonflag through and redirect child stdout to stderr/capture when JSON mode is enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rmp-cli/src/bindings.rs` around lines 82 - 161, The bindings function currently ignores the json flag when invoking child-work helpers, so external tool stdout (cargo/uniffi/xcodebuild) can mix into the JSON output; update each helper invocation from bindings (clean_ios, check_ios_sources, generate_ios_sources, build_ios_xcframework, clean_android, check_android_sources, generate_android_sources, build_android_so) to accept and receive the json boolean, and change those helper implementations to, when json == true, redirect or capture child process stdout (write child stdout to stderr or suppress it) so only the structured JSON from json_print is emitted to stdout; ensure function signatures (e.g., generate_ios_sources(...), build_ios_xcframework(...), generate_android_sources(...), build_android_so(...)) are adjusted consistently to take the json flag and branch their child-process spawning to use stderr/capture in JSON mode.native/rmp-cli/src/bindings.rs-769-773 (1)
769-773:⚠️ Potential issue | 🟠 MajorAvoid deleting the entire shared
jniLibstree.
android/app/src/main/jniLibsis the shared packaging location for all native libraries, not just Maple's generated.sos. Removing the whole directory here will silently wipe unrelated JNI dependencies the app may already ship. Delete only Maple's generated slices, or write into a dedicated generated location.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rmp-cli/src/bindings.rs` around lines 769 - 773, The code currently removes the entire shared jniLibs tree via std::fs::remove_dir_all(&out_dir) which can wipe unrelated JNI artifacts; change the strategy to either (A) write Maple outputs into a dedicated subdirectory (e.g., set out_dir = root.join("android/app/src/main/jniLibs/maple")) and call remove_dir_all/create_dir_all on that subdirectory only, or (B) if keeping the top-level jniLibs, replace the blanket std::fs::remove_dir_all(&out_dir) with logic that iterates the entries in out_dir and only deletes Maple-generated files/directories (detect by a stable marker such as a "maple" subfolder name or filename pattern like libmaple*.so) before calling create_dir_all; update references to out_dir and the std::fs::remove_dir_all/std::fs::create_dir_all calls accordingly.native/rust/src/lib.rs-494-499 (1)
494-499:⚠️ Potential issue | 🟠 MajorRemove raw conversation content from stderr logs.
Line 498 prints every streamed agent chunk, including message text. That leaks conversation content into logcat/console output and is hard to scrub later. Drop this log or reduce it to non-content metadata behind a debug-only path.
🔇 Minimal safe change
- eprintln!("[agent] step={step} messages={messages:?}");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rust/src/lib.rs` around lines 494 - 499, The eprintln! in the InternalEvent::AgentMessageChunk arm currently prints raw message content (messages) and must be removed or silenced; update the handler for InternalEvent::AgentMessageChunk to stop logging message text — either delete the eprintln! call or replace it with a debug-only log that emits only non-sensitive metadata (e.g., step or a boolean has_messages) and does not include messages' contents, ensuring you modify the match arm handling InternalEvent::AgentMessageChunk (look for variables step and messages) accordingly.native/rust/src/lib.rs-458-464 (1)
458-464:⚠️ Potential issue | 🟠 MajorEmit a token-clear update on restore failure.
native/android/app/src/main/java/cloud/opensecret/maple/AppManager.kt, Lines 80-104 only removes persisted credentials when it receivesAppUpdate.SessionTokenswith empty values. This branch only routes back to Login, so invalid tokens stay on disk and the app will retry them on every launch. Mirror the logout path here.🧹 Mirror the logout-side credential clearing
InternalEvent::SessionRestoreFailed => { + rev += 1; + state.rev = rev; + let _ = update_tx.send(AppUpdate::SessionTokens { + rev, + access_token: String::new(), + refresh_token: String::new(), + }); state.auth = AuthState::Ready; state.router = Router { default_screen: Screen::Login, screen_stack: vec![], }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rust/src/lib.rs` around lines 458 - 464, When handling InternalEvent::SessionRestoreFailed, in addition to setting state.auth = AuthState::Ready and resetting state.router to Router { default_screen: Screen::Login, screen_stack: vec![] }, also emit the same AppUpdate used by the logout path that clears persisted credentials (i.e., send an AppUpdate::SessionTokens with empty token/user values) so the Android side sees empty session tokens and removes stored credentials; update the handler around InternalEvent::SessionRestoreFailed to mirror the logout logic that emits the empty SessionTokens update.native/rmp-cli/src/bindings.rs-757-767 (1)
757-767:⚠️ Potential issue | 🟠 MajorDon't overwrite
android/local.properties.This replaces the whole file with a single
sdk.dirline and ignores write failures. Existing entries likendk.diror other local overrides get dropped after one run. Update just thesdk.direntry and surface I/O errors instead.🛠️ Safer update pattern
if !sdk.is_empty() { let lp = root.join("android/local.properties"); - let _ = std::fs::create_dir_all(root.join("android")); - let contents = format!("sdk.dir={}\n", sdk); - let _ = std::fs::write(lp, contents); + std::fs::create_dir_all(root.join("android")).map_err(|e| { + CliError::operational(format!("failed to create android dir: {e}")) + })?; + let existing = match std::fs::read_to_string(&lp) { + Ok(s) => s, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => String::new(), + Err(e) => { + return Err(CliError::operational(format!( + "failed to read {}: {e}", + lp.display() + ))); + } + }; + let mut lines: Vec<String> = existing + .lines() + .filter(|line| !line.trim_start().starts_with("sdk.dir=")) + .map(|line| line.to_string()) + .collect(); + lines.push(format!("sdk.dir={sdk}")); + std::fs::write(&lp, lines.join("\n") + "\n").map_err(|e| { + CliError::operational(format!("failed to write {}: {e}", lp.display())) + })?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rmp-cli/src/bindings.rs` around lines 757 - 767, The current block in bindings.rs blindly overwrites android/local.properties (using create_dir_all and std::fs::write) which loses other keys and ignores I/O errors; instead, read the existing file at lp if it exists, update or insert the sdk.dir line while preserving other lines (edit the line that starts with "sdk.dir=" or append if missing), create the android directory as needed with create_dir_all, and write the updated contents back, returning or propagating any std::io::Error rather than swallowing it so callers can handle failures; reference the sdk variable, lp path, and the current use of std::fs::create_dir_all and std::fs::write when locating where to change the logic.
🟡 Minor comments (2)
native/android/app/src/main/res/values/strings.xml-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorRename the Android app name resource to
Maple.
app_namestill saysHello Chat, which conflicts with the Maple branding in this PR. If anything starts referencing this resource, the launcher name will regress.Suggested change
- <string name="app_name">Hello Chat</string> + <string name="app_name">Maple</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/android/app/src/main/res/values/strings.xml` at line 2, The app name resource currently defines <string name="app_name">Hello Chat</string>, which conflicts with Maple branding; update the value of the resource named app_name to "Maple" (replace the current "Hello Chat" value) so the launcher and any references use the correct brand name; ensure the string resource entry with name="app_name" is the only change and keep the XML well-formed.native/rmp-cli/src/run.rs-465-469 (1)
465-469:⚠️ Potential issue | 🟡 MinorReturn the real Android target kind in JSON.
ensure_android_emulator()can return a physical device when--serialpoints at one, but this payload always says"kind":"emulator". That breaks callers consuming the structured output.🛠️ Suggested fix
if json { json_print(&JsonOk { ok: true, - data: serde_json::json!({"platform":"android","kind":"emulator","serial":serial,"app_id":pkg}), + data: serde_json::json!({ + "platform":"android", + "kind": if serial.starts_with("emulator-") { "emulator" } else { "device" }, + "serial": serial, + "app_id": pkg + }), }); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rmp-cli/src/run.rs` around lines 465 - 469, The JSON output hardcodes "kind":"emulator" even though ensure_android_emulator() may return a physical device; update the JSON payload to use the actual kind from the value returned by ensure_android_emulator() (e.g., use the returned target's kind/variant or an is_emulator() check) instead of the literal "emulator" so the JSON reflects platform:"android", kind:<actual_kind>, serial:serial, app_id:pkg; locate the JSON emission near json_print(&JsonOk { ... }) and replace the hardcoded string with the target's real kind from the function's return.
🧹 Nitpick comments (8)
native/rust/build.rs (1)
1-1: Consider removing the empty build script or documenting its purpose.An empty
build.rsadds compilation overhead without providing any functionality. If this is a placeholder for future UniFFI bindings or other build-time tasks, consider adding a comment explaining the intent. Otherwise, remove it to speed up builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rust/build.rs` at line 1, The build script contains an empty main function (fn main() {}) which adds unnecessary build overhead; either remove the build.rs file entirely or replace the empty fn main with a short explanatory comment indicating it is intentionally left blank as a placeholder for future UniFFI bindings or other build-time tasks so reviewers and CI understand the intent; update any project docs or Cargo config if you remove it to ensure no build expectations remain.native/.gitignore (1)
7-7: Commitflake.lockwith the flake.Ignoring the lock file makes the native toolchain drift across contributors and CI. If the flake is part of the supported workflow, its inputs should be pinned in version control.
Suggested change
-flake.lock🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/.gitignore` at line 7, Remove the flake.lock entry from .gitignore and commit the actual flake.lock file into the repository so the native toolchain inputs are pinned; specifically edit the .gitignore to delete the line "flake.lock", add the generated flake.lock to source control, and create a commit that includes the tracked flake.lock (so functions/tools that depend on the flake use the pinned inputs).native/android/app/src/main/java/cloud/opensecret/maple/ui/theme/Theme.kt (1)
63-112: Consider defining remaining Material 3 typography slots.The typography only defines 8 of the 15 Material 3 text styles (missing
displayLarge,displayMedium,displaySmall,headlineSmall,titleSmall,bodySmall,labelSmall). Material 3 will fall back to defaults for undefined styles, which may use system fonts instead of Manrope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/android/app/src/main/java/cloud/opensecret/maple/ui/theme/Theme.kt` around lines 63 - 112, MapleTypography currently sets only 8 Material3 styles and omits displayLarge/Medium/Small, headlineSmall, titleSmall, bodySmall, and labelSmall, causing fallbacks to system fonts; update the MapleTypography definition to include those missing TextStyle entries (displayLarge, displayMedium, displaySmall, headlineSmall, titleSmall, bodySmall, labelSmall), setting fontFamily = ManropeFamily and appropriate FontWeight, fontSize and lineHeight values consistent with the existing styles so all Material3 slots use ManropeFamily.native/justfile (2)
6-8: Hardcoded user-specific path in comment.The comment contains a developer-specific path (
/Users/tony/Dev/OpenSecret/maple/native/justfile). Consider using a placeholder or relative reference:# │ From macOS: just <recipe> │ -# │ From OrbStack: mac just -f /Users/tony/Dev/OpenSecret/maple/native/justfile <recipe> │ +# │ From OrbStack: mac just -f /path/to/maple/native/justfile <recipe> │🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/justfile` around lines 6 - 8, Replace the hardcoded developer-specific path "/Users/tony/Dev/OpenSecret/maple/native/justfile" in the commented example line with a neutral placeholder or relative reference (e.g., "<path-to-repo>/native/justfile" or "mac just -f ./native/justfile <recipe>") so the example is portable; update the comment in the justfile that currently contains the mac/OrbStack example line to use the placeholder instead of the user-specific path.
15-16: Sameosslhelper limitation as root justfile.This helper assumes macOS with Homebrew. See the comment on the root justfile for a potential fallback approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/justfile` around lines 15 - 16, The ossl helper in the justfile currently hardcodes Homebrew paths for OpenSSL (the ossl variable) which only works on macOS; update ossl to detect platform or provide fallbacks: if on macOS keep the Homebrew-derived PKG_CONFIG_PATH/OPENSSL_DIR, otherwise fall back to system pkg-config/OPENSSL_DIR or allow an environment override (e.g., use existing OPENSSL_DIR if set) so builds on Linux/CI don't break; modify the ossl definition in the native/justfile accordingly and ensure referenced targets that use ossl still consume the environment variables.native/ios/Sources/MapleWordmark.swift (2)
101-104: Force unwrap onCGPath.copy(using:).While unlikely to fail with a valid transform, the force unwrap could theoretically crash. Consider using optional binding or
guard let:- let transformed = cgPath.copy(using: [transform])! - let path = Path(transformed) - context.fill(path, with: .color(color)) + if let transformed = cgPath.copy(using: [transform]) { + let path = Path(transformed) + context.fill(path, with: .color(color)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/ios/Sources/MapleWordmark.swift` around lines 101 - 104, The loop in MapleWordmark.swift force-unwraps cgPath.copy(using:) which can crash; replace the force unwrap in the for cgPath in paths loop by using optional binding (guard let transformed = cgPath.copy(using: [transform]) else { continue } or if let) and only create Path(transformed) and call context.fill(path, with: .color(color)) when transformed is non-nil, optionally logging or skipping when copy returns nil; update references to cgPath.copy(using:), Path(...), and context.fill(...) accordingly.
10-11: UnusedGeometryReaderparameter.The geometry proxy is captured with
_but unused. Since the Canvas already receivessize, and the frame is explicitly set on line 108, you could simplify by removing GeometryReader:var body: some View { - GeometryReader { _ in - Canvas { context, size in + Canvas { context, size inThen close the Canvas before
.frame(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/ios/Sources/MapleWordmark.swift` around lines 10 - 11, Remove the unused GeometryReader wrapper around Canvas in MapleWordmark.swift: delete the GeometryReader { _ in ... } and move the Canvas block to the top-level view, ensuring you close the Canvas before applying the existing .frame(...) modifier; since Canvas already receives the size parameter, drop the unused geometry proxy and keep Canvas(size-aware) with the same drawing code and frame call.justfile (1)
215-217:native_osslassumes Homebrew on macOS.The OpenSSL configuration uses
brew --prefix openssl@3, which only works on macOS with Homebrew installed. Consider adding a fallback or documenting this requirement for contributors on other platforms.-native_ossl := 'PKG_CONFIG_PATH="$(brew --prefix openssl@3)/lib/pkgconfig" OPENSSL_DIR="$(brew --prefix openssl@3)"' +# macOS with Homebrew - Linux users may need to adjust paths +native_ossl := 'PKG_CONFIG_PATH="${OPENSSL_PKG_CONFIG:-$(brew --prefix openssl@3 2>/dev/null || echo /usr/lib/pkgconfig)/lib/pkgconfig}" OPENSSL_DIR="${OPENSSL_DIR:-$(brew --prefix openssl@3 2>/dev/null || echo /usr)}"'Alternatively, document the macOS-only assumption in the comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` around lines 215 - 217, The justfile sets native_ossl to use brew --prefix openssl@3 which only works on macOS/Homebrew; update the definition of native_ossl (the variable named native_ossl) to either detect platform and choose a fallback (e.g., use pkg-config or OPENSSL_DIR from environment when not on macOS) or add a clear comment above native_ossl documenting the macOS/Homebrew requirement and how contributors on Linux/Windows should set PKG_CONFIG_PATH/OPENSSL_DIR manually; reference and change the native_ossl variable so the behavior is explicit for non-macOS systems.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06b6e9f4-a5d1-4762-8388-cef07ee9aaab
⛔ Files ignored due to path filters (37)
native/Cargo.lockis excluded by!**/*.locknative/android/app/src/main/res/font/array_bold.ttfis excluded by!**/*.ttfnative/android/app/src/main/res/font/manrope_bold.ttfis excluded by!**/*.ttfnative/android/app/src/main/res/font/manrope_medium.ttfis excluded by!**/*.ttfnative/android/app/src/main/res/font/manrope_regular.ttfis excluded by!**/*.ttfnative/android/app/src/main/res/font/manrope_semibold.ttfis excluded by!**/*.ttfnative/android/app/src/main/res/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.pngnative/android/app/src/main/res/mipmap-hdpi/ic_launcher_round.pngis excluded by!**/*.pngnative/android/app/src/main/res/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.pngnative/android/app/src/main/res/mipmap-mdpi/ic_launcher_round.pngis excluded by!**/*.pngnative/android/app/src/main/res/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.pngnative/android/app/src/main/res/mipmap-xhdpi/ic_launcher_round.pngis excluded by!**/*.pngnative/android/app/src/main/res/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.pngnative/android/app/src/main/res/mipmap-xxhdpi/ic_launcher_round.pngis excluded by!**/*.pngnative/android/app/src/main/res/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.pngnative/android/app/src/main/res/mipmap-xxxhdpi/ic_launcher_round.pngis excluded by!**/*.pngnative/desktop/iced/Array-Bold.ttfis excluded by!**/*.ttfnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_1024.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_120.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_152.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_167.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_180.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_20.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_29.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_40.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_58.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_60.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_76.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_80.pngis excluded by!**/*.pngnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/icon_87.pngis excluded by!**/*.pngnative/ios/Sources/Fonts/Array-Bold.ttfis excluded by!**/*.ttfnative/ios/Sources/Fonts/Manrope-Bold.ttfis excluded by!**/*.ttfnative/ios/Sources/Fonts/Manrope-Medium.ttfis excluded by!**/*.ttfnative/ios/Sources/Fonts/Manrope-Regular.ttfis excluded by!**/*.ttfnative/ios/Sources/Fonts/Manrope-SemiBold.ttfis excluded by!**/*.ttfnative/ios/Sources/maple-wordmark.svgis excluded by!**/*.svgnative/rmp-cli/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (53)
.gitignorejustfilenative/.env.examplenative/.gitignorenative/Cargo.tomlnative/README.mdnative/android/app/build.gradle.ktsnative/android/app/src/main/AndroidManifest.xmlnative/android/app/src/main/java/cloud/opensecret/maple/AppManager.ktnative/android/app/src/main/java/cloud/opensecret/maple/MainActivity.ktnative/android/app/src/main/java/cloud/opensecret/maple/rust/maple_core.ktnative/android/app/src/main/java/cloud/opensecret/maple/ui/MainApp.ktnative/android/app/src/main/java/cloud/opensecret/maple/ui/theme/Color.ktnative/android/app/src/main/java/cloud/opensecret/maple/ui/theme/Theme.ktnative/android/app/src/main/res/drawable/maple_wordmark.xmlnative/android/app/src/main/res/values/strings.xmlnative/android/app/src/main/res/values/themes.xmlnative/android/build.gradle.ktsnative/android/gradle.propertiesnative/android/settings.gradle.ktsnative/desktop/iced/Cargo.tomlnative/desktop/iced/src/main.rsnative/desktop/iced/src/theme.rsnative/flake.nixnative/ios/Info.plistnative/ios/Sources/App.swiftnative/ios/Sources/AppManager.swiftnative/ios/Sources/Assets.xcassets/AppIcon.appiconset/Contents.jsonnative/ios/Sources/Assets.xcassets/Contents.jsonnative/ios/Sources/ContentView.swiftnative/ios/Sources/MapleTheme.swiftnative/ios/Sources/MapleWordmark.swiftnative/ios/project.ymlnative/justfilenative/rmp-architecture-bible.mdnative/rmp-cli/Cargo.tomlnative/rmp-cli/README.mdnative/rmp-cli/src/bindings.rsnative/rmp-cli/src/cli.rsnative/rmp-cli/src/config.rsnative/rmp-cli/src/devices.rsnative/rmp-cli/src/doctor.rsnative/rmp-cli/src/init.rsnative/rmp-cli/src/main.rsnative/rmp-cli/src/run.rsnative/rmp-cli/src/util.rsnative/rmp.tomlnative/rust/Cargo.tomlnative/rust/build.rsnative/rust/src/lib.rsnative/rust/uniffi.tomlnative/uniffi-bindgen/Cargo.tomlnative/uniffi-bindgen/src/main.rs
native/android/app/src/main/java/cloud/opensecret/maple/AppManager.kt
Outdated
Show resolved
Hide resolved
| // Start attestation handshake | ||
| { | ||
| let os = os_client.clone(); | ||
| let tx = core_tx_async.clone(); | ||
| rt.spawn(async move { | ||
| let event = match os.perform_attestation_handshake().await { | ||
| Ok(()) => InternalEvent::AttestationComplete, | ||
| Err(e) => InternalEvent::AttestationFailed(e.to_string()), | ||
| }; | ||
| let _ = tx.send(CoreMsg::Internal(Box::new(event))); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Startup attestation can clobber session restore.
The constructor always spawns an initial attestation task, and these handlers unconditionally switch the app to Ready/Login. If RestoreSession finishes first, a late AttestationComplete or AttestationFailed will overwrite the restored LoggedIn state and bounce the user back to the login screen. Guard startup events with an initialization generation/token, or ignore them once restore has begun.
Also applies to: 394-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/rust/src/lib.rs` around lines 327 - 338, The attestation spawn (using
os_client.clone(), core_tx_async.clone(), rt.spawn and
perform_attestation_handshake) can race with session restore and unconditionally
send InternalEvent::AttestationComplete/AttestationFailed wrapped in
CoreMsg::Internal, overwriting a restored LoggedIn state; fix by adding an
initialization generation/token or a guard flag in the core state that marks
"restore in progress" and have the spawned attestation task check that
token/flag before sending events (or include the generation with the
InternalEvent and have the core ignore events whose generation is older than the
current one); update both the startup attestation blocks (the one shown and the
similar block around lines 394-408) to attach/check this generation so late
attestation results are ignored once RestoreSession begins.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Deploying maple with
|
| Latest commit: |
f95cf40
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c239f0ff.maple-ca8.pages.dev |
| Branch Preview URL: | https://native-3-0.maple-ca8.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@justfile`:
- Around line 254-259: The `native-run-all` just recipe runs `native-run-ios`,
`native-run-android`, and `native-run-desktop` sequentially (blocking) which
prevents concurrent launches; update the Justfile to make the behavior explicit
by either renaming the target (e.g., `native-run-ios-then-android-then-desktop`)
or adding a clear comment/docstring above `native-run-all` stating it runs tasks
sequentially and users should open separate terminals for concurrent runs;
alternatively, if true concurrent launches are desired, add a new target
`native-run-concurrent` that launches `native-run-ios`, `native-run-android`,
and `native-run-desktop` in the background (with proper signal handling) rather
than changing the existing `native-run-all` behavior—refer to the
`native-run-all`, `native-run-ios`, `native-run-android`, and
`native-run-desktop` task names when making the change.
justfile
Outdated
| # Build all native platforms | ||
| native-build-all: native-build-desktop native-build-android | ||
| @echo "Native desktop and Android built. iOS is build+run only via rmp." | ||
|
|
||
| # Run all native platforms | ||
| native-run-all: native-bindings native-run-ios native-run-android native-run-desktop |
There was a problem hiding this comment.
native-run-all will not run concurrently.
native-run-ios, native-run-android, and native-run-desktop are blocking interactive processes (simulator/emulator/app). When specified as dependencies, just executes them sequentially—meaning only iOS will run, and Android/Desktop won't start until you manually terminate iOS.
If the intent is a convenience alias to "run everything at once," consider either:
- Documenting that users should run these in separate terminals
- Using background execution (though that complicates signal handling)
- Renaming to clarify behavior (e.g.,
native-run-ios-then-android-then-desktop)
📝 Suggested documentation clarification
# Run all native platforms
-native-run-all: native-bindings native-run-ios native-run-android native-run-desktop
+# NOTE: These are blocking processes - run in separate terminals instead
+# native-run-all: native-bindings native-run-ios native-run-android native-run-desktopOr keep as-is if sequential execution is intentional for testing purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@justfile` around lines 254 - 259, The `native-run-all` just recipe runs
`native-run-ios`, `native-run-android`, and `native-run-desktop` sequentially
(blocking) which prevents concurrent launches; update the Justfile to make the
behavior explicit by either renaming the target (e.g.,
`native-run-ios-then-android-then-desktop`) or adding a clear comment/docstring
above `native-run-all` stating it runs tasks sequentially and users should open
separate terminals for concurrent runs; alternatively, if true concurrent
launches are desired, add a new target `native-run-concurrent` that launches
`native-run-ios`, `native-run-android`, and `native-run-desktop` in the
background (with proper signal handling) rather than changing the existing
`native-run-all` behavior—refer to the `native-run-all`, `native-run-ios`,
`native-run-android`, and `native-run-desktop` task names when making the
change.
| return None; | ||
| } | ||
| let s = String::from_utf8_lossy(&out.stdout); | ||
| let avd = s.lines().last()?.trim(); |
There was a problem hiding this comment.
🔴 emulator_avd_name reads last line instead of first line, always returning None
The adb emu avd name command outputs the AVD name on the first line followed by "OK" on the last line. At native/rmp-cli/src/run.rs:703, s.lines().last() retrieves "OK", which is then filtered out by the check on line 704 (avd.eq_ignore_ascii_case("ok")), causing the function to always return None. This means the headless emulator detection logic (emulator_is_headless_only) that depends on the AVD name will never work, and the restart-headless-emulator-with-GUI feature in ensure_android_emulator is silently broken.
Was this helpful? React with 👍 or 👎 to provide feedback.
Let native local, dev, and prod builds switch endpoints without baking the API base URL into Rust. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Keep the iced desktop app on the shared default API URL path and record the related lockfile and formatting updates. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
| Message::SendMessage => { | ||
| if let ScreenState::Chat { compose } = screen { | ||
| if !compose.trim().is_empty() { | ||
| manager.dispatch(AppAction::SendMessage { | ||
| content: compose.clone(), | ||
| }); | ||
| *compose = String::new(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Desktop iced client clears compose text on Enter even when agent is typing, causing data loss
In the desktop iced SendMessage handler (native/desktop/iced/src/main.rs:407-415), pressing Enter dispatches AppAction::SendMessage and clears the compose text without checking state.is_agent_typing. The Rust core at native/rust/src/lib.rs:817 guards with if content.is_empty() || state.is_agent_typing { continue; }, silently dropping the message. But the desktop has already cleared *compose = String::new() on line 413, so the user's typed text is lost. Both iOS (native/ios/Sources/ContentView.swift:419) and Android (native/android/app/src/main/java/cloud/opensecret/maple/ui/MainApp.kt:519) correctly guard against this by checking isAgentTyping before sending.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
native/rust/src/lib.rs (1)
372-382:⚠️ Potential issue | 🔴 CriticalStartup attestation can still clobber session restore.
This task can still deliver
AttestationComplete/AttestationFailedafterRestoreSessionhas already moved the app toLoggedIn, and the handlers below still overwrite that state back toReady/Login. Guard startup attestation with an init generation/flag and ignore stale results once restore begins.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/rust/src/lib.rs` around lines 372 - 382, The attestation task spawned from perform_attestation_handshake() can send InternalEvent::AttestationComplete/AttestationFailed after RestoreSession has already transitioned the app to LoggedIn and thereby overwrite it; add an init-generation or boolean flag on the startup sequence (e.g., attach a startup_id or startup_in_progress flag to the struct that owns core_tx_async/os_client) and capture that id/flag in the spawned closure, then before sending CoreMsg::Internal ignore results whose captured id/flag does not match the current startup generation or where startup_in_progress has been cleared by RestoreSession; update RestoreSession to bump/clear that generation/flag when it begins so stale attestation results are dropped and cannot transition state back to Ready/Login.native/ios/Sources/AppManager.swift (1)
58-72:⚠️ Potential issue | 🟠 MajorDo not fall back to
defaultApiUrl()for shipped iOS builds.When
OPEN_SECRET_API_URLis missing, this still resolves to the Rust default (http://0.0.0.0:3000). That leaves packaged builds pointed at localhost instead of a real backend.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/ios/Sources/AppManager.swift` around lines 58 - 72, The configuredApiUrl() function currently falls back to defaultApiUrl(), which causes shipped iOS builds to point at the Rust dev default; change configuredApiUrl() so it does NOT return defaultApiUrl() when OPEN_SECRET_API_URL is absent—remove the final return defaultApiUrl() and instead return an explicit empty value (e.g., empty string or nil) or surface an error so production builds cannot silently point to localhost; update callers that expect a value to handle the empty/nil case accordingly (refer to configuredApiUrl() and defaultApiUrl()).native/desktop/iced/src/main.rs (1)
53-56:⚠️ Potential issue | 🟠 MajorDo not ship the desktop app with a localhost fallback.
If
OPEN_SECRET_API_URLis missing, this still falls back tomaple_core::default_api_url()(http://0.0.0.0:3000). Packaged builds then point at the user's machine instead of a real backend.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/desktop/iced/src/main.rs` around lines 53 - 56, The code currently falls back to maple_core::default_api_url() when OPEN_SECRET_API_URL is missing, which causes packaged desktop builds to point at localhost; change the api_url retrieval so it does not use maple_core::default_api_url()—instead require OPEN_SECRET_API_URL to be present and fail fast (e.g., replace the unwrap_or_else fallback with an expect/propagate error) so main.rs's api_url variable cannot silently default to http://0.0.0.0:3000; keep the environment key name OPEN_SECRET_API_URL in the error message for clarity.native/android/app/src/main/java/cloud/opensecret/maple/AppManager.kt (1)
58-63:⚠️ Potential issue | 🟠 MajorThe Android fallback is still emulator-only.
If
BuildConfig.OPEN_SECRET_API_URLis empty, this maps the Rust localhost default tohttp://10.0.2.2:3000. That only works in the emulator; physical devices and release builds will still point at a local machine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/android/app/src/main/java/cloud/opensecret/maple/AppManager.kt` around lines 58 - 63, configuredApiUrl() currently rewrites the Rust default "http://0.0.0.0:3000" to the emulator-only "http://10.0.2.2:3000" unconditionally; change it to only perform that translation when running on an Android emulator (so physical devices and release builds keep the original/default host). Update configuredApiUrl() to detect emulator status (e.g., using android.os.Build properties like Build.FINGERPRINT, Build.MODEL or Build.PRODUCT) and only return "http://10.0.2.2:3000" when an emulator is detected; otherwise return apiUrl (or the value from defaultApiUrl()) unchanged. Ensure you reference and preserve BuildConfig.OPEN_SECRET_API_URL and defaultApiUrl() when implementing this conditional logic.native/android/app/build.gradle.kts (1)
18-24:⚠️ Potential issue | 🔴 Critical
versionCodeis still outside Android's supported range.
defaultConfig.versionCodeis anInt, so3000000000will not compile here, and it also exceeds Google Play's maximum allowed value. This needs to be reduced to an in-rangeInt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/android/app/build.gradle.kts` around lines 18 - 24, defaultConfig.versionCode is set to 3000000000 which is too large for an Int and exceeds Google Play's max; change the literal to a valid 32-bit Int within Play's allowed range (e.g. <= 2100000000) or compute a safe int from versionName (e.g. derive from 3.0.0 -> 300000000) and replace the value on the versionCode assignment so the build compiles and adheres to Play limits.
🧹 Nitpick comments (4)
native/justfile (1)
57-68: Consider extracting duplicated API URL logic.The API URL determination pattern (
api_env="{{api_env}}"; while [...]; api_url=...) is repeated across six recipes. Extracting this to a helper function or a shell script would reduce duplication and simplify maintenance.Example: Define a reusable function at the top
+# Helper function to resolve API URL +resolve_api_url := ''' + api_env="$1" + while [[ "$api_env" == api_env=* ]]; do api_env="${api_env#api_env=}"; done + case "$api_env" in + dev) echo "https://enclave.secretgpt.ai" ;; + prod) echo "https://enclave.trymaple.ai" ;; + *) echo "http://0.0.0.0:3000" ;; + esac +''' + # build desktop app only (api_env: local|dev|prod) build-desktop api_env="local": - api_env="{{api_env}}"; while [[ "$api_env" == api_env=* ]]; do api_env="${api_env#api_env=}"; done; \ - api_url="http://0.0.0.0:3000"; \ - if [ "$api_env" = "dev" ]; then api_url="https://enclave.secretgpt.ai"; elif [ "$api_env" = "prod" ]; then api_url="https://enclave.trymaple.ai"; fi; \ + api_url=$({{resolve_api_url}} "{{api_env}}"); \ {{ossl}} OPEN_SECRET_API_URL="$api_url" cargo build -p maple_desktop_iced🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/justfile` around lines 57 - 68, Extract the duplicated API URL resolution into a single helper in the justfile (e.g., define a get_api_url() recipe or a shell function at the top) and call it from the recipes that currently contain the repeated block (notably build-desktop and run-desktop); replace the repeated lines starting with api_env="{{api_env}}"; while ... and the if/elif branch that sets api_url with a call to the helper so each recipe simply invokes the helper to set OPEN_SECRET_API_URL before running cargo build/run (keep function name like get_api_url or resolve_api_url to match references).native/desktop/iced/src/theme.rs (3)
249-259: Same border configuration issue: width is 0.0 with non-default color.Similar to
branded_header_style, this setscolor: NEUTRAL_200butwidth: 0.0renders the color ineffective.♻️ Simplify if no border is intended
pub fn header_style(_theme: &iced::Theme) -> iced::widget::container::Style { iced::widget::container::Style { background: Some(iced::Background::Color(NEUTRAL_0)), - border: Border { - radius: 0.0.into(), - width: 0.0, - color: NEUTRAL_200, - }, + border: Border::default(), ..Default::default() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/desktop/iced/src/theme.rs` around lines 249 - 259, The header_style currently sets Border { radius: 0.0.into(), width: 0.0, color: NEUTRAL_200 } which makes the color ineffective; update the function (header_style) to either remove the explicit Border entirely if no border is intended or set a visible width (e.g., width: 1.0) along with color: NEUTRAL_200 so the border shows, mirroring the same fix applied to branded_header_style; ensure you only change header_style's Border configuration and keep other fields as-is.
261-277: Border color is set but border width is 0.0, making the color ineffective.In
branded_header_style, the border color is configured butwidth: 0.0means the border won't render. If this is intentional (no border desired), the color assignment is dead code. If a subtle border is intended, setwidthto a positive value.♻️ Option A: Remove unused border color
pub fn branded_header_style(_theme: &iced::Theme) -> iced::widget::container::Style { iced::widget::container::Style { background: Some(iced::Background::Color(Color { a: 0.88, ..PEBBLE_800 })), - border: Border { - width: 0.0, - color: Color { - a: 0.1, - ..PEBBLE_600 - }, - ..Default::default() - }, + border: Border::default(), ..Default::default() } }♻️ Option B: Add border width if subtle border is desired
border: Border { - width: 0.0, + width: 1.0, color: Color { a: 0.1, ..PEBBLE_600 }, ..Default::default() },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/desktop/iced/src/theme.rs` around lines 261 - 277, In branded_header_style the container sets a Border color but uses width: 0.0 so the color never renders; either remove the Border color assignment (dead code) or set a small positive width (e.g., 1.0 or a fractional value) so the border becomes visible — update the Border struct in branded_header_style (and keep references to PEBBLE_600 if using the color) within the iced::widget::container::Style to reflect the chosen option.
168-184: Consider applying opacity to disabled ghost button for consistency.The
ghost_button_styledisabled state usesNEUTRAL_400directly, whileprimary_button_styleandsecondary_button_styleapply a0.5opacity to their disabled states. This creates a subtle visual inconsistency across button variants.♻️ Optional: Apply opacity for consistency
let text_color = match status { iced::widget::button::Status::Active => PEBBLE_500, iced::widget::button::Status::Hovered => PEBBLE_700, iced::widget::button::Status::Pressed => PEBBLE_800, - iced::widget::button::Status::Disabled => NEUTRAL_400, + iced::widget::button::Status::Disabled => Color { a: 0.5, ..NEUTRAL_400 }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/desktop/iced/src/theme.rs` around lines 168 - 184, In ghost_button_style, make the disabled text color match the other variants by applying 0.5 opacity to NEUTRAL_400 (instead of using NEUTRAL_400 directly); update the disabled branch inside the ghost_button_style function so it returns the same semi-transparent neutral color used by primary_button_style and secondary_button_style for their disabled states.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@justfile`:
- Around line 222-232: The three recipes declare an unused parameter api_env
(native-bindings, native-bindings-swift, native-bindings-kotlin); either drop
the parameter declarations or wire api_env through to the commands they run. Fix
by propagating api_env into the invocation of {{native_nix}} and/or the rmp
command (so the underlying native build gets the selected environment) or remove
api_env from the recipe signatures if not needed; update the recipes
native-bindings, native-bindings-swift, and native-bindings-kotlin accordingly.
In `@native/justfile`:
- Around line 30-36: The bindings recipes declare an unused parameter api_env
(seen on recipes bindings-swift, bindings-kotlin, and bindings) — either remove
the api_env parameter from each recipe signature to avoid confusion, or wire it
through like other recipes by exporting OPEN_SECRET_API_URL (or the appropriate
env var) based on api_env and passing it into the rmp/cargo run command; update
the recipe bodies for bindings-swift, bindings-kotlin, and bindings to either
drop api_env or to set OPEN_SECRET_API_URL (e.g., conditional on api_env values
local|dev|prod) before invoking rmp/cargo so the parameter is actually used.
In `@native/rmp-cli/src/bindings.rs`:
- Around line 96-150: The check path currently still invokes
build_ios_xcframework and build_android_so after running
check_ios_sources/check_android_sources; change the control flow so that when
args.check is true no builds are performed — either return early after the check
calls or move the build_ios_xcframework and build_android_so invocations into
the else branch that runs when !args.check. Apply this to the Swift branch
(match arm handling crate::cli::BindingsTarget::Swift, functions
check_ios_sources and build_ios_xcframework), the Kotlin branch
(crate::cli::BindingsTarget::Kotlin, functions check_android_sources and
build_android_so), and the All branch (both
check_ios_sources/check_android_sources and
build_ios_xcframework/build_android_so).
In `@native/rmp-cli/src/run.rs`:
- Around line 417-448: The Android path always builds the debug variant; update
the logic in run.rs where you create the gradle Command and set apk/pkg
(currently using ":app:assembleDebug", apk =
"android/app/build/outputs/apk/debug/app-debug.apk", and pkg =
format!("{app_id}.dev")) to branch on the release flag: when release is true,
run ":app:assembleRelease", point apk to
"android/app/build/outputs/apk/release/app-release.apk", and set pkg to the
production package id (e.g., app_id without the ".dev" suffix); otherwise keep
the existing debug assemble, apk path, and pkg naming. Ensure the Status check
and file-exists error messages still apply to the selected apk.
In `@native/rmp-cli/src/util.rs`:
- Around line 33-64: discover_xcode_dev_dir currently only checks DEVELOPER_DIR
and /Applications; add a step to run and read the output of "xcode-select -p"
(e.g., via std::process::Command) when DEVELOPER_DIR is unset, convert that path
to a PathBuf, and apply the same validation used elsewhere (check presence of
"usr/bin/xcrun" or "usr/bin/simctl" and call developer_dir_supports_iphoneos)
before falling back to scanning /Applications; update discover_xcode_dev_dir to
return that validated path if valid and handle/propagate any command errors as a
CliError similar to existing error handling.
In `@native/rust/src/lib.rs`:
- Around line 545-550: The stderr logging in the
InternalEvent::AgentMessageChunk match arm currently prints full user messages
via eprintln! with "messages:?"; remove or redact that sensitive payload and
instead log only non-sensitive metadata (e.g., step and messages.len() or a
redacted placeholder) in the AgentMessageChunk branch so conversation content is
not written to stderr/logcat/Xcode; update the eprintln! call in the
InternalEvent::AgentMessageChunk handler to omit messages:? and use a safe
alternative like step and message count or a fixed string.
- Around line 505-511: In the InternalEvent::SessionRestoreFailed handler, in
addition to setting state.auth = AuthState::Ready and resetting state.router,
clear any in-memory and persisted session tokens so stale credentials aren't
retried: set the relevant field (e.g., state.session_tokens or state.tokens) to
an empty/default SessionTokens (SessionTokens::default() or
SessionTokens::empty()) and call your persistence/flush routine (or emit the
existing SessionTokens update event) to overwrite storage so the native launch
code stops attempting the broken restore path; update the
InternalEvent::SessionRestoreFailed branch in lib.rs accordingly.
---
Duplicate comments:
In `@native/android/app/build.gradle.kts`:
- Around line 18-24: defaultConfig.versionCode is set to 3000000000 which is too
large for an Int and exceeds Google Play's max; change the literal to a valid
32-bit Int within Play's allowed range (e.g. <= 2100000000) or compute a safe
int from versionName (e.g. derive from 3.0.0 -> 300000000) and replace the value
on the versionCode assignment so the build compiles and adheres to Play limits.
In `@native/android/app/src/main/java/cloud/opensecret/maple/AppManager.kt`:
- Around line 58-63: configuredApiUrl() currently rewrites the Rust default
"http://0.0.0.0:3000" to the emulator-only "http://10.0.2.2:3000"
unconditionally; change it to only perform that translation when running on an
Android emulator (so physical devices and release builds keep the
original/default host). Update configuredApiUrl() to detect emulator status
(e.g., using android.os.Build properties like Build.FINGERPRINT, Build.MODEL or
Build.PRODUCT) and only return "http://10.0.2.2:3000" when an emulator is
detected; otherwise return apiUrl (or the value from defaultApiUrl()) unchanged.
Ensure you reference and preserve BuildConfig.OPEN_SECRET_API_URL and
defaultApiUrl() when implementing this conditional logic.
In `@native/desktop/iced/src/main.rs`:
- Around line 53-56: The code currently falls back to
maple_core::default_api_url() when OPEN_SECRET_API_URL is missing, which causes
packaged desktop builds to point at localhost; change the api_url retrieval so
it does not use maple_core::default_api_url()—instead require
OPEN_SECRET_API_URL to be present and fail fast (e.g., replace the
unwrap_or_else fallback with an expect/propagate error) so main.rs's api_url
variable cannot silently default to http://0.0.0.0:3000; keep the environment
key name OPEN_SECRET_API_URL in the error message for clarity.
In `@native/ios/Sources/AppManager.swift`:
- Around line 58-72: The configuredApiUrl() function currently falls back to
defaultApiUrl(), which causes shipped iOS builds to point at the Rust dev
default; change configuredApiUrl() so it does NOT return defaultApiUrl() when
OPEN_SECRET_API_URL is absent—remove the final return defaultApiUrl() and
instead return an explicit empty value (e.g., empty string or nil) or surface an
error so production builds cannot silently point to localhost; update callers
that expect a value to handle the empty/nil case accordingly (refer to
configuredApiUrl() and defaultApiUrl()).
In `@native/rust/src/lib.rs`:
- Around line 372-382: The attestation task spawned from
perform_attestation_handshake() can send
InternalEvent::AttestationComplete/AttestationFailed after RestoreSession has
already transitioned the app to LoggedIn and thereby overwrite it; add an
init-generation or boolean flag on the startup sequence (e.g., attach a
startup_id or startup_in_progress flag to the struct that owns
core_tx_async/os_client) and capture that id/flag in the spawned closure, then
before sending CoreMsg::Internal ignore results whose captured id/flag does not
match the current startup generation or where startup_in_progress has been
cleared by RestoreSession; update RestoreSession to bump/clear that
generation/flag when it begins so stale attestation results are dropped and
cannot transition state back to Ready/Login.
---
Nitpick comments:
In `@native/desktop/iced/src/theme.rs`:
- Around line 249-259: The header_style currently sets Border { radius:
0.0.into(), width: 0.0, color: NEUTRAL_200 } which makes the color ineffective;
update the function (header_style) to either remove the explicit Border entirely
if no border is intended or set a visible width (e.g., width: 1.0) along with
color: NEUTRAL_200 so the border shows, mirroring the same fix applied to
branded_header_style; ensure you only change header_style's Border configuration
and keep other fields as-is.
- Around line 261-277: In branded_header_style the container sets a Border color
but uses width: 0.0 so the color never renders; either remove the Border color
assignment (dead code) or set a small positive width (e.g., 1.0 or a fractional
value) so the border becomes visible — update the Border struct in
branded_header_style (and keep references to PEBBLE_600 if using the color)
within the iced::widget::container::Style to reflect the chosen option.
- Around line 168-184: In ghost_button_style, make the disabled text color match
the other variants by applying 0.5 opacity to NEUTRAL_400 (instead of using
NEUTRAL_400 directly); update the disabled branch inside the ghost_button_style
function so it returns the same semi-transparent neutral color used by
primary_button_style and secondary_button_style for their disabled states.
In `@native/justfile`:
- Around line 57-68: Extract the duplicated API URL resolution into a single
helper in the justfile (e.g., define a get_api_url() recipe or a shell function
at the top) and call it from the recipes that currently contain the repeated
block (notably build-desktop and run-desktop); replace the repeated lines
starting with api_env="{{api_env}}"; while ... and the if/elif branch that sets
api_url with a call to the helper so each recipe simply invokes the helper to
set OPEN_SECRET_API_URL before running cargo build/run (keep function name like
get_api_url or resolve_api_url to match references).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b539e0f6-fc76-478a-863f-2816aa2ac340
⛔ Files ignored due to path filters (1)
native/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
justfilenative/Cargo.tomlnative/android/app/build.gradle.ktsnative/android/app/src/main/java/cloud/opensecret/maple/AppManager.ktnative/desktop/iced/src/main.rsnative/desktop/iced/src/theme.rsnative/ios/Info.plistnative/ios/Sources/AppManager.swiftnative/justfilenative/rmp-cli/src/bindings.rsnative/rmp-cli/src/run.rsnative/rmp-cli/src/util.rsnative/rust/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- native/ios/Info.plist
| # Regenerate native FFI bindings (Swift + Kotlin) (api_env: local|dev|prod) | ||
| native-bindings api_env="local": | ||
| {{native_nix}} "unset CC CXX AR RANLIB && rmp bindings all" | ||
|
|
||
| # Regenerate native Swift FFI bindings (api_env: local|dev|prod) | ||
| native-bindings-swift api_env="local": | ||
| {{native_nix}} "unset CC CXX AR RANLIB && rmp bindings swift" | ||
|
|
||
| # Regenerate native Kotlin FFI bindings (api_env: local|dev|prod) | ||
| native-bindings-kotlin api_env="local": | ||
| {{native_nix}} "unset CC CXX AR RANLIB && rmp bindings kotlin" |
There was a problem hiding this comment.
Unused api_env parameter in native-bindings recipes.
Same issue as native/justfile: the api_env parameter is declared but the bindings commands don't use it. Either remove the parameter or wire it through to the underlying command.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@justfile` around lines 222 - 232, The three recipes declare an unused
parameter api_env (native-bindings, native-bindings-swift,
native-bindings-kotlin); either drop the parameter declarations or wire api_env
through to the commands they run. Fix by propagating api_env into the invocation
of {{native_nix}} and/or the rmp command (so the underlying native build gets
the selected environment) or remove api_env from the recipe signatures if not
needed; update the recipes native-bindings, native-bindings-swift, and
native-bindings-kotlin accordingly.
| # regenerate Swift FFI bindings (api_env: local|dev|prod) | ||
| bindings-swift api_env="local": | ||
| if command -v nix >/dev/null 2>&1; then \ | ||
| nix develop --command bash -c "unset CC CXX AR RANLIB && if command -v rmp >/dev/null 2>&1; then rmp bindings swift; else cargo run --manifest-path rmp-cli/Cargo.toml -- bindings swift; fi"; \ | ||
| else \ | ||
| unset CC CXX AR RANLIB && if command -v rmp >/dev/null 2>&1; then rmp bindings swift; else cargo run --manifest-path rmp-cli/Cargo.toml -- bindings swift; fi; \ | ||
| fi |
There was a problem hiding this comment.
Unused api_env parameter in bindings recipes.
The api_env parameter is declared on lines 31, 39, and 47 but is never used in the recipe bodies. The bindings commands don't pass api_env or set OPEN_SECRET_API_URL. If bindings generation doesn't require API configuration, consider removing the parameter to avoid confusion; otherwise, wire it through like the other recipes.
Suggested fix (if parameter is not needed)
-# regenerate Swift FFI bindings (api_env: local|dev|prod)
-bindings-swift api_env="local":
+# regenerate Swift FFI bindings
+bindings-swift:
if command -v nix >/dev/null 2>&1; then \Apply similar changes to bindings-kotlin (line 39) and bindings (line 47).
📝 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.
| # regenerate Swift FFI bindings (api_env: local|dev|prod) | |
| bindings-swift api_env="local": | |
| if command -v nix >/dev/null 2>&1; then \ | |
| nix develop --command bash -c "unset CC CXX AR RANLIB && if command -v rmp >/dev/null 2>&1; then rmp bindings swift; else cargo run --manifest-path rmp-cli/Cargo.toml -- bindings swift; fi"; \ | |
| else \ | |
| unset CC CXX AR RANLIB && if command -v rmp >/dev/null 2>&1; then rmp bindings swift; else cargo run --manifest-path rmp-cli/Cargo.toml -- bindings swift; fi; \ | |
| fi | |
| # regenerate Swift FFI bindings | |
| bindings-swift: | |
| if command -v nix >/dev/null 2>&1; then \ | |
| nix develop --command bash -c "unset CC CXX AR RANLIB && if command -v rmp >/dev/null 2>&1; then rmp bindings swift; else cargo run --manifest-path rmp-cli/Cargo.toml -- bindings swift; fi"; \ | |
| else \ | |
| unset CC CXX AR RANLIB && if command -v rmp >/dev/null 2>&1; then rmp bindings swift; else cargo run --manifest-path rmp-cli/Cargo.toml -- bindings swift; fi; \ | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/justfile` around lines 30 - 36, The bindings recipes declare an unused
parameter api_env (seen on recipes bindings-swift, bindings-kotlin, and
bindings) — either remove the api_env parameter from each recipe signature to
avoid confusion, or wire it through like other recipes by exporting
OPEN_SECRET_API_URL (or the appropriate env var) based on api_env and passing it
into the rmp/cargo run command; update the recipe bodies for bindings-swift,
bindings-kotlin, and bindings to either drop api_env or to set
OPEN_SECRET_API_URL (e.g., conditional on api_env values local|dev|prod) before
invoking rmp/cargo so the parameter is actually used.
| match args.target { | ||
| crate::cli::BindingsTarget::Swift => { | ||
| let _ = ios.ok_or_else(|| CliError::user("rmp.toml missing [ios] section"))?; | ||
| if args.clean { | ||
| clean_ios(root, verbose)?; | ||
| } | ||
| if args.check { | ||
| check_ios_sources(root, &cfg, verbose)?; | ||
| } else { | ||
| generate_ios_sources(root, &cfg, profile, false, verbose)?; | ||
| } | ||
| build_ios_xcframework( | ||
| root, | ||
| &core_lib, | ||
| &core_pkg, | ||
| &default_ios_targets(), | ||
| profile, | ||
| verbose, | ||
| )?; | ||
| } | ||
| crate::cli::BindingsTarget::Kotlin => { | ||
| let _ = android.ok_or_else(|| CliError::user("rmp.toml missing [android] section"))?; | ||
| if args.clean { | ||
| clean_android(root, verbose)?; | ||
| } | ||
| if args.check { | ||
| check_android_sources(root, &cfg, verbose)?; | ||
| } else { | ||
| generate_android_sources(root, &cfg, profile, false, verbose)?; | ||
| } | ||
| build_android_so(root, &core_pkg, &default_android_abis(), profile, verbose)?; | ||
| } | ||
| crate::cli::BindingsTarget::All => { | ||
| let _ = ios.ok_or_else(|| CliError::user("rmp.toml missing [ios] section"))?; | ||
| let _ = android.ok_or_else(|| CliError::user("rmp.toml missing [android] section"))?; | ||
| if args.clean { | ||
| clean_ios(root, verbose)?; | ||
| clean_android(root, verbose)?; | ||
| } | ||
| if args.check { | ||
| check_ios_sources(root, &cfg, verbose)?; | ||
| check_android_sources(root, &cfg, verbose)?; | ||
| } else { | ||
| generate_ios_sources(root, &cfg, profile, false, verbose)?; | ||
| generate_android_sources(root, &cfg, profile, false, verbose)?; | ||
| } | ||
| build_ios_xcframework( | ||
| root, | ||
| &core_lib, | ||
| &core_pkg, | ||
| &default_ios_targets(), | ||
| profile, | ||
| verbose, | ||
| )?; | ||
| build_android_so(root, &core_pkg, &default_android_abis(), profile, verbose)?; |
There was a problem hiding this comment.
--check still performs full platform builds.
In the args.check branches, the source diff runs first, but the code then immediately builds the XCFramework and Android .so outputs anyway. That makes bindings --check mutate build artifacts and require Xcode/NDK even when the generated sources already match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/rmp-cli/src/bindings.rs` around lines 96 - 150, The check path
currently still invokes build_ios_xcframework and build_android_so after running
check_ios_sources/check_android_sources; change the control flow so that when
args.check is true no builds are performed — either return early after the check
calls or move the build_ios_xcframework and build_android_so invocations into
the else branch that runs when !args.check. Apply this to the Swift branch
(match arm handling crate::cli::BindingsTarget::Swift, functions
check_ios_sources and build_ios_xcframework), the Kotlin branch
(crate::cli::BindingsTarget::Kotlin, functions check_android_sources and
build_android_so), and the All branch (both
check_ios_sources/check_android_sources and
build_ios_xcframework/build_android_so).
| // Assemble debug APK. | ||
| human_log(verbose, "gradle assembleDebug"); | ||
| let ci = is_ci(); | ||
| let mut cmd = Command::new("./gradlew"); | ||
| cmd.current_dir(root.join("android")) | ||
| .arg(":app:assembleDebug"); | ||
| if ci { | ||
| // CI stability + debuggability. | ||
| cmd.arg("--no-daemon") | ||
| .arg("--console=plain") | ||
| .arg("--stacktrace"); | ||
| } else if verbose { | ||
| cmd.arg("--console=plain"); | ||
| } | ||
| let status = cmd | ||
| .stdout(Stdio::inherit()) | ||
| .stderr(Stdio::inherit()) | ||
| .status() | ||
| .map_err(|e| CliError::operational(format!("failed to run gradlew: {e}")))?; | ||
| if !status.success() { | ||
| return Err(CliError::operational("gradle assembleDebug failed")); | ||
| } | ||
|
|
||
| let apk = root.join("android/app/build/outputs/apk/debug/app-debug.apk"); | ||
| if !apk.is_file() { | ||
| return Err(CliError::operational(format!( | ||
| "expected apk not found: {}", | ||
| apk.to_string_lossy() | ||
| ))); | ||
| } | ||
|
|
||
| let pkg = format!("{app_id}.dev"); |
There was a problem hiding this comment.
--release is ignored in the Android run path.
This branch always runs :app:assembleDebug, reads app-debug.apk, and launches "{appId}.dev", even when release is true. rmp run android --release therefore still builds and installs the debug variant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/rmp-cli/src/run.rs` around lines 417 - 448, The Android path always
builds the debug variant; update the logic in run.rs where you create the gradle
Command and set apk/pkg (currently using ":app:assembleDebug", apk =
"android/app/build/outputs/apk/debug/app-debug.apk", and pkg =
format!("{app_id}.dev")) to branch on the release flag: when release is true,
run ":app:assembleRelease", point apk to
"android/app/build/outputs/apk/release/app-release.apk", and set pkg to the
production package id (e.g., app_id without the ".dev" suffix); otherwise keep
the existing debug assemble, apk path, and pkg naming. Ensure the Status check
and file-exists error messages still apply to the selected apk.
| pub fn discover_xcode_dev_dir() -> Result<PathBuf, CliError> { | ||
| if let Ok(v) = std::env::var("DEVELOPER_DIR") { | ||
| let p = PathBuf::from(v); | ||
| if (p.join("usr/bin/xcrun").exists() || p.join("usr/bin/simctl").exists()) | ||
| && developer_dir_supports_iphoneos(&p) | ||
| { | ||
| return Ok(p); | ||
| } | ||
| } | ||
| let apps = std::fs::read_dir("/Applications") | ||
| .map_err(|e| CliError::operational(format!("failed to read /Applications: {e}")))?; | ||
| let mut candidates: Vec<PathBuf> = vec![]; | ||
| for ent in apps.flatten() { | ||
| let name = ent.file_name(); | ||
| let name = name.to_string_lossy(); | ||
| if !name.starts_with("Xcode") || !name.ends_with(".app") { | ||
| continue; | ||
| } | ||
| let dev = ent.path().join("Contents/Developer"); | ||
| if dev.is_dir() { | ||
| candidates.push(dev); | ||
| } | ||
| } | ||
| candidates.sort(); | ||
| for dev in candidates.into_iter().rev() { | ||
| if developer_dir_supports_iphoneos(&dev) { | ||
| return Ok(dev); | ||
| } | ||
| } | ||
| Err(CliError::operational( | ||
| "Xcode with iPhoneOS SDK not found under /Applications", | ||
| )) |
There was a problem hiding this comment.
Check xcode-select -p before scanning /Applications.
With DEVELOPER_DIR unset, this ignores the active developer directory entirely. A valid Xcode selected via xcode-select but installed outside /Applications will be treated as missing, so iOS commands fail on correctly configured machines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/rmp-cli/src/util.rs` around lines 33 - 64, discover_xcode_dev_dir
currently only checks DEVELOPER_DIR and /Applications; add a step to run and
read the output of "xcode-select -p" (e.g., via std::process::Command) when
DEVELOPER_DIR is unset, convert that path to a PathBuf, and apply the same
validation used elsewhere (check presence of "usr/bin/xcrun" or "usr/bin/simctl"
and call developer_dir_supports_iphoneos) before falling back to scanning
/Applications; update discover_xcode_dev_dir to return that validated path if
valid and handle/propagate any command errors as a CliError similar to existing
error handling.
| InternalEvent::SessionRestoreFailed => { | ||
| state.auth = AuthState::Ready; | ||
| state.router = Router { | ||
| default_screen: Screen::Login, | ||
| screen_stack: vec![], | ||
| }; | ||
| } |
There was a problem hiding this comment.
Clear persisted tokens when session restore fails.
A restore failure only sends the UI back to login. It never emits empty SessionTokens, so the stale tokens remain in storage and every launch retries the same broken restore path from native/ios/Sources/AppManager.swift, native/android/app/src/main/java/cloud/opensecret/maple/AppManager.kt, and native/desktop/iced/src/main.rs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/rust/src/lib.rs` around lines 505 - 511, In the
InternalEvent::SessionRestoreFailed handler, in addition to setting state.auth =
AuthState::Ready and resetting state.router, clear any in-memory and persisted
session tokens so stale credentials aren't retried: set the relevant field
(e.g., state.session_tokens or state.tokens) to an empty/default SessionTokens
(SessionTokens::default() or SessionTokens::empty()) and call your
persistence/flush routine (or emit the existing SessionTokens update event) to
overwrite storage so the native launch code stops attempting the broken restore
path; update the InternalEvent::SessionRestoreFailed branch in lib.rs
accordingly.
| InternalEvent::AgentMessageChunk { messages, step } => { | ||
| // Each event carries new messages for this step. | ||
| // Finalize any currently-streaming message, then | ||
| // append all new messages (last one stays streaming). | ||
| eprintln!("[agent] step={step} messages={messages:?}"); | ||
|
|
There was a problem hiding this comment.
Remove chat content from stderr logging.
messages:? dumps full streamed agent payloads into stderr/logcat/Xcode logs. That's user conversation data and should not be logged in normal builds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/rust/src/lib.rs` around lines 545 - 550, The stderr logging in the
InternalEvent::AgentMessageChunk match arm currently prints full user messages
via eprintln! with "messages:?"; remove or redact that sensitive payload and
instead log only non-sensitive metadata (e.g., step and messages.len() or a
redacted placeholder) in the AgentMessageChunk branch so conversation content is
not written to stderr/logcat/Xcode; update the eprintln! call in the
InternalEvent::AgentMessageChunk handler to omit messages:? and use a safe
alternative like step and message count or a fixed string.
Ship signed dev builds for iOS, Android, macOS, and Linux on every PR so we can validate native 3.0 releases without changing the existing 2.0 automation. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
native/scripts/ci/package-linux.sh (1)
84-85: Consider pinninglinuxdeployto a specific release for reproducible builds.The script downloads from the
continuousrelease, which changes over time and could introduce unexpected behavior or break builds. Pinning to a specific version ensures reproducibility.💡 Example with pinned version
-curl -fsSL "https://github.com/linuxdeploy/linuxdeploy/releases/download/continuous/linuxdeploy-x86_64.AppImage" -o "$linuxdeploy_path" +# Pin to a specific release for reproducibility +LINUXDEPLOY_VERSION="1-alpha-20240109-1" +curl -fsSL "https://github.com/linuxdeploy/linuxdeploy/releases/download/${LINUXDEPLOY_VERSION}/linuxdeploy-x86_64.AppImage" -o "$linuxdeploy_path"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/scripts/ci/package-linux.sh` around lines 84 - 85, The script currently downloads linuxdeploy from the "continuous" channel which makes builds non-reproducible; change it to use a pinned release by adding a LINUXDEPLOY_VERSION variable and substitute that into the download URL (replace "continuous" with the specific tag) where linuxdeploy_path is used, e.g., construct the URL using LINUXDEPLOY_VERSION and keep the chmod +x "$linuxdeploy_path" step as-is so the script always retrieves a fixed, reproducible linuxdeploy-x86_64.AppImage.native/android/app/build.gradle.kts (1)
133-135: Update to stable version1.1.0.
security-crypto:1.1.0-alpha06is an alpha release. A stable1.1.0is available and should be used for production builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/android/app/build.gradle.kts` around lines 133 - 135, Replace the alpha dependency declaration implementation("androidx.security:security-crypto:1.1.0-alpha06") with the stable release by updating it to implementation("androidx.security:security-crypto:1.1.0"); ensure this change is made in the dependency block where the security-crypto implementation is declared so production builds use the stable 1.1.0 artifact.native/scripts/ci/build-android-release.sh (1)
6-6: Consider addingrequire_envvalidation for sourced variables.The script relies on variables from
common.sh(native_root,ci_build_root,maple_version_value,maple_build_number,open_secret_api_url) but doesn't explicitly validate they're set. Addingrequire_envafter sourcing would provide clearer error messages ifcommon.shfails to compute expected values.🛡️ Suggested validation
source "$(cd "$(dirname "$0")" && pwd)/common.sh" +# Validate variables exported by common.sh +require_env native_root ci_build_root maple_version_value maple_build_number open_secret_api_url + require_command cargo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/scripts/ci/build-android-release.sh` at line 6, Add explicit environment validation after sourcing common.sh by calling require_env for each variable the script depends on: native_root, ci_build_root, maple_version_value, maple_build_number, and open_secret_api_url. Locate the top of build-android-release.sh where require_command is invoked (and where common.sh is sourced) and add require_env calls for those variable names so the script fails fast with clear messages if any sourced value is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/native-beta-android.yml:
- Around line 64-70: The heredoc writing native/android/keystore.properties
currently includes leading spaces so keys become " keyAlias" etc and
Properties.load() calls (e.g., keystoreProperties.getProperty("keyAlias") in
build.gradle.kts) fail; update the workflow to write the properties without
leading whitespace (for example use an unindented heredoc or a dedenting form)
so lines are exactly "keyAlias=...", "password=...", "storeFile=..." and
reference RUNNER_TEMP/maple-upload.jks and native/android/keystore.properties
when making the change.
In `@native/scripts/ci/common.sh`:
- Around line 55-79: The tauri_sign_artifact function writes the private key to
a temp file (key_file) but doesn’t guarantee cleanup if signing fails or the
script is interrupted; add a trap immediately after creating key_file (after
key_file="$(mktemp)") to remove "$key_file" on EXIT and ERR, then clear that
trap after the explicit rm -f "$key_file" so the temp is always cleaned up even
on failure or signals; keep the existing artifact signature checks and exit
behavior unchanged.
---
Nitpick comments:
In `@native/android/app/build.gradle.kts`:
- Around line 133-135: Replace the alpha dependency declaration
implementation("androidx.security:security-crypto:1.1.0-alpha06") with the
stable release by updating it to
implementation("androidx.security:security-crypto:1.1.0"); ensure this change is
made in the dependency block where the security-crypto implementation is
declared so production builds use the stable 1.1.0 artifact.
In `@native/scripts/ci/build-android-release.sh`:
- Line 6: Add explicit environment validation after sourcing common.sh by
calling require_env for each variable the script depends on: native_root,
ci_build_root, maple_version_value, maple_build_number, and open_secret_api_url.
Locate the top of build-android-release.sh where require_command is invoked (and
where common.sh is sourced) and add require_env calls for those variable names
so the script fails fast with clear messages if any sourced value is missing.
In `@native/scripts/ci/package-linux.sh`:
- Around line 84-85: The script currently downloads linuxdeploy from the
"continuous" channel which makes builds non-reproducible; change it to use a
pinned release by adding a LINUXDEPLOY_VERSION variable and substitute that into
the download URL (replace "continuous" with the specific tag) where
linuxdeploy_path is used, e.g., construct the URL using LINUXDEPLOY_VERSION and
keep the chmod +x "$linuxdeploy_path" step as-is so the script always retrieves
a fixed, reproducible linuxdeploy-x86_64.AppImage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0a2f84e-23da-485e-bafc-b6f339e256fb
📒 Files selected for processing (9)
.github/workflows/native-beta-android.yml.github/workflows/native-beta-desktop.yml.github/workflows/native-beta-ios.ymlnative/android/app/build.gradle.ktsnative/scripts/ci/build-android-release.shnative/scripts/ci/build-ios-testflight.shnative/scripts/ci/common.shnative/scripts/ci/package-linux.shnative/scripts/ci/package-macos.sh
| run: | | ||
| echo "$ANDROID_KEYSTORE_BASE64" | base64 --decode > "$RUNNER_TEMP/maple-upload.jks" | ||
| cat > native/android/keystore.properties <<EOF | ||
| keyAlias=$ANDROID_KEY_ALIAS | ||
| password=$ANDROID_KEY_PASSWORD | ||
| storeFile=$RUNNER_TEMP/maple-upload.jks | ||
| EOF |
There was a problem hiding this comment.
Leading whitespace in keystore.properties may cause parsing failures.
The heredoc content has leading spaces before each property key. Java's Properties.load() will include these spaces as part of the key names, causing lookups like keystoreProperties.getProperty("keyAlias") in build.gradle.kts to fail.
🐛 Proposed fix
run: |
echo "$ANDROID_KEYSTORE_BASE64" | base64 --decode > "$RUNNER_TEMP/maple-upload.jks"
cat > native/android/keystore.properties <<EOF
- keyAlias=$ANDROID_KEY_ALIAS
- password=$ANDROID_KEY_PASSWORD
- storeFile=$RUNNER_TEMP/maple-upload.jks
- EOF
+keyAlias=$ANDROID_KEY_ALIAS
+password=$ANDROID_KEY_PASSWORD
+storeFile=$RUNNER_TEMP/maple-upload.jks
+EOF🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/native-beta-android.yml around lines 64 - 70, The heredoc
writing native/android/keystore.properties currently includes leading spaces so
keys become " keyAlias" etc and Properties.load() calls (e.g.,
keystoreProperties.getProperty("keyAlias") in build.gradle.kts) fail; update the
workflow to write the properties without leading whitespace (for example use an
unindented heredoc or a dedenting form) so lines are exactly "keyAlias=...",
"password=...", "storeFile=..." and reference RUNNER_TEMP/maple-upload.jks and
native/android/keystore.properties when making the change.
| function tauri_sign_artifact() { | ||
| local artifact="$1" | ||
|
|
||
| if [[ -z "${TAURI_SIGNING_PRIVATE_KEY:-}" || -z "${TAURI_SIGNING_PRIVATE_KEY_PASSWORD:-}" ]]; then | ||
| return 0 | ||
| fi | ||
|
|
||
| require_command cargo-tauri | ||
|
|
||
| local key_file | ||
| key_file="$(mktemp)" | ||
| printf '%s' "$TAURI_SIGNING_PRIVATE_KEY" > "$key_file" | ||
|
|
||
| cargo-tauri signer sign \ | ||
| --private-key-path "$key_file" \ | ||
| --password "$TAURI_SIGNING_PRIVATE_KEY_PASSWORD" \ | ||
| "$artifact" | ||
|
|
||
| rm -f "$key_file" | ||
|
|
||
| if [[ ! -f "${artifact}.sig" ]]; then | ||
| printf 'Expected detached signature at %s.sig\n' "$artifact" >&2 | ||
| exit 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
Private key may be left on disk if signing fails or script is interrupted.
If cargo-tauri signer sign fails (e.g., due to set -e in the sourcing script) or the process is interrupted, the temp file containing the private key won't be cleaned up.
🛡️ Proposed fix using trap for guaranteed cleanup
function tauri_sign_artifact() {
local artifact="$1"
if [[ -z "${TAURI_SIGNING_PRIVATE_KEY:-}" || -z "${TAURI_SIGNING_PRIVATE_KEY_PASSWORD:-}" ]]; then
return 0
fi
require_command cargo-tauri
local key_file
key_file="$(mktemp)"
+ trap 'rm -f "$key_file"' EXIT
printf '%s' "$TAURI_SIGNING_PRIVATE_KEY" > "$key_file"
cargo-tauri signer sign \
--private-key-path "$key_file" \
--password "$TAURI_SIGNING_PRIVATE_KEY_PASSWORD" \
"$artifact"
rm -f "$key_file"
+ trap - EXIT
if [[ ! -f "${artifact}.sig" ]]; then
printf 'Expected detached signature at %s.sig\n' "$artifact" >&2
exit 1
fi
}📝 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.
| function tauri_sign_artifact() { | |
| local artifact="$1" | |
| if [[ -z "${TAURI_SIGNING_PRIVATE_KEY:-}" || -z "${TAURI_SIGNING_PRIVATE_KEY_PASSWORD:-}" ]]; then | |
| return 0 | |
| fi | |
| require_command cargo-tauri | |
| local key_file | |
| key_file="$(mktemp)" | |
| printf '%s' "$TAURI_SIGNING_PRIVATE_KEY" > "$key_file" | |
| cargo-tauri signer sign \ | |
| --private-key-path "$key_file" \ | |
| --password "$TAURI_SIGNING_PRIVATE_KEY_PASSWORD" \ | |
| "$artifact" | |
| rm -f "$key_file" | |
| if [[ ! -f "${artifact}.sig" ]]; then | |
| printf 'Expected detached signature at %s.sig\n' "$artifact" >&2 | |
| exit 1 | |
| fi | |
| } | |
| function tauri_sign_artifact() { | |
| local artifact="$1" | |
| if [[ -z "${TAURI_SIGNING_PRIVATE_KEY:-}" || -z "${TAURI_SIGNING_PRIVATE_KEY_PASSWORD:-}" ]]; then | |
| return 0 | |
| fi | |
| require_command cargo-tauri | |
| local key_file | |
| key_file="$(mktemp)" | |
| trap 'rm -f "$key_file"' EXIT | |
| printf '%s' "$TAURI_SIGNING_PRIVATE_KEY" > "$key_file" | |
| cargo-tauri signer sign \ | |
| --private-key-path "$key_file" \ | |
| --password "$TAURI_SIGNING_PRIVATE_KEY_PASSWORD" \ | |
| "$artifact" | |
| rm -f "$key_file" | |
| trap - EXIT | |
| if [[ ! -f "${artifact}.sig" ]]; then | |
| printf 'Expected detached signature at %s.sig\n' "$artifact" >&2 | |
| exit 1 | |
| fi | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/scripts/ci/common.sh` around lines 55 - 79, The tauri_sign_artifact
function writes the private key to a temp file (key_file) but doesn’t guarantee
cleanup if signing fails or the script is interrupted; add a trap immediately
after creating key_file (after key_file="$(mktemp)") to remove "$key_file" on
EXIT and ERR, then clear that trap after the explicit rm -f "$key_file" so the
temp is always cleaned up even on failure or signals; keep the existing artifact
signature checks and exit behavior unchanged.
| InternalEvent::AttestationComplete => { | ||
| state.auth = AuthState::Ready; | ||
| state.router = Router { | ||
| default_screen: Screen::Login, | ||
| screen_stack: vec![], | ||
| }; |
There was a problem hiding this comment.
🔴 Race condition: initial attestation can override successful session restore, kicking user to login
When the app starts, FfiApp::new spawns an initial attestation handshake asynchronously (native/rust/src/lib.rs:376). Immediately after, the platform layer dispatches RestoreSession which spawns its own attestation + get_user call (native/rust/src/lib.rs:745-770). Both async tasks send results to the same core_rx channel. If the restore succeeds first (SessionRestored at line 491 sets Screen::Chat), the initial attestation may complete later and its AttestationComplete handler at line 441 unconditionally resets state.auth = AuthState::Ready and state.router.default_screen = Screen::Login, kicking the already-authenticated user back to the login screen and losing their session state.
Race timeline that triggers the bug
FfiApp::newspawns initial attestation (T=0ms)- Platform dispatches
RestoreSession→ core thread sets tokens, spawns restore attestation (T≈2ms) - Restore attestation + get_user completes →
SessionRestoredprocessed → user sees Chat (T≈200ms) - Initial attestation completes (delayed by network) →
AttestationCompleteprocessed → state reset to Login (T≈250ms)
Prompt for agents
In native/rust/src/lib.rs, the AttestationComplete handler at lines 441-446 unconditionally sets state.auth = AuthState::Ready and state.router to Screen::Login. This creates a race condition with RestoreSession which can complete and set the user to Screen::Chat before the initial attestation finishes.
Fix: Guard the AttestationComplete handler to only transition to Ready/Login if the user is not already logged in. For example:
InternalEvent::AttestationComplete => {
if matches!(state.auth, AuthState::Initializing) {
state.auth = AuthState::Ready;
state.router = Router {
default_screen: Screen::Login,
screen_stack: vec![],
};
}
}
Apply the same guard to AttestationFailed at lines 448-455.
Was this helpful? React with 👍 or 👎 to provide feedback.
Use the correct Android NDK setup, run native binding generation from the project root, and embed the desktop beta API URL at build time so packaged apps stop falling back to localhost. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
| @@ -0,0 +1,3 @@ | |||
| <resources> | |||
| <string name="app_name">Hello Chat</string> | |||
There was a problem hiding this comment.
🟡 Android app_name string resource is 'Hello Chat' instead of 'Maple' (template leftover)
The string resource app_name in native/android/app/src/main/res/values/strings.xml is set to "Hello Chat" — a leftover from template scaffolding. While AndroidManifest.xml directly sets android:label="Maple", the app_name string resource may be referenced by the theme or other system components (e.g., the recent apps list, settings). The same leftover exists in native/android/settings.gradle.kts:15 where rootProject.name = "Hello Chat".
| <string name="app_name">Hello Chat</string> | |
| <string name="app_name">Maple</string> |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
native/scripts/ci/package-linux.sh (3)
73-73: Consider adding email to Maintainer field.Debian packaging best practices recommend the Maintainer field include an email address in the format
Name <email@example.com>. This helps users report issues.📝 Proposed fix
-Maintainer: OpenSecret +Maintainer: OpenSecret <support@opensecret.cloud>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/scripts/ci/package-linux.sh` at line 73, The Maintainer field currently reads "Maintainer: OpenSecret" and should include an email address; update the string "Maintainer: OpenSecret" in package-linux.sh to the Debian recommended format, e.g. "Maintainer: OpenSecret <maintainer@example.com>", ensuring the angle-bracketed email is present and properly quoted/escaped if used inside a shell variable or here-doc.
8-11: Consider adding shellcheck source directive for clarity.The variables
ci_build_root,maple_version_value,maple_build_number,native_root, andopen_secret_api_urlare referenced but defined incommon.sh. Adding a shellcheck directive suppresses the warnings and documents the dependency.📝 Proposed fix
source "$(cd "$(dirname "$0")" && pwd)/common.sh" +# shellcheck source=common.sh require_command cargo cargo-tauri curl dpkg-deb dpkg-shlibdepsAlternatively, add at the top of the variable block:
+# Variables ci_build_root, maple_version_value, maple_build_number, native_root, +# and open_secret_api_url are exported by common.sh linux_build_root="$ci_build_root/desktop-linux"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/scripts/ci/package-linux.sh` around lines 8 - 11, The script references variables defined in common.sh (ci_build_root, maple_version_value, maple_build_number, native_root, open_secret_api_url) but lacks a ShellCheck source directive; add a top-of-file ShellCheck directive like "shellcheck source=common.sh" immediately before the existing source/include of common.sh in package-linux.sh so ShellCheck recognizes those external vars and suppresses warnings while documenting the dependency.
87-88: Consider pinning linuxdeploy version and verifying checksum.Downloading from the
continuousrelease tag means the binary can change between CI runs, affecting reproducibility. For supply chain security, consider:
- Pinning to a specific release version (most recent is
1-alpha-20251107-1, though linuxdeploy releases are all labeled "alpha")- Verifying the SHA256 checksum after download
🔒 Proposed fix with checksum verification
-curl -fsSL "https://github.com/linuxdeploy/linuxdeploy/releases/download/continuous/linuxdeploy-x86_64.AppImage" -o "$linuxdeploy_path" -chmod +x "$linuxdeploy_path" +LINUXDEPLOY_URL="https://github.com/linuxdeploy/linuxdeploy/releases/download/1-alpha-20251107-1/linuxdeploy-x86_64.AppImage" +LINUXDEPLOY_SHA256="<insert-expected-sha256-here>" + +curl -fsSL "$LINUXDEPLOY_URL" -o "$linuxdeploy_path" +echo "$LINUXDEPLOY_SHA256 $linuxdeploy_path" | sha256sum -c - +chmod +x "$linuxdeploy_path"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/scripts/ci/package-linux.sh` around lines 87 - 88, Change the curl download to use a pinned linuxdeploy release instead of the "continuous" tag and add SHA256 checksum verification after download: update the URL that writes to "$linuxdeploy_path" to reference a specific release artifact (the exact release string for linuxdeploy-x86_64.AppImage), compute the downloaded file's SHA256 (e.g., via sha256sum) and compare it against the known expected checksum constant in the script, fail/exit non‑zero if the checksums do not match, and only then run chmod +x on "$linuxdeploy_path"; reference the variables and file name used in the script (linuxdeploy_path and linuxdeploy-x86_64.AppImage) when adding the expected checksum and verification step.native/scripts/ci/build-ios-testflight.sh (1)
40-43: Lock Cargo resolution in CI.This step should use
--lockedso binding generation cannot silently pick up a different dependency graph than the checked-in lockfile.Proposed change
( cd "$native_root" - cargo run --manifest-path "$native_root/rmp-cli/Cargo.toml" -- bindings swift + cargo run --locked --manifest-path "$native_root/rmp-cli/Cargo.toml" -- bindings swift )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@native/scripts/ci/build-ios-testflight.sh` around lines 40 - 43, The cargo invocation that generates Swift bindings (the subshell running cargo run --manifest-path "$native_root/rmp-cli/Cargo.toml" -- bindings swift) should be locked to the checked-in Cargo.lock; update that command to pass the --locked flag so dependency resolution cannot change in CI. Modify the subshell invocation that runs cargo run in build-ios-testflight.sh (the line invoking rmp-cli with -- bindings swift) to include --locked immediately after cargo run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@native/scripts/ci/build-android-release.sh`:
- Line 15: The android_version_code computation uses the wrong base constant;
update the base from 300000000 to 3000000000 so that
android_version_code="${MAPLE_ANDROID_VERSION_CODE:-$((3000000000 +
maple_build_number))}" produces values like 3000000001; locate the expression
that sets android_version_code (and references MAPLE_ANDROID_VERSION_CODE and
maple_build_number) and replace the base constant accordingly to match the PR
objective of 3,000,000,000.
In `@native/scripts/ci/build-ios-testflight.sh`:
- Around line 88-91: The altool upload step (xcrun altool --upload-app ...)
doesn't pass the key file location so it fails when the private key is outside
~/.private_keys; update the xcrun altool invocation in build-ios-testflight.sh
to include the APPLE_API_KEY_PATH variable (e.g. add --apiKeyPath
"$APPLE_API_KEY_PATH") alongside --apiKey "$APPLE_API_KEY" and --apiIssuer
"$APPLE_API_ISSUER" so altool can locate the private key when it's placed in
$RUNNER_TEMP/app-store-connect/ or other custom paths.
---
Nitpick comments:
In `@native/scripts/ci/build-ios-testflight.sh`:
- Around line 40-43: The cargo invocation that generates Swift bindings (the
subshell running cargo run --manifest-path "$native_root/rmp-cli/Cargo.toml" --
bindings swift) should be locked to the checked-in Cargo.lock; update that
command to pass the --locked flag so dependency resolution cannot change in CI.
Modify the subshell invocation that runs cargo run in build-ios-testflight.sh
(the line invoking rmp-cli with -- bindings swift) to include --locked
immediately after cargo run.
In `@native/scripts/ci/package-linux.sh`:
- Line 73: The Maintainer field currently reads "Maintainer: OpenSecret" and
should include an email address; update the string "Maintainer: OpenSecret" in
package-linux.sh to the Debian recommended format, e.g. "Maintainer: OpenSecret
<maintainer@example.com>", ensuring the angle-bracketed email is present and
properly quoted/escaped if used inside a shell variable or here-doc.
- Around line 8-11: The script references variables defined in common.sh
(ci_build_root, maple_version_value, maple_build_number, native_root,
open_secret_api_url) but lacks a ShellCheck source directive; add a top-of-file
ShellCheck directive like "shellcheck source=common.sh" immediately before the
existing source/include of common.sh in package-linux.sh so ShellCheck
recognizes those external vars and suppresses warnings while documenting the
dependency.
- Around line 87-88: Change the curl download to use a pinned linuxdeploy
release instead of the "continuous" tag and add SHA256 checksum verification
after download: update the URL that writes to "$linuxdeploy_path" to reference a
specific release artifact (the exact release string for
linuxdeploy-x86_64.AppImage), compute the downloaded file's SHA256 (e.g., via
sha256sum) and compare it against the known expected checksum constant in the
script, fail/exit non‑zero if the checksums do not match, and only then run
chmod +x on "$linuxdeploy_path"; reference the variables and file name used in
the script (linuxdeploy_path and linuxdeploy-x86_64.AppImage) when adding the
expected checksum and verification step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 055669a4-e7de-4d27-8f28-1b0a71eeb91c
📒 Files selected for processing (5)
.github/workflows/native-beta-android.ymlnative/desktop/iced/src/main.rsnative/scripts/ci/build-android-release.shnative/scripts/ci/build-ios-testflight.shnative/scripts/ci/package-linux.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/native-beta-android.yml
- native/desktop/iced/src/main.rs
|
|
||
| android_build_root="$ci_build_root/android" | ||
| android_version_name="${MAPLE_ANDROID_VERSION_NAME:-${maple_version_value}-beta.${maple_build_number}}" | ||
| android_version_code="${MAPLE_ANDROID_VERSION_CODE:-$((300000000 + maple_build_number))}" |
There was a problem hiding this comment.
Version code base appears incorrect.
The PR objectives state the Android versionCode should be 3000000000 (3 billion), but this line uses 300000000 (300 million) as the base—off by a factor of 10. This would result in version codes like 300000001 instead of 3000000001 for the first beta build.
🐛 Proposed fix
-android_version_code="${MAPLE_ANDROID_VERSION_CODE:-$((300000000 + maple_build_number))}"
+android_version_code="${MAPLE_ANDROID_VERSION_CODE:-$((3000000000 + maple_build_number))}"📝 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.
| android_version_code="${MAPLE_ANDROID_VERSION_CODE:-$((300000000 + maple_build_number))}" | |
| android_version_code="${MAPLE_ANDROID_VERSION_CODE:-$((3000000000 + maple_build_number))}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/scripts/ci/build-android-release.sh` at line 15, The
android_version_code computation uses the wrong base constant; update the base
from 300000000 to 3000000000 so that
android_version_code="${MAPLE_ANDROID_VERSION_CODE:-$((3000000000 +
maple_build_number))}" produces values like 3000000001; locate the expression
that sets android_version_code (and references MAPLE_ANDROID_VERSION_CODE and
maple_build_number) and replace the base constant accordingly to match the PR
objective of 3,000,000,000.
| xcrun altool --upload-app --type ios \ | ||
| --file "$ipa_path" \ | ||
| --apiKey "$APPLE_API_KEY" \ | ||
| --apiIssuer "$APPLE_API_ISSUER" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'API_PRIVATE_KEYS_DIR|AuthKey_|APPLE_API_KEY_PATH|altool' \
.github/workflows native/scripts/ciRepository: OpenSecretCloud/Maple
Length of output: 8545
altool will not locate the private key file in workflows placing it outside ~/.private_keys/.
The script correctly passes $APPLE_API_KEY_PATH to xcodebuild (lines 58, 77) but the subsequent altool upload step (lines 88-91) only provides the key ID and issuer. While workflows like testflight-on-comment.yml and mobile-build.yml place the key in ~/.private_keys/ (where altool searches by default), others like native-beta-ios.yml and native-beta-desktop.yml place it in $RUNNER_TEMP/app-store-connect/. In those cases, altool fails at upload time because it cannot find the key.
Proposed change
if [[ -z "$ipa_path" ]]; then
echo "Failed to locate exported IPA" >&2
exit 1
fi
+altool_keys_dir="$ios_build_root/private_keys"
+ensure_dir "$altool_keys_dir"
+cp "$APPLE_API_KEY_PATH" "$altool_keys_dir/AuthKey_${APPLE_API_KEY}.p8"
+export API_PRIVATE_KEYS_DIR="$altool_keys_dir"
+
xcrun altool --upload-app --type ios \
--file "$ipa_path" \
--apiKey "$APPLE_API_KEY" \
--apiIssuer "$APPLE_API_ISSUER"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@native/scripts/ci/build-ios-testflight.sh` around lines 88 - 91, The altool
upload step (xcrun altool --upload-app ...) doesn't pass the key file location
so it fails when the private key is outside ~/.private_keys; update the xcrun
altool invocation in build-ios-testflight.sh to include the APPLE_API_KEY_PATH
variable (e.g. add --apiKeyPath "$APPLE_API_KEY_PATH") alongside --apiKey
"$APPLE_API_KEY" and --apiIssuer "$APPLE_API_ISSUER" so altool can locate the
private key when it's placed in $RUNNER_TEMP/app-store-connect/ or other custom
paths.
Restore the native Android Gradle wrapper in version control so CI can build the app, and place the App Store Connect key where altool expects it so TestFlight uploads complete. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
1 participant
Maple 3.0 Native App
Introduces
native/directory alongside existingfrontend/(2.0 Tauri), enabling both to coexist during the transition period.What's included
maple_core) — all business logic: single-agent chat, SSE streaming, session persistence, pagination, settings/delete agent.glassEffect()floating islandsKey decisions
cloud.opensecret.maple— matches 2.0's App Store/Play Store listings for upgrade continuity3000000000, above 2.0's2000019000)opensecret = "3.0.0-alpha.0"from crates.io (no submodule)X773Y823TN(matching 2.0)Design system
GlassEffectContainer,.glassEffect(),safeAreaInset)Relationship to 2.0
frontend/(2.0) is completely untouched. Both coexist. When 3.0 is ready for production, it replaces 2.0 on the same store listings.Build & run
Summary by CodeRabbit
New Features
Documentation