Repository navigation
Make web message sending idempotent and improve delivery reliability - #15
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThe pull request replaces shell forwarding with validated HTTP webhooks, adds trusted-proxy and monitoring configuration, improves authentication and message-operation recovery, localizes timestamps, hardens SQLite and systemd installation behavior, and updates related documentation, tests, and translations. ChangesWebhook forwarding and configuration
Authentication and monitoring
Messaging and runtime hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant DeliveryWorker
participant ProductionDispatcher
participant Webhook
participant WebhookEndpoint
DeliveryWorker->>ProductionDispatcher: dispatch webhook profile
ProductionDispatcher->>Webhook: send WebhookMessage
Webhook->>WebhookEndpoint: GET or POST rendered request
WebhookEndpoint-->>Webhook: HTTP status or transport error
Webhook-->>ProductionDispatcher: success, transient, or permanent outcome
ProductionDispatcher-->>DeliveryWorker: delivery result and retry timing
sequenceDiagram
participant Frontend
participant API
participant AuthRoot
Frontend->>API: request protected resource
API-->>Frontend: 401 response
Frontend->>AuthRoot: dispatch unauthorized event
AuthRoot->>AuthRoot: clear authenticated state
AuthRoot-->>Frontend: navigate to login
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/messages/message-console.tsx (1)
428-441: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMark-read errors mislabeled when the follow-up reload fails.
Both
handleMarkConversationRead(556-571) and the auto mark-read effect (428-441) wrap the mark POST andreloadActiveViews()in one try/catch, always settingoperationError("markRead"). If marking succeeds but the reload fails, the banner still says "could not mark as read," which is misleading.handleMarkSelected/handleDeleteSelectedalready split these into separate try/catch blocks with distinct error codes (update/deletevsrefresh) — worth applying the same split here for consistency.♻️ Proposed fix
async function handleMarkConversationRead() { if (!selectedPhone) return; setOperationError(null); try { await apiFetch( `/api/conversations/${encodeURIComponent(selectedPhone)}/read`, { method: "POST", }, ); - await reloadActiveViews(); } catch (err) { setOperationError("markRead"); + console.error(err); + return; + } + try { + await reloadActiveViews(); + } catch (err) { + setOperationError("refresh"); console.error(err); } }Also applies to: 556-571
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/messages/message-console.tsx` around lines 428 - 441, Separate the mark-read POST and follow-up reload error handling in both the auto mark-read effect and handleMarkConversationRead. Keep mark-read failures mapped to operationError("markRead"), but catch reloadActiveViews failures separately and map them to operationError("refresh"), preserving cleanup of markingReadPhonesRef.frontend/src/locales/ja.ts (1)
512-513: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale shell wording left in the Japanese timeouts description.
config.fields.timeouts.sectionDescriptionstill mentions シェルプロファイルの実行, but shell forwarding was removed in this PR. The equivalent string was updated ines.ts(Line 524),fr.ts(Line 522),ko.ts(Line 507) andzh-CN.ts(Line 482);ja.tswas missed.🌐 Proposed fix
sectionDescription: - "接続の確立、プロバイダーリクエスト、およびシェルプロファイルの実行を制限します。すべての値は秒単位です。", + "接続の確立と、プロバイダーまたは Webhook へのリクエストを制限します。すべての値は秒単位です。",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/locales/ja.ts` around lines 512 - 513, Update config.fields.timeouts.sectionDescription in the Japanese locale to remove the outdated シェルプロファイルの実行 wording, keeping only the description of connection establishment and provider requests while preserving the existing seconds-unit wording.
🧹 Nitpick comments (1)
frontend/src/components/config/channel-editor.tsx (1)
605-669: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink the header-name error to the input via
aria-describedby.
aria-invalidis set but the error paragraph isn't associated with the input viaid/aria-describedby, unlike the duplicate-profile-name pattern used elsewhere in this file (lines 439–457). Screen reader users get "invalid" without the reason.♻️ Proposed fix
function WebhookHeaderRow({ name, value, allNames, onChange, }: { name: string; value: string; allNames: string[]; onChange: (oldName: string, name: string, value: string) => void; }) { const { t } = useTranslation(); const [draftName, setDraftName] = useState(name); const missing = draftName.length === 0; const duplicate = draftName.toLowerCase() !== name.toLowerCase() && allNames.some( (existingName) => existingName.toLowerCase() === draftName.toLowerCase(), ); const invalid = missing || duplicate; + const errorId = `webhook-header-${name || "new"}-error`; return ( <div> <div className="grid grid-cols-[1fr_1fr_auto] gap-2"> <Input aria-label={t("config.channel.webhookHeaderName")} aria-invalid={invalid} + aria-describedby={invalid ? errorId : undefined} value={draftName} onChange={(event) => setDraftName(event.target.value)} onBlur={() => { if (!invalid && draftName !== name) { onChange(name, draftName, value); } }} className="h-8 font-mono text-xs" /> ... </div> {invalid ? ( - <p className="mt-1 text-xs text-destructive"> + <p id={errorId} className="mt-1 text-xs text-destructive"> {t( duplicate ? "config.channel.webhookHeaderDuplicate" : "config.channel.webhookHeaderRequired", )} </p> ) : null} </div> ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/config/channel-editor.tsx` around lines 605 - 669, Associate the header-name validation message in WebhookHeaderRow with its name Input by assigning a stable unique id and setting aria-describedby to that id when invalid. Add the matching id to the conditional error paragraph, following the existing duplicate-profile-name pattern, while preserving the current aria-invalid and validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/messages/message-filters.tsx`:
- Line 4: Update the CSV and JSON export handlers in MessageFilters to attach
rejection handling to each void downloadFile call, matching the existing
message-console.tsx/FilterDialog pattern and surfacing the export error through
the component’s established UI feedback mechanism.
In `@frontend/src/locales/es.ts`:
- Around line 324-334: Update the Spanish messages.error strings in the error
object and thread.sendFailed to use the formal usted register, replacing
informal “Inténtalo de nuevo” wording with “Inténtelo de nuevo” while preserving
the existing translations and message structure.
In `@frontend/src/main.tsx`:
- Around line 25-28: Move the rootElement.innerHTML guard in start() before
loadMonitoringPreference() and initMonitoring(), returning immediately on repeat
invocation so no monitoring fetch or re-initialization occurs when mounting is
skipped.
In `@src/config.rs`:
- Around line 705-714: Update config validation to match other channel types by
removing the unconditional webhook profile validation loop in validate(). Keep
webhook validation through profile_for_ref when a webhook is referenced by
forward.enabled, and update
validates_webhook_templates_and_redacts_url_and_headers to reflect that disabled
or unreferenced incomplete profiles are allowed.
In `@src/forward/webhook.rs`:
- Around line 61-70: Update validate_profile around render_template and URL
parsing to reject any {SENDER}, {MESSAGE}, or {DATETIME} token appearing in the
URL authority (host, port, or userinfo), preventing runtime-controlled
destinations. Require URL-encoded token forms when values are intended elsewhere
in the URL, and preserve the existing http/https scheme validation. Ensure
send() cannot receive a profile whose rendered authority can vary with inbound
SMS content.
In `@src/storage/mod.rs`:
- Around line 312-339: Add a Unix-only restrictive umask guard around the
connection opening, WAL setup, and Self::migrate() sequence in the storage
initialization flow, so SQLite-created sidecars begin with 0600 permissions.
Scope the guard tightly and ensure the original process umask is restored on
every exit path, including errors; keep existing restrict_sqlite_file and
restrict_sqlite_sidecars calls as defense-in-depth and leave non-Unix behavior
unchanged.
---
Outside diff comments:
In `@frontend/src/components/messages/message-console.tsx`:
- Around line 428-441: Separate the mark-read POST and follow-up reload error
handling in both the auto mark-read effect and handleMarkConversationRead. Keep
mark-read failures mapped to operationError("markRead"), but catch
reloadActiveViews failures separately and map them to operationError("refresh"),
preserving cleanup of markingReadPhonesRef.
In `@frontend/src/locales/ja.ts`:
- Around line 512-513: Update config.fields.timeouts.sectionDescription in the
Japanese locale to remove the outdated シェルプロファイルの実行 wording, keeping only the
description of connection establishment and provider requests while preserving
the existing seconds-unit wording.
---
Nitpick comments:
In `@frontend/src/components/config/channel-editor.tsx`:
- Around line 605-669: Associate the header-name validation message in
WebhookHeaderRow with its name Input by assigning a stable unique id and setting
aria-describedby to that id when invalid. Add the matching id to the conditional
error paragraph, following the existing duplicate-profile-name pattern, while
preserving the current aria-invalid and validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d661975b-2dc6-4987-a985-714d3aeebe16
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (52)
README.mddocs/operations/long-running-validation.mdfrontend/package.jsonfrontend/pnpm-workspace.yamlfrontend/src/channel-editor.test.tsxfrontend/src/components/config/channel-editor.tsxfrontend/src/components/config/config-editor.tsxfrontend/src/components/config/config-section-editors.tsxfrontend/src/components/config/config-sections.tsfrontend/src/components/messages/message-console.tsxfrontend/src/components/messages/message-filters.tsxfrontend/src/config-editor.test.tsxfrontend/src/language-switcher.test.tsxfrontend/src/lib/api.test.tsfrontend/src/lib/api.tsfrontend/src/lib/config-api.tsfrontend/src/lib/config-model.tsfrontend/src/lib/events.test.tsfrontend/src/lib/events.tsfrontend/src/lib/monitoring.test.tsfrontend/src/lib/monitoring.tsfrontend/src/locales/en.tsfrontend/src/locales/es.tsfrontend/src/locales/fr.tsfrontend/src/locales/ja.tsfrontend/src/locales/ko.tsfrontend/src/locales/zh-CN.tsfrontend/src/main.tsxfrontend/src/message-console.test.tsxfrontend/src/root-auth.test.tsxfrontend/src/routes/__root.tsxinstall.shsrc/api/auth.rssrc/api/config.rssrc/api/mod.rssrc/api/modem.rssrc/config.rssrc/delivery.rssrc/delivery/dispatcher.rssrc/delivery/worker.rssrc/forward/mod.rssrc/forward/shell.rssrc/forward/webhook.rssrc/main.rssrc/modem.rssrc/monitoring.rssrc/persistence/mod.rssrc/runner.rssrc/runtime.rssrc/storage/mod.rssrc/wizard.rstests/install.sh
💤 Files with no reviewable changes (2)
- frontend/src/components/config/config-section-editors.tsx
- src/forward/shell.rs
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.
Summary
Testing
Summary by CodeRabbit
trusted_proxiesfor proxy-aware client handling./api/monitoringand privacy-conscious setup prompts.