feat(messaging): add signal configuration to web app#392
Open
ibhagwan wants to merge 4 commits intospacedriveapp:mainfrom
Open
feat(messaging): add signal configuration to web app#392ibhagwan wants to merge 4 commits intospacedriveapp:mainfrom
ibhagwan wants to merge 4 commits intospacedriveapp:mainfrom
Conversation
Contributor
|
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:
WalkthroughAdds first-class Signal platform support across frontend and backend: new Signal credential fields and UI, E.164 validation utilities, delivery-target parsing for named Signal instances, adapter resolution and cron/tool adapter propagation, status/permissions wiring, and related tests and docs updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
from
f9a4efb to
0376290
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
1 similar comment
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/send_message_to_another_channel.rs (1)
187-191:⚠️ Potential issue | 🟡 MinorTighten the Signal adapter check before enabling shorthand parsing.
starts_with("signal")is too broad here.current_adapter()can fall back to the channel ID (src/agent/channel.rs:527-535), so a non-Signal channel likesignal-alertswould incorrectly enter the Signal shorthand path for+123...,group:..., or bare UUID targets. Restrict this to the real adapter shapes ("signal"or"signal:...").Suggested fix
if let Some(current_adapter) = self .current_adapter .as_ref() - .filter(|adapter| adapter.starts_with("signal")) + .filter(|adapter| *adapter == "signal" || adapter.starts_with("signal:")) && let Some(target) = parse_implicit_signal_shorthand(&args.target, current_adapter) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/send_message_to_another_channel.rs` around lines 187 - 191, The current check uses adapter.starts_with("signal") which is too permissive; update the predicate in the self.current_adapter.as_ref().filter(...) used alongside parse_implicit_signal_shorthand(&args.target, current_adapter) to only allow exact "signal" or the namespaced form "signal:..." (e.g., adapter == "signal" || adapter.starts_with("signal:")), so non-Signal adapters like "signal-alerts" won't trigger Signal shorthand parsing in current_adapter / parse_implicit_signal_shorthand.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 42-47: Remove the "signal" entry from the DOC_LINKS map so the
settings UI doesn't surface a dead docs link; update the
Partial<Record<Platform, string>> DOC_LINKS constant in ChannelSettingCard
(remove the signal: "https://docs.spacebot.sh/signal-setup" key) and ensure any
code that reads DOC_LINKS (e.g., link rendering for platforms) will simply not
find a URL for Platform "signal" until the docs page is live.
In `@src/api/messaging.rs`:
- Around line 1288-1304: The current ok_or_else(...)? branches for
credentials.signal_http_url and credentials.signal_account return a bare
StatusCode::BAD_REQUEST which bypasses the MessagingInstanceActionResponse path;
change these to return an in-band validation response instead (e.g., construct
and return MessagingInstanceActionResponse::ValidationFailure with a descriptive
message like "signal: http_url is required" or "signal: account is required")
from the enclosing handler function rather than using the StatusCode; apply the
same pattern for the other occurrence noted around the invalid-E.164 branch
(lines ~1453-1469) so all user-correctable validation failures produce
MessagingInstanceActionResponse::ValidationFailure with clear messages instead
of StatusCode::BAD_REQUEST.
- Around line 1288-1304: The code that reads signal_account (via signal_account
and signal_http_url) can silently expand dm_allowed_users when the operator
changes account; fix by detecting when the provided signal_account differs from
the previously auto-added account and remove the old auto-added number from
dm_allowed_users (or rebuild permissions) so the allowed-DM set does not grow
forever; update the create/update branch that uses signal_account (and the
related block around lines handling signal_http_url/signal_account) to compare
the incoming account to the existing configured account and if different, call
the logic that removes the prior auto-added entry (or invoke
SignalPermissions::build_from_seed to regenerate permissions) so only the new
account remains authorized.
- Around line 1143-1146: The change allowed creating "signal" but other handlers
still reject it; update the API to treat Signal consistently by adding "signal"
to the platform handling paths: ensure messaging_status() includes Signal
instances in MessagingStatusResponse.instances, implement a start path for
"signal" inside toggle_platform() (mirroring how "telegram"/"discord" are
started) and allow deletion in delete_messaging_instance() so it no longer
rejects "signal"; ensure any platform validation or match arms that previously
excluded "signal" are updated so creation, reporting, toggling, and deletion all
accept the "signal" platform.
- Around line 136-143: The is_valid_e164 function currently allows numbers like
+01234567 and >15 digits; update is_valid_e164 to require the first digit after
'+' to be between '1' and '9' (non-zero) and enforce a maximum length of 15
digits in addition to the existing minimum of 7, while still ensuring all
characters are ASCII digits; update any unit tests referencing is_valid_e164 to
add cases for '+01234567' (should be invalid) and a >15-digit number (should be
invalid) and keep valid examples within 7–15 digits starting with 1–9.
---
Outside diff comments:
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 187-191: The current check uses adapter.starts_with("signal")
which is too permissive; update the predicate in the
self.current_adapter.as_ref().filter(...) used alongside
parse_implicit_signal_shorthand(&args.target, current_adapter) to only allow
exact "signal" or the namespaced form "signal:..." (e.g., adapter == "signal" ||
adapter.starts_with("signal:")), so non-Signal adapters like "signal-alerts"
won't trigger Signal shorthand parsing in current_adapter /
parse_implicit_signal_shorthand.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac7970c0-8a54-476f-9f64-842e23d52d5b
📒 Files selected for processing (7)
interface/src/api/client.tsinterface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxinterface/src/lib/platformIcons.tsxinterface/src/routes/Settings.tsxsrc/api/messaging.rssrc/tools/send_message_to_another_channel.rs
ibhagwan
changed the title
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
6 times, most recently
from
e771158 to
eb9d9f3
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/messaging/target.rs (2)
337-340:⚠️ Potential issue | 🟠 MajorValidate bare Signal phone numbers with the same E.164 rules.
This branch still accepts values like
01234567and1234567890123456, because it only checks for ASCII digits andlen >= 7before prepending+. That lets malformed Signal targets passparse_delivery_target()even though the rest of the PR tightened E.164 handling.Suggested fix
- // Check if it's a bare phone number (7+ digits required for E.164) - if target.chars().all(|c| c.is_ascii_digit()) && target.len() >= 7 { - return Some(format!("+{target}")); + // Check if it's a bare phone number and normalize only if it becomes valid E.164 + if target.chars().all(|c| c.is_ascii_digit()) { + let normalized = format!("+{target}"); + if is_valid_e164(&normalized) { + return Some(normalized); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 337 - 340, The bare-digit branch in parse_delivery_target accepts malformed numbers; change the check in the block that currently tests target.chars().all(|c| c.is_ascii_digit()) && target.len() >= 7 to enforce E.164 rules: ensure all characters are ASCII digits, length is between 7 and 15 inclusive, and the first digit is not '0' (reject leading zero country codes) before returning Some(format!("+{target}")). Update the logic around the target variable in parse_delivery_target to use these stricter validations.
403-456:⚠️ Potential issue | 🟠 MajorReject empty Signal instance and target segments here.
These match arms currently build
BroadcastTargets for malformed inputs likesignal:uuid:,signal:group:, andsignal::+15551234567. That moves bad delivery targets past validation and into later send-time failures instead of failing at parse time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 403 - 456, The parsing arms for Signal targets (the match that constructs BroadcastTarget instances using normalize_signal_target and direct formats) currently accept empty path segments like uuid/group/instance parts; update each arm to reject empty segments by returning None when an identifier is empty. Concretely: for the arms ["uuid", uuid], ["group", group_id], [instance, "uuid", uuid], and [instance, "group", group_id] add an early guard returning None if uuid/is_empty()/group_id.is_empty(); for the ["e164", phone], [instance, "e164", phone], [phone] if phone.starts_with('+'), and [instance, phone] if phone.starts_with('+') arms return None when phone.is_empty() (or otherwise invalid) before calling normalize_signal_target; and for the single-part arms [single] and [instance, single] return None when single.is_empty() instead of delegating to normalize_signal_target. Reference symbols: BroadcastTarget, normalize_signal_target and the match arms that use patterns ["uuid", uuid], ["group", group_id], ["e164", phone], [phone] if phone.starts_with('+'), [single], [instance, "uuid", uuid], [instance, "group", group_id], [instance, "e164", phone], [instance, phone] if phone.starts_with('+'), and [instance, single].
♻️ Duplicate comments (1)
src/api/messaging.rs (1)
1204-1207:⚠️ Potential issue | 🟠 MajorSignal creation is enabled here, but re-enable still looks like a no-op.
This allowlist now lets the web flow create Signal instances, but
toggle_platform()still has no"signal"start branch. After a Signal adapter is disabled, turning it back on only flips config; no runtime adapter is registered on the same path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1204 - 1207, The allowlist check in messaging.rs now permits "signal" (platform.as_str()), but toggle_platform() lacks a "signal" start branch so re-enabling Signal only flips config and doesn't register the runtime adapter; update toggle_platform() to handle the "signal" case by instantiating and registering the Signal adapter when starting (mirror the existing branches for "discord"/"slack" etc.), using the same adapter registration path and error handling as other platforms (refer to toggle_platform() and platform.as_str() usage) so disabling and re-enabling Signal properly tears down and recreates the runtime adapter.
🧹 Nitpick comments (1)
src/tools/cron.rs (1)
431-441: Add a different-instance regression test.The suite covers same-instance qualified targets, but not an explicit
signal:other_instance:...target created from a different current instance. That is the case most likely to get silently rewritten if parser behavior changes.Suggested test
+ #[test] + fn test_normalize_explicit_other_instance_no_rewrite() { + let result = normalize_delivery_target( + "signal:personal:uuid:550e8400-e29b-41d4-a716-446655440000", + &Some("signal:work".to_string()), + ); + assert_eq!( + result, + "signal:personal:uuid:550e8400-e29b-41d4-a716-446655440000" + ); + } + #[test] fn test_normalize_already_qualified_no_rewrite() {Also applies to: 530-541
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/cron.rs` around lines 431 - 441, Add a regression test to cover the case where normalize_delivery_target is given a delivery_target that already names a different instance (e.g., "signal:other_instance:...") while current_adapter is a different named instance; ensure the test asserts that normalize_delivery_target does NOT rewrite the other-instance target and returns the original string. Locate normalize_delivery_target in src/tools/cron.rs and add a unit test (or expand the existing test module around the tests covering same-instance qualified targets near the other similar case at lines ~530-541) that passes a parsed-style target produced by crate::messaging::target::parse_delivery_target and a different current_adapter value, then assert equality with the original delivery_target.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 651-658: Update the frontend E.164 validation to match the backend
by rejecting numbers that start with 0 and enforcing the 8–15 digit length
(excluding the '+'). Replace the current e164Regex used around
credentialInputs.signal_account with a pattern that requires a '+' then a
non-zero first digit and a total of 8–15 digits (e.g., use a regex equivalent to
/^\+[1-9]\d{7,14}$/), and keep the same setMessage error text; apply the same
change to the other occurrence noted (around lines 975-976) so both client-side
checks mirror the backend rule.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 198-203: The current check allows default Signal adapters to call
parse_implicit_signal_shorthand for bare +... targets, which uses a laxer
parser; update the call site and the helper so implicit shorthand uses the same
validation/normalization as explicit targets by replacing
parse_implicit_signal_shorthand usage with logic that invokes is_valid_e164()
and normalize_signal_target() (or change parse_implicit_signal_shorthand to
internally call is_valid_e164() then normalize_signal_target()) so inputs like
"+01234567" and overly long numbers are rejected consistently; ensure references
to current_adapter and args.target remain but flow through the stricter
validation/normalization functions (preserve function names
parse_implicit_signal_shorthand, is_valid_e164, normalize_signal_target as
anchors).
- Around line 146-170: The code leaves target.adapter == "signal" when called
outside a Signal conversation and multiple named Signal adapters exist, causing
broadcast() to fail with "no messaging adapter named 'signal'"; update the logic
in send_message_to_another_channel (the block handling target.adapter ==
"signal", referencing current_adapter and messaging_manager.adapter_names()) to
detect the ambiguous case (signal_adapters.len() != 1 and no adapter named
exactly "signal") and return an early Err (or a recoverable error result) with a
clear message about multiple named Signal adapters and asking the caller to
specify one, instead of leaving target.adapter as "signal" and deferring to
broadcast().
---
Outside diff comments:
In `@src/messaging/target.rs`:
- Around line 337-340: The bare-digit branch in parse_delivery_target accepts
malformed numbers; change the check in the block that currently tests
target.chars().all(|c| c.is_ascii_digit()) && target.len() >= 7 to enforce E.164
rules: ensure all characters are ASCII digits, length is between 7 and 15
inclusive, and the first digit is not '0' (reject leading zero country codes)
before returning Some(format!("+{target}")). Update the logic around the target
variable in parse_delivery_target to use these stricter validations.
- Around line 403-456: The parsing arms for Signal targets (the match that
constructs BroadcastTarget instances using normalize_signal_target and direct
formats) currently accept empty path segments like uuid/group/instance parts;
update each arm to reject empty segments by returning None when an identifier is
empty. Concretely: for the arms ["uuid", uuid], ["group", group_id], [instance,
"uuid", uuid], and [instance, "group", group_id] add an early guard returning
None if uuid/is_empty()/group_id.is_empty(); for the ["e164", phone], [instance,
"e164", phone], [phone] if phone.starts_with('+'), and [instance, phone] if
phone.starts_with('+') arms return None when phone.is_empty() (or otherwise
invalid) before calling normalize_signal_target; and for the single-part arms
[single] and [instance, single] return None when single.is_empty() instead of
delegating to normalize_signal_target. Reference symbols: BroadcastTarget,
normalize_signal_target and the match arms that use patterns ["uuid", uuid],
["group", group_id], ["e164", phone], [phone] if phone.starts_with('+'),
[single], [instance, "uuid", uuid], [instance, "group", group_id], [instance,
"e164", phone], [instance, phone] if phone.starts_with('+'), and [instance,
single].
---
Duplicate comments:
In `@src/api/messaging.rs`:
- Around line 1204-1207: The allowlist check in messaging.rs now permits
"signal" (platform.as_str()), but toggle_platform() lacks a "signal" start
branch so re-enabling Signal only flips config and doesn't register the runtime
adapter; update toggle_platform() to handle the "signal" case by instantiating
and registering the Signal adapter when starting (mirror the existing branches
for "discord"/"slack" etc.), using the same adapter registration path and error
handling as other platforms (refer to toggle_platform() and platform.as_str()
usage) so disabling and re-enabling Signal properly tears down and recreates the
runtime adapter.
---
Nitpick comments:
In `@src/tools/cron.rs`:
- Around line 431-441: Add a regression test to cover the case where
normalize_delivery_target is given a delivery_target that already names a
different instance (e.g., "signal:other_instance:...") while current_adapter is
a different named instance; ensure the test asserts that
normalize_delivery_target does NOT rewrite the other-instance target and returns
the original string. Locate normalize_delivery_target in src/tools/cron.rs and
add a unit test (or expand the existing test module around the tests covering
same-instance qualified targets near the other similar case at lines ~530-541)
that passes a parsed-style target produced by
crate::messaging::target::parse_delivery_target and a different current_adapter
value, then assert equality with the original delivery_target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b389d9e-75c2-482c-8cff-5005a5ba96b1
📒 Files selected for processing (12)
interface/src/api/client.tsinterface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxinterface/src/lib/platformIcons.tsxinterface/src/routes/Settings.tsxprompts/en/adapters/cron.md.j2src/api/messaging.rssrc/cron/scheduler.rssrc/messaging/target.rssrc/tools.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- interface/src/api/client.ts
- interface/src/lib/platformIcons.tsx
- interface/src/routes/Settings.tsx
- interface/src/components/ChannelEditModal.tsx
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/api/messaging.rs (1)
521-586:⚠️ Potential issue | 🟠 Major
messaging_status()still drops Signal’s platform status.This builds
PlatformStatusinto_signal_statusand then throws it away, so the response never exposes Signal’s configured/enabled state. The instances list helps, but the top-level status contract is still incomplete for the web UI.Suggested fix
pub(super) struct MessagingStatusResponse { discord: PlatformStatus, slack: PlatformStatus, telegram: PlatformStatus, email: PlatformStatus, webhook: PlatformStatus, twitch: PlatformStatus, + signal: PlatformStatus, instances: Vec<AdapterInstanceStatus>, } - let (discord, slack, telegram, email, webhook, twitch, instances) = if config_path.exists() { + let (discord, slack, telegram, email, webhook, twitch, signal, instances) = if config_path.exists() { ... - let _signal_status = doc + let signal_status = doc .get("messaging") .and_then(|m| m.get("signal")) ... ( discord_status, slack_status, telegram_status, email_status, webhook_status, twitch_status, + signal_status, instances, ) } else { let default = PlatformStatus { configured: false, enabled: false, }; ( default.clone(), default.clone(), default.clone(), default.clone(), default.clone(), - default, + default.clone(), + default, Vec::new(), ) }; Ok(Json(MessagingStatusResponse { discord, slack, telegram, email, webhook, twitch, + signal, instances, }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 521 - 586, The code builds Signal's PlatformStatus into the variable `_signal_status` and then drops it, so change the binding to keep it (e.g., `signal_status` instead of `_signal_status`) and ensure that `signal_status` is included in the function's returned/top-level status map so the web UI sees Signal's configured/enabled state; update the `messaging_status()` logic that constructs PlatformStatus (the closure that currently returns PlatformStatus and calls `push_instance_status`) to return and propagate `signal_status` into the final response alongside the existing instances, not just rely on `instances` populated by `push_instance_status`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/messaging.rs`:
- Around line 1167-1180: The code currently creates a new ArcSwap and calls
state.set_signal_permissions(...) inside the toggle path (in the block that
constructs SignalPermissions from_config), which causes the API to miss the
startup adapter's swap; instead, seed ApiState.signal_permissions at startup and
reuse that same ArcSwap here: check for an existing ArcSwap in ApiState
(state.signal_permissions) and, if present, update its pointee rather than
creating a new ArcSwap; if not present (startup fallback) then create and set it
as you do now. Refer to ApiState.signal_permissions, set_signal_permissions,
SignalPermissions::from_config and the startup swap created in main.rs when
wiring the swap through to the ApiState so the same arc_swap instance is used by
both the adapter and the API.
In `@src/messaging/target.rs`:
- Around line 273-294: Update the E.164 validator to match the frontend by
requiring at least 8 digits after '+': in is_valid_e164 change the minimum check
from digits.len() < 7 to digits.len() < 8 (so it enforces /^\+[1-9]\d{7,14}$/);
then update downstream expectations that rely on this rule — adjust tests in the
messaging API test suite (references in api messaging tests) and change the
hardcoded shorthand error string used in send_message_to_another_channel to
reflect the new "8 digits" minimum.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 146-175: The code incorrectly treats the presence of the plain
"signal" adapter as an ambiguity when combined with named adapters; change the
logic in the block that collects signal_adapters (from
self.messaging_manager.adapter_names()) to detect whether the default "signal"
adapter exists and count only named adapters (those starting with "signal:"). If
"signal" exists, do not error (leave target.adapter as "signal"); otherwise, if
exactly one named adapter exists, set target.adapter to that named instance; if
more than one named adapter exists and no plain "signal" present, return the
ambiguity SendMessageError. Update the check around target.adapter,
self.current_adapter, signal_adapters, and the error branch accordingly so
broadcast() can still perform exact lookup when "signal" is present.
---
Duplicate comments:
In `@src/api/messaging.rs`:
- Around line 521-586: The code builds Signal's PlatformStatus into the variable
`_signal_status` and then drops it, so change the binding to keep it (e.g.,
`signal_status` instead of `_signal_status`) and ensure that `signal_status` is
included in the function's returned/top-level status map so the web UI sees
Signal's configured/enabled state; update the `messaging_status()` logic that
constructs PlatformStatus (the closure that currently returns PlatformStatus and
calls `push_instance_status`) to return and propagate `signal_status` into the
final response alongside the existing instances, not just rely on `instances`
populated by `push_instance_status`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5308c193-efb4-4dd4-8f9e-8ccae4d8da39
📒 Files selected for processing (5)
interface/src/components/ChannelSettingCard.tsxsrc/api/messaging.rssrc/api/state.rssrc/messaging/target.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- interface/src/components/ChannelSettingCard.tsx
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
2 times, most recently
from
68aef7a to
cb44a63
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/messaging/target.rs (1)
273-284:⚠️ Potential issue | 🟠 MajorTighten the E.164 lower bound to match the web flow.
Line 283 still accepts numbers with only 7 digits after
+(e.g.+1234567). That is looser than the Signal web validation introduced in this PR, so API/tool parsing can accept values the form rejects.Suggested fix
/// Requirements: /// - Must start with '+' /// - First digit after '+' must be 1-9 (not 0) -/// - Minimum 7 digits total (6 after '+') -/// - Maximum 15 digits total (14 after '+', E.164 standard) +/// - Minimum 8 digits after '+' +/// - Maximum 15 digits after '+' (E.164 standard) /// - All characters after '+' must be ASCII digits pub fn is_valid_e164(phone: &str) -> bool { if let Some(digits) = phone.strip_prefix('+') { - if digits.len() < 7 || digits.len() > 15 { + if digits.len() < 8 || digits.len() > 15 { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 273 - 284, The is_valid_e164 function currently allows phone numbers with 7 digits after '+', which is too permissive; update the length check in is_valid_e164 from digits.len() < 7 to digits.len() < 8 so it requires at least 8 digits after '+' (and keep the existing upper bound of 15), and update the function doc comment/requirements to reflect the new minimum (minimum 8 digits after '+' / 9 digits total if you prefer consistent wording) so the implementation and docs match the tightened web-flow validation.
🧹 Nitpick comments (1)
src/api/messaging.rs (1)
1439-1445: Avoid abbreviated binding names (acc) in credential parsing.Please use a full variable name for readability and guideline compliance.
Suggested rename
- Some(acc) => acc, + Some(account_value) => account_value,As per coding guidelines "Don't abbreviate variable names. Use
queuenotq,messagenotmsg,channelnotch. Common abbreviations likeconfigare fine".Also applies to: 1636-1642
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1439 - 1445, Replace the abbreviated binding name "acc" used when matching credentials.signal_account with a descriptive name like "signal_account" (or "signal_account_ref") to improve readability and follow guidelines; update the match arm in the parsing logic where credentials.signal_account.as_ref().filter(|s| !s.is_empty()) is matched (and the similar occurrence around the other match at the later location) so the Some(...) binding uses the full variable name and adjust any subsequent uses in that scope accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/messaging.rs`:
- Around line 1425-1431: The code currently accepts signal_http_url if non-empty
but doesn't reject whitespace-only or malformed URLs; update the handling around
credentials.signal_http_url (used in both the default Signal branch and the
named Signal branch) to first trim() the string, reject if empty after trim,
then attempt to parse/normalize it (e.g., using Url::parse or equivalent) and
ensure it has an allowed scheme (http or https) before assigning/persisting; on
parse or validation failure return or log an explicit error so
malformed/whitespace URLs are never written to config.
---
Duplicate comments:
In `@src/messaging/target.rs`:
- Around line 273-284: The is_valid_e164 function currently allows phone numbers
with 7 digits after '+', which is too permissive; update the length check in
is_valid_e164 from digits.len() < 7 to digits.len() < 8 so it requires at least
8 digits after '+' (and keep the existing upper bound of 15), and update the
function doc comment/requirements to reflect the new minimum (minimum 8 digits
after '+' / 9 digits total if you prefer consistent wording) so the
implementation and docs match the tightened web-flow validation.
---
Nitpick comments:
In `@src/api/messaging.rs`:
- Around line 1439-1445: Replace the abbreviated binding name "acc" used when
matching credentials.signal_account with a descriptive name like
"signal_account" (or "signal_account_ref") to improve readability and follow
guidelines; update the match arm in the parsing logic where
credentials.signal_account.as_ref().filter(|s| !s.is_empty()) is matched (and
the similar occurrence around the other match at the later location) so the
Some(...) binding uses the full variable name and adjust any subsequent uses in
that scope accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c20ecf93-ee6a-4941-b0c1-11da0964f9fb
📒 Files selected for processing (5)
interface/src/components/ChannelSettingCard.tsxsrc/api/messaging.rssrc/api/state.rssrc/messaging/target.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- interface/src/components/ChannelSettingCard.tsx
- src/api/state.rs
- Add signal_http_url and signal_account credential fields - Add E.164 phone number validation for Signal accounts - Auto-populate dm_allowed_users with Signal account on instance creation - Support both default and named Signal adapter instances - Add unit tests for E.164 validation Validation ensures phone numbers are in E.164 format (+ followed by 7+ digits)
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
from
cb44a63 to
6a5ca6f
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
from
6a5ca6f to
5d946fe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
interface/src/components/ChannelSettingCard.tsx (1)
975-977:⚠️ Potential issue | 🟡 MinorAlign helper text with the actual non-zero first-digit validation rule.
The field hint currently omits the
first digit must be 1-9rule, which can mislead users entering+0....Suggested text update
- Your Signal phone number in E.164 format (+ followed by 6-14 digits) + Your Signal phone number in E.164 format (+ followed by 6-14 digits, first digit 1-9)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ChannelSettingCard.tsx` around lines 975 - 977, Update the helper text shown in the ChannelSettingCard component to reflect the actual validation rule that the first digit after the "+" cannot be zero; specifically change the copy near the phone number hint (the <p> element in ChannelSettingCard.tsx that currently reads "Your Signal phone number in E.164 format (+ followed by 6-14 digits)") to include "first digit must be 1-9" (e.g., "Your Signal phone number in E.164 format (+ followed by 6–14 digits, first digit must be 1–9)") so the displayed hint matches the non-zero first-digit validation enforced by the form.src/api/messaging.rs (1)
1425-1431:⚠️ Potential issue | 🟠 MajorTrim and validate
signal_http_urlbefore persisting.The current checks still accept whitespace-only and malformed values, which can be saved and fail later during adapter start.
Suggested fix pattern (apply in both default + named Signal branches)
- let http_url = match credentials.signal_http_url.as_ref().filter(|s| !s.is_empty()) { + let http_url = match credentials + .signal_http_url + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()) + { Some(url) => url, None => { /* validation failure response */ } }; + if !(http_url.starts_with("http://") || http_url.starts_with("https://")) { + return Ok(Json(MessagingInstanceActionResponse { + success: false, + message: "signal: http_url must start with http:// or https://".to_string(), + })); + }Also applies to: 1622-1628
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1425 - 1431, The code allows whitespace-only or malformed signal_http_url to pass through; update the logic that computes http_url from credentials.signal_http_url to first trim the string (e.g., via .trim()) and ensure it is non-empty, then validate/parse it (e.g., Url::parse or reqwest::Url::parse) and only accept the value if parsing succeeds; if parsing fails, treat it as None/error as appropriate. Apply the exact same trimming+parsing fix in both the default and named Signal branches where signal_http_url is used (the code paths that assign to http_url from credentials.signal_http_url).src/messaging/target.rs (1)
273-294:⚠️ Potential issue | 🟠 MajorAlign E.164 bounds with the web form.
is_valid_e164()currently allows+123456and rejects 15-digit numbers, so backend validation diverges from the frontend constraint (^\+[1-9]\d{7,14}$).Suggested fix
- if digits.len() < 6 || digits.len() > 14 { + if digits.len() < 8 || digits.len() > 15 { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 273 - 294, The validation in is_valid_e164 currently allows 6–14 digits after '+' but the frontend expects 8–15 digits after '+' (regex ^\+[1-9]\d{7,14}$), so update the length bounds: change the length check in is_valid_e164 to require digits.len() >= 8 && digits.len() <= 15 (i.e. return false if digits.len() < 8 || digits.len() > 15), keep the first-digit non-'0' check (digits.chars().next()) and the digits.chars().all(|c| c.is_ascii_digit()) check unchanged.
🧹 Nitpick comments (1)
src/api/messaging.rs (1)
1444-1445: Prefer full variable names in new branches.
accis abbreviated; use a full name (account_value,account_input, etc.) for readability and consistency.As per coding guidelines: "Don't abbreviate variable names. Use
queuenotq,messagenotmsg,channelnotch."Also applies to: 1641-1642
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1444 - 1445, In the match arms where the variable is named `acc` (e.g., the branch showing `Some(acc) => acc, None => {`), rename `acc` to a full, descriptive identifier such as `account_value` or `account_input` to follow the no-abbreviation guideline; update both the `Some(...)` pattern and any downstream uses so the return and pattern remain consistent. Apply the same renaming to the other occurrence around the 1641-1642 region so both match arms and their uses use the same descriptive name (`account_value`/`account_input`) for readability and consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/cron.rs`:
- Around line 431-439: normalize_delivery_target currently rewrites
delivery_target whenever current_adapter appears to be a named instance of
parsed.adapter, but parse_delivery_target only preserves named-instance
semantics for Signal, which can misroute other platforms; update
normalize_delivery_target so the rewrite is only applied when parsed.adapter
indicates the Signal platform (e.g., parsed.adapter == "signal" or the SIGNAL
adapter constant) in addition to the existing current_adapter check, so this
function only substitutes named Signal instances until generic named-instance
parsing is implemented in parse_delivery_target.
---
Duplicate comments:
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 975-977: Update the helper text shown in the ChannelSettingCard
component to reflect the actual validation rule that the first digit after the
"+" cannot be zero; specifically change the copy near the phone number hint (the
<p> element in ChannelSettingCard.tsx that currently reads "Your Signal phone
number in E.164 format (+ followed by 6-14 digits)") to include "first digit
must be 1-9" (e.g., "Your Signal phone number in E.164 format (+ followed by
6–14 digits, first digit must be 1–9)") so the displayed hint matches the
non-zero first-digit validation enforced by the form.
In `@src/api/messaging.rs`:
- Around line 1425-1431: The code allows whitespace-only or malformed
signal_http_url to pass through; update the logic that computes http_url from
credentials.signal_http_url to first trim the string (e.g., via .trim()) and
ensure it is non-empty, then validate/parse it (e.g., Url::parse or
reqwest::Url::parse) and only accept the value if parsing succeeds; if parsing
fails, treat it as None/error as appropriate. Apply the exact same
trimming+parsing fix in both the default and named Signal branches where
signal_http_url is used (the code paths that assign to http_url from
credentials.signal_http_url).
In `@src/messaging/target.rs`:
- Around line 273-294: The validation in is_valid_e164 currently allows 6–14
digits after '+' but the frontend expects 8–15 digits after '+' (regex
^\+[1-9]\d{7,14}$), so update the length bounds: change the length check in
is_valid_e164 to require digits.len() >= 8 && digits.len() <= 15 (i.e. return
false if digits.len() < 8 || digits.len() > 15), keep the first-digit non-'0'
check (digits.chars().next()) and the digits.chars().all(|c| c.is_ascii_digit())
check unchanged.
---
Nitpick comments:
In `@src/api/messaging.rs`:
- Around line 1444-1445: In the match arms where the variable is named `acc`
(e.g., the branch showing `Some(acc) => acc, None => {`), rename `acc` to a
full, descriptive identifier such as `account_value` or `account_input` to
follow the no-abbreviation guideline; update both the `Some(...)` pattern and
any downstream uses so the return and pattern remain consistent. Apply the same
renaming to the other occurrence around the 1641-1642 region so both match arms
and their uses use the same descriptive name (`account_value`/`account_input`)
for readability and consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8069f225-dbea-4331-b01c-8c0be86d3512
📒 Files selected for processing (13)
interface/src/api/client.tsinterface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxinterface/src/lib/platformIcons.tsxinterface/src/routes/Settings.tsxprompts/en/adapters/cron.md.j2src/api/messaging.rssrc/api/state.rssrc/cron/scheduler.rssrc/messaging/target.rssrc/tools.rssrc/tools/cron.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/cron/scheduler.rs
- interface/src/api/client.ts
- interface/src/routes/Settings.tsx
- prompts/en/adapters/cron.md.j2
- src/tools.rs
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
2 times, most recently
from
fc9a785 to
7954430
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/api/messaging.rs (1)
1442-1530: Extract shared Signal credential parsing/validation into one helper.The default and named branches duplicate nearly identical Signal URL/account/DM parsing logic. A shared helper would reduce drift risk and keep error text behavior aligned.
Also applies to: 1640-1728
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1442 - 1530, The Signal branch repeats URL/account/DM parsing logic—extract this into a single helper (e.g., parse_signal_credentials) that takes the credentials struct and returns a Result containing the normalized http_url (no trailing slash), validated account, and an optional Vec<String> of dm_allowed_users or an Err(MessagingInstanceActionResponse) on validation failure; reuse this helper in both the "signal" default and named branches to replace the duplicated blocks that reference signal_http_url, signal_account, signal_dm_allowed_users, is_valid_e164, and the existing tracing::warn / reqwest::Url::parse checks, and then write the returned values into platform_table["http_url"], ["account"], and ["dm_allowed_users"] exactly as the original code did (preserve error messages and logging).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 683-686: The validation error string is hardcoded when calling
setMessage({ text: `Invalid phone number(s): ${invalid_users.join(', ')}. Must
be E.164 format (+ followed by 6-15 digits).`, ... }), which can diverge from
the shared E164_ERROR_TEXT; update this to reuse the shared constant by
composing the message using E164_ERROR_TEXT (e.g., `Invalid phone number(s):
${invalid_users.join(', ')}. ${E164_ERROR_TEXT}`) so ChannelSettingCard's DM
allow-list validation message stays consistent with the shared copy.
In `@src/api/messaging.rs`:
- Around line 1174-1222: When request.adapter is None and you enable Signal, you
only start the default adapter; to be symmetric with the disable path you must
also start all enabled named Signal instances. After the existing
default-adapter startup (SignalAdapter::new + manager.register_and_start),
iterate the collection of configured Signal entries (e.g., the state's signal
configs/named configs), and for each config that is enabled and not the default,
construct a SignalAdapter::new with that config's
name/http_url/account/ignore_stories and the computed permissions/tmp_dir, then
call manager.register_and_start for each (skipping ones already running). Keep
using the same permissions (from state.signal_permissions) and
instance_dir/tmp_dir logic and reuse register_and_start error handling
(tracing::error!) as shown for the default adapter.
---
Nitpick comments:
In `@src/api/messaging.rs`:
- Around line 1442-1530: The Signal branch repeats URL/account/DM parsing
logic—extract this into a single helper (e.g., parse_signal_credentials) that
takes the credentials struct and returns a Result containing the normalized
http_url (no trailing slash), validated account, and an optional Vec<String> of
dm_allowed_users or an Err(MessagingInstanceActionResponse) on validation
failure; reuse this helper in both the "signal" default and named branches to
replace the duplicated blocks that reference signal_http_url, signal_account,
signal_dm_allowed_users, is_valid_e164, and the existing tracing::warn /
reqwest::Url::parse checks, and then write the returned values into
platform_table["http_url"], ["account"], and ["dm_allowed_users"] exactly as the
original code did (preserve error messages and logging).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3c63926-65c8-4f40-a98d-ce0f9b5005b6
📒 Files selected for processing (12)
interface/src/api/client.tsinterface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxinterface/src/lib/format.tsinterface/src/lib/platformIcons.tsxinterface/src/routes/Settings.tsxsrc/api/messaging.rssrc/api/state.rssrc/main.rssrc/messaging/signal.rssrc/messaging/target.rssrc/tools/send_message_to_another_channel.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main.rs
- interface/src/api/client.ts
- src/messaging/signal.rs
- interface/src/routes/Settings.tsx
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
3 times, most recently
from
e9ea83a to
cbd8728
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
interface/src/components/ChannelSettingCard.tsx (1)
987-1005: Consider adding Enter-to-submit on Signal HTTP URL input for consistency.Other credential fields in this form support Enter submit; adding it here improves keyboard flow.
💡 Optional tweak
<Input size="lg" value={credentialInputs.signal_http_url ?? ""} onChange={(e) => setCredentialInputs({...credentialInputs, signal_http_url: e.target.value})} placeholder="http://127.0.0.1:8686" + onKeyDown={(e) => { if (e.key === "Enter") handleSave(); }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ChannelSettingCard.tsx` around lines 987 - 1005, The Signal HTTP URL Input for credentialInputs.signal_http_url is missing an Enter key handler; add an onKeyDown={(e) => { if (e.key === "Enter") handleSave(); }} prop to that Input (the same pattern used on the signal_account Input) so pressing Enter while focused on the signal_http_url field triggers the existing handleSave and keeps keyboard behavior consistent; update the Input where value is credentialInputs.signal_http_url and onChange calls setCredentialInputs accordingly.src/api/messaging.rs (1)
265-273: Consider validating dm_allowed_users entries.The dm_allowed_users are parsed and stored without validation. While Signal targets can be various formats (uuid:, group:, E.164 phone numbers), completely invalid or empty entries would be silently accepted and persisted.
Optional: Add basic validation for dm_allowed_users entries
// Parse dm_allowed_users if provided let dm_users = credentials.signal_dm_allowed_users.as_ref().map(|dm_str| { dm_str .split(',') .map(|s| s.trim()) .filter(|s| !s.is_empty()) + .filter(|s| { + // Accept common Signal target formats + s.starts_with('+') || s.starts_with("uuid:") || s.starts_with("group:") + }) .map(|s| s.to_string()) .collect::<Vec<String>>() });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 265 - 273, The dm_allowed_users parsing (credentials.signal_dm_allowed_users -> dm_users) currently accepts any non-empty trimmed string; add validation to reject or normalize invalid entries before collecting: validate each entry against allowed Signal target patterns (e.g., starts_with("uuid:"), starts_with("group:"), or a basic E.164 regex for phone numbers) and skip or return an error for entries that fail validation; update the parsing closure used to build dm_users to filter-invalid or map to a Result so callers can handle invalid input, and ensure the final dm_users is Vec<String> only of validated entries (or propagate a clear validation error).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 663-693: The Signal allow-list is not preserved in the edit flow
because ChannelEditModal currently returns only signal_http_url and
signal_account; update the save/submit logic in the ChannelEditModal component
(the function that builds the credentials object returned on edit) to also
include signal_dm_allowed_users (e.g., credentials.signal_dm_allowed_users =
credentialInputs.signal_dm_allowed_users ||
existingCredentials.signal_dm_allowed_users) so the allow-list is carried
through when empty inputs are omitted, and ensure the field name matches
ChannelSettingCard's usage (signal_dm_allowed_users) so edits don't drop the
allow-list.
---
Nitpick comments:
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 987-1005: The Signal HTTP URL Input for
credentialInputs.signal_http_url is missing an Enter key handler; add an
onKeyDown={(e) => { if (e.key === "Enter") handleSave(); }} prop to that Input
(the same pattern used on the signal_account Input) so pressing Enter while
focused on the signal_http_url field triggers the existing handleSave and keeps
keyboard behavior consistent; update the Input where value is
credentialInputs.signal_http_url and onChange calls setCredentialInputs
accordingly.
In `@src/api/messaging.rs`:
- Around line 265-273: The dm_allowed_users parsing
(credentials.signal_dm_allowed_users -> dm_users) currently accepts any
non-empty trimmed string; add validation to reject or normalize invalid entries
before collecting: validate each entry against allowed Signal target patterns
(e.g., starts_with("uuid:"), starts_with("group:"), or a basic E.164 regex for
phone numbers) and skip or return an error for entries that fail validation;
update the parsing closure used to build dm_users to filter-invalid or map to a
Result so callers can handle invalid input, and ensure the final dm_users is
Vec<String> only of validated entries (or propagate a clear validation error).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f68cfd4-efa0-4213-9539-3447f799152a
📒 Files selected for processing (2)
interface/src/components/ChannelSettingCard.tsxsrc/api/messaging.rs
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
from
cbd8728 to
84ca00a
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
interface/src/components/ChannelSettingCard.tsx (1)
644-647: Consider validatingsignal_http_urlformat client-side before submit.Right now only non-empty is enforced (Line 644). Adding a lightweight URL parse check would catch typos earlier and reduce failed round-trips.
Possible tweak
} else if (platform === "signal") { - if (!credentialInputs.signal_http_url?.trim()) { + const rawSignalUrl = credentialInputs.signal_http_url?.trim(); + if (!rawSignalUrl) { setMessage({text: "HTTP URL is required", type: "error"}); return; } + try { + const parsed = new URL(rawSignalUrl); + if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { + throw new Error("invalid protocol"); + } + credentials.signal_http_url = parsed.toString().replace(/\/$/, ""); + } catch { + setMessage({text: "HTTP URL must be a valid http(s) URL", type: "error"}); + return; + } if (!credentialInputs.signal_account?.trim()) { setMessage({text: "Account phone number is required", type: "error"}); return; } @@ - credentials.signal_http_url = credentialInputs.signal_http_url.trim(); credentials.signal_account = account;Also applies to: 661-662
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ChannelSettingCard.tsx` around lines 644 - 647, Validate credentialInputs.signal_http_url with a lightweight client-side URL check before submit: in the save/submit handler(s) where credentialInputs.signal_http_url is currently checked (the block using setMessage({text: "HTTP URL is required", type: "error"})), add a parse/regex check to ensure it is a well-formed HTTP/HTTPS URL (e.g., ensure it starts with http:// or https:// and can be parsed), and if invalid call setMessage with an appropriate error text and return; apply the same validation in the other usage referenced around the credentialInputs.signal_http_url check at the second location (the lines around 661-662) so both submit paths enforce the URL format.src/api/messaging.rs (1)
274-281: Consider validating uuid:/group: prefix content.The validation accepts entries like
"uuid:"or"group:"with nothing after the prefix. While the adapter will likely reject these at runtime, adding a minimum-content check would provide better immediate user feedback.♻️ Optional enhancement
// Valid formats: uuid:xxx, group:xxx, +E.164 - !(entry.starts_with("uuid:") || entry.starts_with("group:") || is_valid_e164(entry)) + !(entry.strip_prefix("uuid:").is_some_and(|s| !s.is_empty()) + || entry.strip_prefix("group:").is_some_and(|s| !s.is_empty()) + || is_valid_e164(entry))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 274 - 281, The current filter that computes invalid_entries accepts "uuid:" or "group:" with no content; update the predicate used in the invalid_entries computation to require non-empty content after the "uuid:" and "group:" prefixes (e.g., check entry.len() > "uuid:".len() / "group:".len() and that the substring after the prefix is not all-whitespace) in addition to the existing is_valid_e164 check so entries like "uuid:" or "group:" are treated as invalid; modify the closure where entries.iter().filter(...) is used and reference the same identifiers (entries, invalid_entries, is_valid_e164) when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@interface/src/components/ChannelSettingCard.tsx`:
- Around line 644-647: Validate credentialInputs.signal_http_url with a
lightweight client-side URL check before submit: in the save/submit handler(s)
where credentialInputs.signal_http_url is currently checked (the block using
setMessage({text: "HTTP URL is required", type: "error"})), add a parse/regex
check to ensure it is a well-formed HTTP/HTTPS URL (e.g., ensure it starts with
http:// or https:// and can be parsed), and if invalid call setMessage with an
appropriate error text and return; apply the same validation in the other usage
referenced around the credentialInputs.signal_http_url check at the second
location (the lines around 661-662) so both submit paths enforce the URL format.
In `@src/api/messaging.rs`:
- Around line 274-281: The current filter that computes invalid_entries accepts
"uuid:" or "group:" with no content; update the predicate used in the
invalid_entries computation to require non-empty content after the "uuid:" and
"group:" prefixes (e.g., check entry.len() > "uuid:".len() / "group:".len() and
that the substring after the prefix is not all-whitespace) in addition to the
existing is_valid_e164 check so entries like "uuid:" or "group:" are treated as
invalid; modify the closure where entries.iter().filter(...) is used and
reference the same identifiers (entries, invalid_entries, is_valid_e164) when
making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd71a4db-009b-470f-8afe-dfcf5dd63a18
📒 Files selected for processing (3)
interface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxsrc/api/messaging.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- interface/src/components/ChannelEditModal.tsx
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
2 times, most recently
from
7df807b to
e2319a8
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/tools/send_message_to_another_channel.rs (1)
196-205:⚠️ Potential issue | 🟠 MajorKeep explicit
signal:targets on the default adapter when it exists.Inside a named Signal conversation this always rewrites
signal:+.../signal:uuid:...tocurrent_adapter. If bothsignalandsignal:<name>are registered, there is no way to explicitly use the default Signal account anymore.Suggested fix
- if let Some(current_adapter) = self - .current_adapter - .as_ref() - .filter(|adapter| adapter.starts_with("signal:")) - { + if self.messaging_manager.has_adapter("signal").await { + // Keep explicit `signal:<target>` on the default adapter. + } else if let Some(current_adapter) = self + .current_adapter + .as_ref() + .filter(|adapter| adapter.starts_with("signal:")) + { // In a named Signal conversation — use that instance target.adapter = current_adapter.clone(); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/send_message_to_another_channel.rs` around lines 196 - 205, The code overwrites explicit "signal:<...>" targets with the conversation's named adapter by unconditionally replacing target.adapter when self.current_adapter starts_with("signal:"), losing the ability to target the default Signal account; update the logic in the block that examines target.adapter and self.current_adapter so it only substitutes target.adapter with current_adapter when the original target was exactly "signal" (or empty/null), and do not change target.adapter if the original value already started with "signal:" (i.e., preserve explicit "signal:<name|+...|uuid...>" entries); refer to target.adapter and self.current_adapter to implement this conditional check.src/api/messaging.rs (1)
1600-1608:⚠️ Potential issue | 🟠 MajorBlank
signal_dm_allowed_userswon’t clear the saved allow-list.These branches only write
dm_allowed_userswhen the parsed list is non-empty. With the currentChannelEditModal.tsxpayload (signal_dm_allowed_users?.trim() || undefined), clearing the field leaves the existing TOML key untouched, so previously authorized senders remain authorized.Also applies to: 1732-1740
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1600 - 1608, The code only writes dm_allowed_users when dm_users is Some and non-empty, so clearing the input (which becomes None/undefined) leaves the old key intact; update the logic around platform_table["dm_allowed_users"] in the function that builds the TOML so that: if dm_users is Some and not empty, set platform_table["dm_allowed_users"] = toml_edit::value(dm_array); otherwise (dm_users is None or Some but empty) remove the key from platform_table (e.g., platform_table.remove("dm_allowed_users")); apply the same change to the similar block around the other occurrence (lines referenced 1732-1740) so blanking the field actually clears the saved allow-list; reference symbols: platform_table["dm_allowed_users"], dm_users, and signal_dm_allowed_users.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/messaging.rs`:
- Around line 1369-1400: The code starts a named Signal adapter regardless of
the root messaging.signal enabled flag, causing runtime/config desync; update
the branch that constructs and starts SignalAdapter (references: signal_config,
instance.name, crate::config::binding_runtime_adapter_key, SignalAdapter::new,
manager.register_and_start) to first check the root enable flag (e.g.
signal_config.enabled or the existing local enabled variable used elsewhere
where instance status is computed as instance.enabled && enabled) and only
construct/register the adapter when that root enabled is true, so adapter
lifecycle matches the reported instance status and restart behavior.
In `@src/messaging/target.rs`:
- Around line 544-565: The create_messaging_instance() path allows names that
parse_named_instance_target() would reject, causing mismatch between stored
instance names and target parsing; to fix, validate the provided instance name
inside create_messaging_instance() (or call/reuse the existing
is_valid_instance_name(name: &str) from src/messaging/target.rs) and return a
clear validation error when it fails (reject all-digit names, Slack-style IDs
like Txxxxx/Cxxxxx, names >20 chars, or characters outside
alphanumeric/underscore/hyphen), or move the validation into a shared helper
used by both create_messaging_instance() and parse_named_instance_target() so
both code paths enforce the same rules.
- Around line 511-529: The named-adapter match arm is incorrectly treating "dm"
as a valid instance (via is_valid_instance_name), causing "discord:dm:<user>"
and "slack:dm:<user>" to be rewritten; update the condition in the match arm
(around is_valid_instance_name(instance) in the same branch that builds
BroadcastTarget with adapter format!("{}:{}", platform, instance)) to explicitly
exclude the "dm" instance for those platforms (e.g., add a check that !(platform
== "discord" || platform == "slack") || instance != "dm", or equivalent) so DM
forms fall through to the DM-specific parsing logic instead of being treated as
named instances.
---
Duplicate comments:
In `@src/api/messaging.rs`:
- Around line 1600-1608: The code only writes dm_allowed_users when dm_users is
Some and non-empty, so clearing the input (which becomes None/undefined) leaves
the old key intact; update the logic around platform_table["dm_allowed_users"]
in the function that builds the TOML so that: if dm_users is Some and not empty,
set platform_table["dm_allowed_users"] = toml_edit::value(dm_array); otherwise
(dm_users is None or Some but empty) remove the key from platform_table (e.g.,
platform_table.remove("dm_allowed_users")); apply the same change to the similar
block around the other occurrence (lines referenced 1732-1740) so blanking the
field actually clears the saved allow-list; reference symbols:
platform_table["dm_allowed_users"], dm_users, and signal_dm_allowed_users.
In `@src/tools/send_message_to_another_channel.rs`:
- Around line 196-205: The code overwrites explicit "signal:<...>" targets with
the conversation's named adapter by unconditionally replacing target.adapter
when self.current_adapter starts_with("signal:"), losing the ability to target
the default Signal account; update the logic in the block that examines
target.adapter and self.current_adapter so it only substitutes target.adapter
with current_adapter when the original target was exactly "signal" (or
empty/null), and do not change target.adapter if the original value already
started with "signal:" (i.e., preserve explicit "signal:<name|+...|uuid...>"
entries); refer to target.adapter and self.current_adapter to implement this
conditional check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3c88e00-1a9f-43ec-943b-36571ab53773
📒 Files selected for processing (17)
interface/src/api/client.tsinterface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxinterface/src/lib/format.tsinterface/src/lib/platformIcons.tsxinterface/src/routes/Settings.tsxprompts/en/fragments/conversation_context.md.j2src/agent/channel.rssrc/api/channels.rssrc/api/messaging.rssrc/api/state.rssrc/main.rssrc/messaging/signal.rssrc/messaging/target.rssrc/prompts/engine.rssrc/tools/send_message_to_another_channel.rstests/context_dump.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/api/state.rs
- src/main.rs
- interface/src/lib/platformIcons.tsx
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/api/messaging.rs (1)
1374-1405:⚠️ Potential issue | 🟠 MajorGate named Signal startup on root
signal.enabled.Line 1377 starts a named Signal adapter even when the root Signal platform can still be disabled, which can desynchronize runtime state from reported status/config.
🔧 Suggested fix
Some(adapter_name) => { // Toggle specific named instance only let adapter_key = adapter_name.trim(); - if let Some(instance) = + if signal_config.enabled + && let Some(instance) = signal_config.instances.iter().find(|instance| { instance.name == adapter_key && instance.enabled }) { let runtime_key = crate::config::binding_runtime_adapter_key( "signal", Some(instance.name.as_str()), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1374 - 1405, The named Signal adapter startup block should be gated by the root platform enabled flag to avoid starting instances when the root Signal platform is disabled; update the Some(adapter_name) branch to first check signal_config.enabled (or the equivalent root flag) and only proceed to locate the instance, build runtime_key/permissions/tmp_dir and call manager.register_and_start(adapter) if the root is enabled, otherwise skip (and optionally log) so runtime state stays consistent; relevant symbols: signal_config, adapter_name, instance, crate::config::binding_runtime_adapter_key, crate::config::SignalPermissions::from_instance_config, SignalAdapter::new, manager.register_and_start.
🧹 Nitpick comments (1)
src/api/messaging.rs (1)
1603-1628: Extract Signal credential persistence into one helper.These two blocks duplicate the same parse/store logic. A shared helper would reduce drift risk when validation/storage rules change.
Also applies to: 1738-1763
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1603 - 1628, Create a single helper (e.g., handle_signal_credentials or persist_signal_credentials) that encapsulates the parse/store logic currently duplicated where parse_signal_credentials is invoked and platform_table is mutated: call parse_signal_credentials(credentials), normalize http_url.trim_end_matches('/'), set platform_table["http_url"] and platform_table["account"], and handle dm_allowed_users by writing a toml_edit::Array when Some/non-empty or removing the key when empty/None; replace both duplicated blocks (the one using parse_signal_credentials at lines shown and the other at 1738-1763) with a call to this helper so all parsing/validation and persistence lives in one place and returns the same Ok(Json(response)) or continues on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/messaging.rs`:
- Around line 274-280: The validator for signal_dm_allowed_users currently
accepts prefix-only entries like "uuid:" and "group:"; update the filter in the
entries validation (the iterator/filter block that uses starts_with("uuid:") and
starts_with("group:") and is_valid_e164) so that for "uuid:" and "group:" you
also verify there is a non-empty payload after the colon (e.g., ensure the
substring after the first ':' is Some and not empty) before treating it as
valid; keep the is_valid_e164(entry) check for E.164 numbers and reject any
entry that fails these checks.
---
Duplicate comments:
In `@src/api/messaging.rs`:
- Around line 1374-1405: The named Signal adapter startup block should be gated
by the root platform enabled flag to avoid starting instances when the root
Signal platform is disabled; update the Some(adapter_name) branch to first check
signal_config.enabled (or the equivalent root flag) and only proceed to locate
the instance, build runtime_key/permissions/tmp_dir and call
manager.register_and_start(adapter) if the root is enabled, otherwise skip (and
optionally log) so runtime state stays consistent; relevant symbols:
signal_config, adapter_name, instance,
crate::config::binding_runtime_adapter_key,
crate::config::SignalPermissions::from_instance_config, SignalAdapter::new,
manager.register_and_start.
---
Nitpick comments:
In `@src/api/messaging.rs`:
- Around line 1603-1628: Create a single helper (e.g., handle_signal_credentials
or persist_signal_credentials) that encapsulates the parse/store logic currently
duplicated where parse_signal_credentials is invoked and platform_table is
mutated: call parse_signal_credentials(credentials), normalize
http_url.trim_end_matches('/'), set platform_table["http_url"] and
platform_table["account"], and handle dm_allowed_users by writing a
toml_edit::Array when Some/non-empty or removing the key when empty/None;
replace both duplicated blocks (the one using parse_signal_credentials at lines
shown and the other at 1738-1763) with a call to this helper so all
parsing/validation and persistence lives in one place and returns the same
Ok(Json(response)) or continues on success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1d6f25c-da80-4885-a6e9-ee73f73d1ffc
📒 Files selected for processing (2)
src/api/messaging.rssrc/messaging/target.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/messaging/target.rs
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
from
d2fa980 to
94548fd
Compare
- Add Signal to Platform type and PLATFORM_LABELS - Add Signal credential form with http_url and account fields - Add E.164 format validation in frontend - Add Signal to available platforms list - Add Signal icon mapping (using generic comment icon) - Include placeholder documentation link fix(messaging): resolve Signal adapter validation and consistency issues Fixes several issues with Signal messaging adapter implementation: **E.164 Phone Number Validation (issue spacedriveapp#5)** - Move is_valid_e164 to messaging::target module for reuse - Add proper E.164 validation: first digit must be 1-9 (not 0) - Enforce maximum 15 digits per E.164 standard - Update normalize_signal_target to use shared validation - Add test cases for +01234567 (invalid) and >15 digit numbers **Validation Response Format (issue spacedriveapp#2)** - Replace StatusCode::BAD_REQUEST with MessagingInstanceActionResponse - Return user-friendly validation failures for missing signal_http_url - Return user-friendly validation failures for missing signal_account - Applied to both default and named instance creation paths **dm_allowed_users Cleanup (issue spacedriveapp#3)** - Remove old auto-added account from dm_allowed_users when account changes - Prevent allowlist from growing indefinitely on account updates - Only the new account remains in the authorized list **Signal Platform Deletion Support (issue spacedriveapp#4)** - Add 'signal' to delete_messaging_instance supported platforms - Add Signal-specific credential cleanup in delete handler - Enables proper deletion of Signal instances via API **Adapter Name Matching (issue spacedriveapp#6)** - Fix implicit Signal shorthand detection in send_message tool - Change from starts_with('signal') to exact match or signal: prefix - Prevents adapters like 'signal-alerts' from triggering Signal shorthand Note: DOC_LINKS entry for signal docs (issue spacedriveapp#1) retained as-is per request fix(api): add Signal to messaging status endpoint Add Signal instance detection to messaging_status function: - Read [messaging.signal] section for default instance - Read [[messaging.signal.instances]] for named instances - Push instance status to instances array when configured - Requires both http_url and account fields to be considered configured This fixes the issue where Signal instances were not appearing in the web UI. fix(signal): align validation rules and add toggle_platform support Frontend validation (ChannelSettingCard.tsx): - Update E.164 regex to /^\+[1-9]\d{5,13}$/ for true E.164 compliance - Enforce 7-15 digits total (6-14 after '+'), first digit 1-9 - Update error message and help text to reflect correct range Backend validation (target.rs): - Change is_valid_e164 to require 6-14 digits after '+' (7-15 total) - normalize_signal_target: Reuse is_valid_e164() for bare digit validation - parse_signal_target_parts: Add empty segment checks to all match arms - Reject empty uuid, group_id, phone, instance, or single segments - Prevents invalid targets like 'uuid:' or 'signal:work:' API state (state.rs): - Add signal_permissions field to ApiState - Add set_signal_permissions() method for hot-reload support toggle_platform (messaging.rs): - Add Signal case to toggle_platform for runtime adapter start/stop - Support both default and named Signal instances - Update existing ArcSwap pointee instead of creating new one when permissions exist send_message_to_another_channel: - parse_implicit_signal_shorthand: Use is_valid_e164() for strict validation - Add specific error messages for each validation failure case - Add early error for ambiguous Signal adapter selection when multiple named instances exist without a default, preventing broadcast() failure - Fix adapter resolution to distinguish default 'signal' from named adapters Tests (messaging.rs): - Update E.164 tests for 6-14 digit range (7-15 total)
…h available channels format Problem: The LLM was confusing the current Signal group with other groups when creating cron jobs. The conversation context showed: Channel: Group Chat abc123 (signal) While available channels showed: Group Chat abc123 (signal, id: signal:adapter:group:abc123) This format mismatch caused the LLM to pick the wrong group ID from the available channels list when asked about "this chat". Solution: Update the conversation context template to include the full channel ID in the same format as available channels: Channel: Group Chat abc123 (signal, id: signal:adapter:group:abc123) Changes: - prompts/en/fragments/conversation_context.md.j2: Add conversation_id to template - src/prompts/engine.rs: Add conversation_id parameter to render_conversation_context() - src/agent/channel.rs: Pass conversation_id when building context - src/api/channels.rs: Pass channel id when building context Now the LLM can clearly correlate the current chat with the correct entry in the available channels list, preventing group ID confusion.
ibhagwan
force-pushed
the
feat/signal-web-ui-config
branch
from
94548fd to
27c75fc
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/messaging/target.rs (1)
533-536:⚠️ Potential issue | 🟠 MajorPreserve
dm:targets before the legacy fallback.Line 533 currently catches
discord:dm:<user>/slack:dm:<user>and collapses them to<user>, which drops DM semantics. Add DM-specific arms ahead of the legacy branch.🔧 Suggested fix
match parts { + [platform, "dm", user_id] + if (*platform == "discord" || *platform == "slack") && !user_id.is_empty() => + { + Some(BroadcastTarget { + adapter: (*platform).to_string(), + target: format!("dm:{user_id}"), + }) + } // Named adapter: platform:instance:target [platform, instance, target, ..] if parts.len() >= 3 && !instance.is_empty() && !target.is_empty() && is_valid_instance_name(instance) && !((*platform == "discord" || *platform == "slack") && *instance == "dm") => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 533 - 536, The match arm that collapses discord:dm:<user> and slack:dm:<user> into just <user> is losing DM semantics; add explicit DM-specific arms before the legacy fallback so DM targets are preserved. Specifically, add two arms matching [platform, "dm", user] (or similar slices) and return Some(BroadcastTarget { adapter: (*platform).to_string(), target: format!("dm:{}", *user) }) (or otherwise preserve the "dm:" prefix) for the BroadcastTarget; keep the existing legacy [platform, .., target] arm afterwards.src/api/messaging.rs (2)
274-280:⚠️ Potential issue | 🟡 MinorReject prefix-only Signal allow-list entries.
Line 279 still accepts
uuid:andgroup:without payload. These should be invalid.🔧 Suggested fix
let invalid_entries: Vec<&String> = entries .iter() .filter(|entry| { // Valid formats: uuid:xxx, group:xxx, +E.164 - !(entry.starts_with("uuid:") || entry.starts_with("group:") || is_valid_e164(entry)) + let is_uuid = entry + .strip_prefix("uuid:") + .is_some_and(|value| !value.is_empty()); + let is_group = entry + .strip_prefix("group:") + .is_some_and(|value| !value.is_empty()); + !(is_uuid || is_group || is_valid_e164(entry)) }) .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 274 - 280, The current filter in the invalid_entries computation accepts "uuid:" and "group:" with no payload; update the predicate used in the entries.iter().filter(...) (around the invalid_entries variable in messaging.rs) to reject prefix-only values by verifying that when entry.starts_with("uuid:") or entry.starts_with("group:"), the substring after the colon is non-empty (e.g., ensure entry.len() > "uuid:".len() / "group:".len() or check the part after ':' is not empty); keep the existing is_valid_e164(entry) check for E.164 entries.
1369-1400:⚠️ Potential issue | 🟠 MajorGate named-instance startup on root Signal enable state.
In Line 1372 onward, named Signal instances can start even when
messaging.signal.enabledis false, which desynchronizes runtime state from reported status.🛠️ Suggested fix
Some(adapter_name) => { // Toggle specific named instance only + if !signal_config.enabled { + return Ok(Json(serde_json::json!({ + "success": false, + "message": "signal root adapter is disabled; enable it before starting named instances" + }))); + } let adapter_key = adapter_name.trim(); if let Some(instance) = signal_config.instances.iter().find(|instance| { instance.name == adapter_key && instance.enabled })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1369 - 1400, The named-instance startup in the Some(adapter_name) branch should be gated by the root Signal enabled flag; modify the code in the Some(adapter_name) arm to first check signal_config.enabled (or the equivalent root flag) and only proceed to create runtime_key, permissions, tmp_dir, construct SignalAdapter and call manager.register_and_start(adapter). If the root flag is false, skip starting the named instance (and optionally emit a trace/debug log indicating the instance was not started due to messaging.signal.enabled being false) to keep runtime state in sync with reported status.
🧹 Nitpick comments (2)
src/main.rs (1)
3110-3112: Use a non-abbreviated binding name for permissions.Please rename
permson Line 3110 to a full word (for example,permissions) to match Rust naming conventions used in this repo.Suggested diff
- if let Some(perms) = &*signal_permissions { - api_state.set_signal_permissions(perms.clone()).await; + if let Some(permissions) = &*signal_permissions { + api_state.set_signal_permissions(permissions.clone()).await; }As per coding guidelines, "Don't abbreviate variable names. Use queue not q, message not msg, channel not ch. Common abbreviations like config are fine".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 3110 - 3112, The binding name `perms` in the conditional handling of `signal_permissions` should be renamed to a non-abbreviated identifier to match project naming conventions; change the `if let Some(perms) = &*signal_permissions { ... }` pattern to use e.g. `permissions` and update the use inside the block to call `api_state.set_signal_permissions(permissions.clone()).await;` so the pattern, variable and call reference the full-word identifier (relating to `signal_permissions` and `api_state.set_signal_permissions`).src/messaging/target.rs (1)
440-479: Useis_valid_instance_name()in Signal named-target branches for consistency.Line 440 onward accepts any non-empty
instance, while creation paths enforce stricter instance-name rules. Aligning parser guards withis_valid_instance_name()keeps behavior predictable.♻️ Suggested refactor
- [instance, "uuid", uuid] if !instance.is_empty() && !uuid.is_empty() => { + [instance, "uuid", uuid] + if !instance.is_empty() && is_valid_instance_name(instance) && !uuid.is_empty() => + { Some(BroadcastTarget { adapter: format!("signal:{instance}"), target: format!("uuid:{uuid}"), }) } - [instance, "group", group_id] if !instance.is_empty() && !group_id.is_empty() => { + [instance, "group", group_id] + if !instance.is_empty() + && is_valid_instance_name(instance) + && !group_id.is_empty() => + { Some(BroadcastTarget { adapter: format!("signal:{instance}"), target: format!("group:{group_id}"), }) } - [instance, "e164", phone] if !instance.is_empty() && !phone.is_empty() => { + [instance, "e164", phone] + if !instance.is_empty() && is_valid_instance_name(instance) && !phone.is_empty() => + { normalize_signal_target(&format!("e164:{phone}")).map(|target| BroadcastTarget { adapter: format!("signal:{instance}"), target, }) } [instance, phone] - if !instance.is_empty() && phone.starts_with('+') && !phone.is_empty() => + if !instance.is_empty() + && is_valid_instance_name(instance) + && phone.starts_with('+') + && !phone.is_empty() => { normalize_signal_target(phone).map(|target| BroadcastTarget { adapter: format!("signal:{instance}"), target, }) } [instance, single] if !instance.is_empty() + && is_valid_instance_name(instance) && !single.is_empty() && !single.chars().all(|c| c.is_ascii_digit()) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 440 - 479, The Signal named-target match arms accept any non-empty instance but elsewhere instance names are validated with is_valid_instance_name(); update the guards in the branches that construct BroadcastTarget (the arms matching [instance, "uuid", uuid], [instance, "group", group_id], [instance, "e164", phone], [instance, phone] where phone starts_with('+'), and [instance, single] non-numeric) to call is_valid_instance_name(instance) instead of !instance.is_empty(); keep the existing checks (uuid/group/phone/non-numeric) and still use normalize_signal_target(...) and adapter: format!("signal:{instance}")/target construction as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/messaging.rs`:
- Around line 221-225: The log currently includes the raw url_str which can leak
embedded credentials; remove or mask credentials before logging and returning
the error. Replace uses of tracing::warn!(%e, url = %url_str, ...) and the
message field in MessagingInstanceActionResponse to use a redacted value (e.g.,
call a small helper mask_url_credentials(url_str) that strips userinfo between
"://" and "@" or returns a safe placeholder when parsing fails), or simply omit
the url entirely from the trace and response. Update the error branch that
references url_str, the tracing::warn! invocation, and the
MessagingInstanceActionResponse construction to use the redacted value or no
URL.
---
Duplicate comments:
In `@src/api/messaging.rs`:
- Around line 274-280: The current filter in the invalid_entries computation
accepts "uuid:" and "group:" with no payload; update the predicate used in the
entries.iter().filter(...) (around the invalid_entries variable in messaging.rs)
to reject prefix-only values by verifying that when entry.starts_with("uuid:")
or entry.starts_with("group:"), the substring after the colon is non-empty
(e.g., ensure entry.len() > "uuid:".len() / "group:".len() or check the part
after ':' is not empty); keep the existing is_valid_e164(entry) check for E.164
entries.
- Around line 1369-1400: The named-instance startup in the Some(adapter_name)
branch should be gated by the root Signal enabled flag; modify the code in the
Some(adapter_name) arm to first check signal_config.enabled (or the equivalent
root flag) and only proceed to create runtime_key, permissions, tmp_dir,
construct SignalAdapter and call manager.register_and_start(adapter). If the
root flag is false, skip starting the named instance (and optionally emit a
trace/debug log indicating the instance was not started due to
messaging.signal.enabled being false) to keep runtime state in sync with
reported status.
In `@src/messaging/target.rs`:
- Around line 533-536: The match arm that collapses discord:dm:<user> and
slack:dm:<user> into just <user> is losing DM semantics; add explicit
DM-specific arms before the legacy fallback so DM targets are preserved.
Specifically, add two arms matching [platform, "dm", user] (or similar slices)
and return Some(BroadcastTarget { adapter: (*platform).to_string(), target:
format!("dm:{}", *user) }) (or otherwise preserve the "dm:" prefix) for the
BroadcastTarget; keep the existing legacy [platform, .., target] arm afterwards.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 3110-3112: The binding name `perms` in the conditional handling of
`signal_permissions` should be renamed to a non-abbreviated identifier to match
project naming conventions; change the `if let Some(perms) =
&*signal_permissions { ... }` pattern to use e.g. `permissions` and update the
use inside the block to call
`api_state.set_signal_permissions(permissions.clone()).await;` so the pattern,
variable and call reference the full-word identifier (relating to
`signal_permissions` and `api_state.set_signal_permissions`).
In `@src/messaging/target.rs`:
- Around line 440-479: The Signal named-target match arms accept any non-empty
instance but elsewhere instance names are validated with
is_valid_instance_name(); update the guards in the branches that construct
BroadcastTarget (the arms matching [instance, "uuid", uuid], [instance, "group",
group_id], [instance, "e164", phone], [instance, phone] where phone
starts_with('+'), and [instance, single] non-numeric) to call
is_valid_instance_name(instance) instead of !instance.is_empty(); keep the
existing checks (uuid/group/phone/non-numeric) and still use
normalize_signal_target(...) and adapter: format!("signal:{instance}")/target
construction as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88b512ae-8d64-4583-8071-79d0b9e5151e
📒 Files selected for processing (17)
interface/src/api/client.tsinterface/src/components/ChannelEditModal.tsxinterface/src/components/ChannelSettingCard.tsxinterface/src/lib/format.tsinterface/src/lib/platformIcons.tsxinterface/src/routes/Settings.tsxprompts/en/fragments/conversation_context.md.j2src/agent/channel.rssrc/api/channels.rssrc/api/messaging.rssrc/api/state.rssrc/main.rssrc/messaging/signal.rssrc/messaging/target.rssrc/prompts/engine.rssrc/tools/send_message_to_another_channel.rstests/context_dump.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- interface/src/lib/format.ts
- tests/context_dump.rs
- interface/src/components/ChannelEditModal.tsx
- interface/src/routes/Settings.tsx
- src/api/channels.rs
- src/api/state.rs
- interface/src/lib/platformIcons.tsx
- src/messaging/signal.rs
- interface/src/components/ChannelSettingCard.tsx
- prompts/en/fragments/conversation_context.md.j2
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As per title, this adds signal configuration to web interface settings/channels.
@jamiepine, couple of questions:
Note
Adds Signal messaging platform support to the web configuration UI. Includes frontend input fields for signal-cli HTTP URL and account phone number (E.164 format validation), backend credential handling with auto-population of account to dm_allowed_users, and comprehensive E.164 phone number validation tests.
Written by Tembo for commit 0d763a7. This will update automatically on new commits.
Note
Updated Summary: Comprehensive Signal adapter integration including API endpoints for configuring Signal credentials with E.164 phone number validation, frontend UI components (ChannelEditModal and ChannelSettingCard) for Signal-specific inputs, and enhancements to cron job and message routing systems. Backend validates and normalizes E.164 phone numbers (+ prefix with 6-15 digits, first digit 1-9), auto-populates dm_allowed_users from account number, and includes fallback number parsing for multiple formats.
Written by Tembo for commit 0d4c4f6. This will update automatically on new commits.