feat: unarchive message thread on new inbound message (per-phone setting)#954
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 9 minor |
🟢 Metrics 19 complexity · 12 duplication
Metric Results Complexity 19 Duplication 12
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
c600660 to
9495f26
Compare
There was a problem hiding this comment.
Pull request overview
Adds a per-phone unarchive_thread setting that automatically unarchives an archived message thread when a new inbound/received message arrives for that phone. This flows from the Phone entity → received-message event payload → thread listener → thread update service, and is exposed in the web settings UI with tests covering the behavior.
Changes:
- API: Introduce
Phone.UnarchiveThread, propagate it on themessage.phone.receivedevent, and unarchive threads duringMessageThreadService.UpdateThreadfor inbound received messages. - Web: Send/persist
unarchive_threadfrom the phones store and expose a settings toggle; regenerate TS API models from Swagger. - Tests/Docs: Add E2E integration tests and update test coverage docs; add design/plan documents.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| web/shared/types/api.ts | Regenerated API types to include unarchive_thread on phone entities and requests. |
| web/app/stores/phones.ts | Sends unarchive_thread in the phone upsert/update request payload. |
| web/app/pages/settings/index.vue | Adds a per-phone switch to toggle auto-unarchive-on-receive. |
| tests/unarchive_thread_integration_test.go | New black-box integration tests validating enabled/disabled unarchive behavior. |
| tests/README.md | Documents the new E2E test coverage item. |
| docs/superpowers/specs/2026-07-18-webhook-email-payload-formatting-design.md | Adds a design doc (webhook email payload formatting). |
| docs/superpowers/specs/2026-07-18-unarchive-thread-on-receive-design.md | Design doc for the unarchive-on-receive feature. |
| docs/superpowers/plans/2026-07-18-unarchive-thread-on-receive.md | Implementation plan for the feature. |
| api/pkg/services/phone_service.go | Applies UnarchiveThread during phone upsert when provided. |
| api/pkg/services/message_thread_service.go | Adds unarchive decision helper and unarchives threads on inbound received updates. |
| api/pkg/services/message_thread_service_test.go | Unit test coverage for the shouldUnarchive helper. |
| api/pkg/services/message_service.go | Loads phone setting during receive and forwards it into the received-event payload. |
| api/pkg/requests/phone_update_request.go | Accepts unarchive_thread and maps it into upsert params using the partial-update presence check. |
| api/pkg/listeners/message_thread_listener.go | Forwards payload.UnarchiveThread into thread update params for received messages. |
| api/pkg/events/message_phone_received_event.go | Extends received-message event payload with unarchive_thread. |
| api/pkg/entities/phone.go | Adds UnarchiveThread to the Phone entity with default false. |
| api/docs/swagger.yaml | Regenerated OpenAPI spec including the new field. |
| api/docs/swagger.json | Regenerated OpenAPI JSON including the new field. |
| api/docs/docs.go | Regenerated embedded Swagger template including the new field. |
Files not reviewed (1)
- api/docs/docs.go: Generated file
| "io" | ||
| "net/http" | ||
| "testing" | ||
| "time" |
There was a problem hiding this comment.
Fixed in e1c3d1f — fetchThreads now wraps owner with url.QueryEscape, so the E.164 + is no longer decoded to a space in the query string.
| func fetchThreads(ctx context.Context, t *testing.T, owner string, archived bool) []integrationThread { | ||
| t.Helper() | ||
|
|
||
| url := fmt.Sprintf("%s/v1/message-threads?owner=%s&is_archived=%t&limit=20", apiBaseURL, owner, archived) |
There was a problem hiding this comment.
Fixed in e1c3d1f — same fix: owner is now passed through url.QueryEscape when building the message-threads query.
| MissedCallAutoReply *string `json:"missed_call_auto_reply" example:"e.g. This phone cannot receive calls. Please send an SMS instead."` | ||
|
|
||
| // UnarchiveThread moves an archived thread back to the inbox when a new message is received on this phone. | ||
| UnarchiveThread bool `json:"unarchive_thread" example:"false"` | ||
|
|
There was a problem hiding this comment.
Not changing this — the stated goal (keep the field optional in the OpenAPI schema) is not achievable in this repo. Swagger is generated with --requiredByDefault, which marks every field required regardless of pointer type. missed_call_auto_reply is a *string and is already listed under required for requests.PhoneUpsert, so switching unarchive_thread to *bool would not remove it from required. The non-pointer bool is consistent with the other partial-update fields (max_send_attempts, messages_per_minute), and the server never enforces required at runtime — absent keys are skipped via the fields presence check, so partial updates keep working.
| # Webhook Email Payload Formatting | ||
|
|
||
| - Date: 2026-07-18 | ||
| - Status: Approved (design) | ||
| - Scope: `api/` email rendering only. | ||
|
|
There was a problem hiding this comment.
Fixed — the docs: design webhook payload formatting commit was an unrelated stray carried over from another feature branch. Dropped it from this branch (the doc still lives on feature/webhook-email-payload-formatting), so this PR now contains only the unarchive-thread changes.
Greptile SummaryThis PR introduces a per-phone
Confidence Score: 4/5Safe to merge — the core unarchive logic is correct and well-tested, with no data-loss or corruption risk. The data flow is sound: UpdateArchive and Update both mutate the thread pointer in-place, so both the archive-clear and the last-message fields are saved in the single repository.Update call. The two early-return guards in UpdateThread are each gated on HasLastMessage(params.MessageID), which is false for any genuinely new message, so the unarchive path is always reached for real inbound messages. The only issues found are a new unconditional DB query per inbound message (even when the feature is disabled) and a test helper that quietly overwrites the phone's SIM as a side effect. api/pkg/services/message_service.go (extra phone load on every receive path) and tests/unarchive_thread_integration_test.go (setUnarchiveThread helper's SIM side-effect). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Android as Android Phone
participant API as MessageService.ReceiveMessage
participant DB_Phone as Phone Repository
participant Queue as Event Dispatcher
participant Listener as MessageThreadListener
participant ThreadSvc as MessageThreadService.UpdateThread
participant DB_Thread as Thread Repository
Android->>API: POST /v1/messages/receive
API->>DB_Phone: Load(userID, ownerE164)
Note over API,DB_Phone: Read UnarchiveThread flag
DB_Phone-->>API: phone.UnarchiveThread
API->>Queue: "Dispatch(MessagePhoneReceived{UnarchiveThread})"
API-->>Android: 200 OK
Queue->>Listener: OnMessagePhoneReceived
Listener->>ThreadSvc: "UpdateThread(params{UnarchiveThread, Status=received})"
ThreadSvc->>DB_Thread: LoadByOwnerContact
DB_Thread-->>ThreadSvc: "thread{IsArchived}"
Note over ThreadSvc: shouldUnarchive check
ThreadSvc->>ThreadSvc: thread.UpdateArchive(false) [if needed]
ThreadSvc->>DB_Thread: Update(thread)
DB_Thread-->>ThreadSvc: saved
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Android as Android Phone
participant API as MessageService.ReceiveMessage
participant DB_Phone as Phone Repository
participant Queue as Event Dispatcher
participant Listener as MessageThreadListener
participant ThreadSvc as MessageThreadService.UpdateThread
participant DB_Thread as Thread Repository
Android->>API: POST /v1/messages/receive
API->>DB_Phone: Load(userID, ownerE164)
Note over API,DB_Phone: Read UnarchiveThread flag
DB_Phone-->>API: phone.UnarchiveThread
API->>Queue: "Dispatch(MessagePhoneReceived{UnarchiveThread})"
API-->>Android: 200 OK
Queue->>Listener: OnMessagePhoneReceived
Listener->>ThreadSvc: "UpdateThread(params{UnarchiveThread, Status=received})"
ThreadSvc->>DB_Thread: LoadByOwnerContact
DB_Thread-->>ThreadSvc: "thread{IsArchived}"
Note over ThreadSvc: shouldUnarchive check
ThreadSvc->>ThreadSvc: thread.UpdateArchive(false) [if needed]
ThreadSvc->>DB_Thread: Update(thread)
DB_Thread-->>ThreadSvc: saved
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| owner := phonenumbers.Format(¶ms.Owner, phonenumbers.E164) | ||
|
|
||
| unarchiveThread := false | ||
| phone, err := service.phoneService.Load(ctx, params.UserID, owner) | ||
| if err != nil { | ||
| ctxLogger.Warn(stacktrace.Propagate(err, fmt.Sprintf("cannot load phone [%s] for user [%s] to resolve UnarchiveThread; defaulting to false", owner, params.UserID))) | ||
| } else { | ||
| unarchiveThread = phone.UnarchiveThread | ||
| } |
There was a problem hiding this comment.
phoneService.Load is now called for every received message, regardless of whether the thread is archived or whether the user has enabled unarchive_thread. For users with the feature disabled (the default), this adds an extra round-trip to the database on every inbound message with no effect. The UnarchiveThread flag is false by default, so the vast majority of messages will read the phone only to find false.
A lighter alternative would be to carry the feature only when the thread is actually archived — but that would require restructuring the event-driven flow. A simpler mitigation is to read the flag lazily inside UpdateThread when thread.IsArchived is already known to be true, rather than snapshotting it upfront in ReceiveMessage.
There was a problem hiding this comment.
This extra load is inherent to the chosen design: the flag is snapshotted onto the received-message event so the unarchive decision can be made without giving MessageThreadService a phone/repository dependency. Reading it lazily inside UpdateThread would reintroduce exactly that coupling, which was intentionally avoided. It is a single indexed lookup by (user, owner) on an already-hot receive path. Flagging for the maintainer to decide if the trade-off should change.
There was a problem hiding this comment.
Addressed in f355239. The phone is no longer loaded on every inbound message. The UnarchiveThread flag was removed from the event payload; instead MessageThreadService.UpdateThread lazily loads the phone only when an inbound message lands on an already-archived thread (the rare path), so the common case does zero extra reads.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf78429e-573c-406c-9f7c-1d6e1bddbbb5
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf78429e-573c-406c-9f7c-1d6e1bddbbb5
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf78429e-573c-406c-9f7c-1d6e1bddbbb5
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf78429e-573c-406c-9f7c-1d6e1bddbbb5
- URL-escape the E.164 owner in the message-threads query (+ decodes to space) - Use native stacktrace.Propagate printf args (avoids go vet non-constant format warning) - Document why setUnarchiveThread must send the required sim field Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf78429e-573c-406c-9f7c-1d6e1bddbbb5
443feaf to
e1c3d1f
Compare
Consult the phone's UnarchiveThread setting inside MessageThreadService.UpdateThread only when an inbound message lands on an archived thread, instead of loading the phone on every received message. Removes the per-message phone read in ReceiveMessage and drops the UnarchiveThread field from the received-message event payload. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf78429e-573c-406c-9f7c-1d6e1bddbbb5
| if service.shouldCheckUnarchive(thread, params) { | ||
| phone, phoneErr := service.phoneRepository.Load(ctx, params.UserID, params.Owner) | ||
| if phoneErr != nil { | ||
| ctxLogger.Warn(stacktrace.Propagate(phoneErr, "cannot load phone [%s] for user [%s] to resolve UnarchiveThread; leaving thread [%s] archived", params.Owner, params.UserID, thread.ID)) | ||
| } else if phone.UnarchiveThread { |
There was a problem hiding this comment.
Updated the PR description to match the implemented flow. The event-based approach was intentionally replaced with a lazy phone load in UpdateThread (gated by shouldCheckUnarchive) so the common non-archived path does zero extra DB reads; the phone is only read when an inbound message hits an already-archived thread.
| assert.True(t, service.shouldCheckUnarchive(archived, received), "archived + inbound -> consult phone setting") | ||
| assert.False(t, service.shouldCheckUnarchive(archived, sent), "outbound status -> no check") | ||
| assert.False(t, service.shouldCheckUnarchive(notArchived, received), "already unarchived -> no check") | ||
| } |
There was a problem hiding this comment.
Added in a4fc0af. TestShouldCheckUnarchive now covers not-archived + non-received.
| # Webhook Email Payload Formatting Implementation Plan | ||
|
|
||
| > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | ||
|
|
||
| **Goal:** Render the webhook failure email's Event Payload as readable indented JSON with safe lightweight syntax colors in HTML and readable indentation in plain text. | ||
|
|
||
| **Architecture:** Add a focused formatter in `api/pkg/emails` that produces a plain string and a safely constructed `template.HTML` code block. Teach the custom Hermes HTML dictionary template to use `hermes.Entry.UnsafeValue` when explicitly supplied, then wire only the webhook email's Event Payload entry to that path. |
There was a problem hiding this comment.
Removed in a4fc0af. The webhook email payload formatting plan doc doesn't belong on this branch.
The phone-to-API-key association runs asynchronously via the phone_api_key event listener, so a freshly provisioned phone can return 401 on receive until the auth cache clears. Retry receiveInbound on that transient 401 instead of failing immediately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf78429e-573c-406c-9f7c-1d6e1bddbbb5
Remove an unrelated webhook email payload formatting plan doc that was accidentally included in this feature branch, and add the missing shouldCheckUnarchive case for a non-archived thread with a non-received status so every branch of the predicate is exercised. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf78429e-573c-406c-9f7c-1d6e1bddbbb5
Integrate current origin/main (thread archive-view preservation #952, unarchive-on-receive #954, webhook email payload formatting #955) with the read-receipts feature set, preserving both. Semantic resolutions: - UpdateThread now resolves the per-phone unarchive decision and the inbound unread watermark in a single atomic repository.UpdateActivity call. Added an Unarchive flag to MessageThreadActivityUpdate so is_archived=false is applied in the same transaction as the read-state/last-message updates. - Missed calls and inbound SMS both use MessageStatusReceived + MarksUnread, so they share the same unarchive + mark-unread rules. - Combined the message thread service test suites (read-receipt stubs plus shouldCheckUnarchive coverage) and updated the service/handler test helpers to the 5-arg NewMessageThreadService signature (adds phoneRepository). - Merged tests/README.md coverage entries for both E2E suites. - Regenerated Swagger and web/shared/types/api.ts so both main's new fields and read-receipt fields are present. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: bf00a0ac-e11f-4015-b295-3cdd9b491229
Summary
Adds a per-phone Unarchive Thread setting (JSON
unarchive_thread). When enabled, an archived message thread is automatically moved back to the inbox when a new inbound message is received for that phone. Disabled by default.Scope
API + web (no Android changes).
How it works (data flow)
Phone.UnarchiveThreadentity field (gorm:"default:false").MessageThreadService.UpdateThreaduses the pure helpershouldCheckUnarchive(thread archived AND status=received) to gate a lazy phone load: only when an inbound message lands on an already-archived thread does it read the phone viaPhoneRepository.Loadand checkUnarchiveThread. The common path (thread not archived) does zero extra DB reads.PUT /v1/phonesacceptsunarchive_threadvia the existing partial-update pattern (applied only when present).Web
updatePhonestore action sendsunarchive_thread.VSwitchbound toactivePhone.unarchive_thread.Testing
TestShouldCheckUnarchive(all branches of the predicate).TestUnarchiveThreadOnReceive_Enabled/_Disabledintests/(raw HTTP against the live Docker stack).go build ./...clean;go test -vet=off ./...passes; webpnpm lintclean.🤖 Generated with Copilot CLI