Maple 3.0: Native Rust/SwiftUI/Compose/iced app#462
Maple 3.0: Native Rust/SwiftUI/Compose/iced app#462AnthonyRonning wants to merge 31 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>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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)
|
![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.
🔴 Unconditional AgentDone after stream loop causes stale typing-indicator reset race condition
After the SSE while let Some(event) = stream.next().await loop at native/rust/src/lib.rs:845-865, an InternalEvent::AgentDone is unconditionally sent at line 867-869 as a "safety net." However, this creates a race condition: if the stream ends with an AgentError event, the error handler at native/rust/src/lib.rs:583-591 sets is_agent_typing = false and shows a toast. The user can then send a new message (M2), which sets is_agent_typing = true (line 835). Meanwhile, the old stream's task finishes and posts the stale AgentDone to the core channel. When the core processes this stale AgentDone, it incorrectly sets is_agent_typing = false for M2's in-progress stream, hiding the typing indicator while the new agent response is still streaming. Additionally, even in the normal (non-error) case, when the stream sends a Done event at line 856-857, the unconditional AgentDone at line 867 causes a duplicate that triggers an unnecessary extra state emission.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Deploying maple with
|
| Latest commit: |
ac3575c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://79b95cc6.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>
native/desktop/iced/src/main.rs
Outdated
| 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 compose text silently lost when Enter is pressed while agent is typing
In the iced desktop app, pressing Enter on the compose text input always dispatches Message::SendMessage, which unconditionally clears the local compose text at line 465. However, the Rust core (native/rust/src/lib.rs:834) silently drops SendMessage actions when state.is_agent_typing is true (via continue). The Send button is correctly disabled when the agent is typing (native/desktop/iced/src/main.rs:1243-1245), but the text input's on_submit at native/desktop/iced/src/main.rs:1251 fires regardless. The user's typed message is permanently lost.
The Android and iOS UIs both guard against this: Android checks !state.isAgentTyping before dispatching (native/android/app/src/main/java/cloud/opensecret/maple/ui/MainApp.kt:894), and iOS checks !manager.state.isAgentTyping (native/ios/Sources/ContentView.swift:669).
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.
native/rust/src/lib.rs
Outdated
| 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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
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>
Use a 512x512 desktop icon for AppImage packaging so linuxdeploy accepts the asset and the beta Linux artifacts complete successfully. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Make the App Store icon opaque, add the required iPad orientation support, and fail CI if altool reports a rejected upload so TestFlight jobs only pass when Apple accepts the build. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Use brand-aligned palettes for the login and chat screens so dark mode stays readable while the final design spec is still pending. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Keep the dark theme closer to the subtle light-mode treatment and declare the app's export compliance in Info.plist so TestFlight stops asking on each build. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Keep app restarts from surfacing ephemeral attestation errors, and downgrade any remaining native error toasts to non-blocking notices. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
native/desktop/iced/src/main.rs
Outdated
| Message::DismissToast { rev, toast } => { | ||
| if state.rev == rev && state.toast.as_deref() == Some(toast.as_str()) { | ||
| manager.dispatch(AppAction::ClearToast); | ||
| } |
There was a problem hiding this comment.
🟡 Desktop toast auto-dismiss fails due to strict rev equality check
In DismissToast handling, the desktop checks state.rev == rev where rev was captured 4 seconds earlier when the toast first appeared. Since every Rust core action (including the 30-second RefreshTimestamps tick and any streaming agent chunks) increments rev and emits a new FullState, state.rev will almost always have diverged from the captured rev by the time the dismiss timer fires. This means toasts will effectively never auto-dismiss during active chat sessions.
By contrast, iOS (native/ios/Sources/ContentView.swift:48) and Android (native/android/app/src/main/java/cloud/opensecret/maple/ui/MainApp.kt:88) only check whether the toast content still matches, which is the correct approach.
Was this helpful? React with 👍 or 👎 to provide feedback.
native/desktop/iced/src/main.rs
Outdated
| let rev = latest.rev; | ||
| Task::perform( | ||
| async move { | ||
| std::thread::sleep(std::time::Duration::from_secs(4)); |
There was a problem hiding this comment.
🟡 Blocking std::thread::sleep used inside async task blocks tokio worker thread
The toast dismiss timer uses std::thread::sleep inside an async move block passed to Task::perform. Since iced is configured with the tokio feature (native/desktop/iced/Cargo.toml:9), this blocks one of the two tokio worker threads (native/rust/src/lib.rs:412 configures worker_threads(2)) for 4 seconds each time a toast appears. Should use tokio::time::sleep(...).await instead.
| std::thread::sleep(std::time::Duration::from_secs(4)); | |
| tokio::time::sleep(std::time::Duration::from_secs(4)).await; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Update iOS, Android, and Desktop to use MPL abbreviated wordmark, plain text for AI messages, user bubbles, floating compose bar with Write placeholder and orange send pill. Remove sender labels, bump text size and message spacing. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Apply exact Figma tokens: header pills 43pt white@0.4, wordmark 16pt, icons 18px pebble800, compose bar white@0.64 rounded 24 with gradient send pill, Manrope Medium 16px/26 message text, radial gradient bg from top center. Desktop: canvas-drawn hamburger/search icons, transparent compose text input. Dark mode preserved on 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>
native/desktop/iced/src/main.rs
Outdated
| Message::DismissToast { rev, toast } => { | ||
| if state.rev == rev && state.toast.as_deref() == Some(toast.as_str()) { | ||
| manager.dispatch(AppAction::ClearToast); | ||
| } |
There was a problem hiding this comment.
🔴 Desktop toast auto-dismiss uses stale rev, so toasts never auto-clear during activity
In the desktop iced client (native/desktop/iced/src/main.rs:346-355), when a toast appears, a DismissToast message is scheduled after 4 seconds carrying the current rev. At line 399-400, the dismiss handler checks state.rev == rev. However, the Rust core increments rev on every action/event (line native/rust/src/lib.rs:1041), including periodic RefreshTimestamps ticks every 30s, agent streaming chunks, etc. Any state change during the 4-second window advances state.rev past the captured value, so the equality check fails and the toast is never dismissed. The toast then remains visible indefinitely until the user manually taps/clicks it.
| Message::DismissToast { rev, toast } => { | |
| if state.rev == rev && state.toast.as_deref() == Some(toast.as_str()) { | |
| manager.dispatch(AppAction::ClearToast); | |
| } | |
| Message::DismissToast { rev: _, toast } => { | |
| if state.toast.as_deref() == Some(toast.as_str()) { | |
| manager.dispatch(AppAction::ClearToast); | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
native/desktop/iced/src/main.rs
Outdated
| Task::perform( | ||
| async move { | ||
| std::thread::sleep(std::time::Duration::from_secs(4)); | ||
| (rev, toast) | ||
| }, | ||
| |(rev, toast)| Message::DismissToast { rev, toast }, | ||
| ) |
There was a problem hiding this comment.
🟡 std::thread::sleep used in async Task::perform, blocking the async executor thread
At native/desktop/iced/src/main.rs:351, std::thread::sleep(Duration::from_secs(4)) is called inside an async move block passed to Task::perform. This blocks the entire executor thread for 4 seconds instead of yielding. Since iced runs tasks on a limited thread pool, this can starve other pending async work. Should use an async-compatible sleep (e.g., tokio::time::sleep or iced::futures::future::pending-style timer).
Prompt for agents
In native/desktop/iced/src/main.rs around line 349-355, replace the std::thread::sleep call inside the async block with an async-friendly sleep. The Task::perform future should use something like async_std::task::sleep or tokio::time::sleep instead of std::thread::sleep. Since the project uses tokio (it's a dependency of maple_core), you could add tokio as a dev/direct dependency for the iced crate and use tokio::time::sleep(Duration::from_secs(4)).await. Alternatively, since iced has its own timer primitives, consider restructuring the toast dismissal to use a subscription-based timer instead of Task::perform with sleep.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
native/flake.nix
Outdated
| cat > android/local.properties <<EOF | ||
| sdk.dir=$ANDROID_HOME | ||
| EOF |
There was a problem hiding this comment.
🟡 Indented heredoc in flake.nix produces local.properties with leading whitespace in sdk.dir value
At native/flake.nix:122-124, the heredoc body sdk.dir=$ANDROID_HOME is indented with spaces (12 spaces of indentation from the Nix shellHook). Since this uses <<EOF (not <<-EOF), the content is written literally with leading whitespace. The resulting android/local.properties will contain sdk.dir=/path/to/sdk. While Java's Properties.load() strips leading whitespace from keys, Gradle's local.properties parser may not handle this consistently, and the value itself will have no leading whitespace only because the = immediately follows the key. This is fragile and inconsistent with the CI workflow's properly-parsed YAML heredoc.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Keep iOS sessions recoverable after relaunch while preserving native mobile chat scrolling and chrome polish. 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>
Let chat scrolls dismiss the iOS keyboard interactively so quick downward drags hide it without breaking normal transcript scrolling. 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>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…nd bundle flow 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>
AnthonyRonning
force-pushed
the
native-3.0
branch
from
6ee0bf3 to
ac3575c
Compare
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