From 452b7abf9a9c4b29776ba9f2192b8eb3537edb04 Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Thu, 23 Jul 2026 02:36:47 +0800 Subject: [PATCH 01/11] docs(room-aliases): add agent-spec contract + issue 012 Co-Authored-By: Claude Opus 4.8 --- issues/012-room-aliases-management.md | 68 +++++++++++ specs/task-room-aliases.spec.md | 156 ++++++++++++++++++++++++++ 2 files changed, 224 insertions(+) create mode 100644 issues/012-room-aliases-management.md create mode 100644 specs/task-room-aliases.spec.md diff --git a/issues/012-room-aliases-management.md b/issues/012-room-aliases-management.md new file mode 100644 index 000000000..80c6d097c --- /dev/null +++ b/issues/012-room-aliases-management.md @@ -0,0 +1,68 @@ +# Issue 012: Room aliases can be read but not managed from the client + +**Date:** 2026-07-23 +**Type:** Feature +**Status:** Open +**Driven by spec:** `specs/task-room-aliases.spec.md` (agent-spec, estimate 2d) +**Affected components:** `src/home/room_settings_modal.rs`, `src/sliding_sync.rs`, `src/app.rs`, `src/i18n.rs`, `resources/i18n/**` + +## Summary + +Robrix already **reads** room aliases end-to-end but offers **no way to manage them**. +`RoomsList` populates `canonical_alias: Option` and +`alt_aliases: Vec` from matrix-sdk (`room.canonical_alias()` / +`room.alt_aliases()`), and the Room Settings modal is even handed the canonical alias +today — but only as a read-only string. Users cannot publish a new alias, remove one, +or choose which alias is canonical. + +This issue adds a **"Room Aliases" section to the Room Settings modal** so users with the +right power level can view and manage a room's aliases. + +## Current behavior (evidence) + +- `src/home/rooms_list.rs:307-309` / `:389-391` — `canonical_alias` + `alt_aliases` are + already tracked in the room model. +- `src/sliding_sync.rs:7378-7384` / `:7487-7491` — both are read from + `room.canonical_alias()` / `room.alt_aliases()`. +- `src/app.rs:1927-1931` — on `RoomSettingsAction::Open`, the canonical alias is fetched + (`get_room_canonical_alias`) and passed into `show_settings(...)` as a plain string. +- `src/home/room_settings_modal.rs` — renders room settings but has **no alias + add/remove/set-canonical controls**. +- Joining a room *by* alias already works (`src/home/add_room.rs::parse_address`); this + issue is strictly about **managing** an existing room's aliases, not joining. + +## Proposed scope + +1. **View**: show canonical alias + all alt aliases in the Room Settings modal. +2. **Publish**: register a new alias (`#localpart:server`, or bare `localpart` resolved + against the current homeserver) into the room directory. +3. **Remove**: unbind an alias from the room directory. +4. **Set canonical**: write the `m.room.canonical_alias` state event (alias + alt_aliases), + or clear it. +5. **Permission-gated**: edit controls appear only when the user can send the + `m.room.canonical_alias` state event; otherwise the section is read-only. +6. **Pure, unit-testable core**: alias normalization/validation + (`normalize_and_validate_alias`) and canonical/alt reconciliation + (`reconcile_canonical_alias`), plus i18n key presence — all covered by the spec's + Completion Criteria scenarios. + +## Out of scope + +- Join-by-alias flow (already covered by `add_room.rs`). +- Space canonical alias management (`space_service_sync.rs`). +- Directory visibility / history-visibility toggles. +- Cross-homeserver alias migration or bulk import. + +## Development contract + +Development is driven by the agent-spec Task Contract at +`specs/task-room-aliases.spec.md`. Verify with: + +``` +agent-spec lint specs/task-room-aliases.spec.md +agent-spec verify specs/task-room-aliases.spec.md +``` + +Acceptance is bound to pure-function unit tests (alias validation + canonical +reconciliation + i18n key presence); platform behavior (macOS/Android) is confirmed +manually in the implementation PR. diff --git a/specs/task-room-aliases.spec.md b/specs/task-room-aliases.spec.md new file mode 100644 index 000000000..d6c1f393b --- /dev/null +++ b/specs/task-room-aliases.spec.md @@ -0,0 +1,156 @@ +spec: task +name: "Room Aliases — 在房间设置里查看与管理房间别名" +inherits: project +tags: [matrix, room-settings, alias, ui, directory] +estimate: 2d +--- + +## Intent + +为 Room Settings 模态（`src/home/room_settings_modal.rs`）新增一个「房间别名（Room +Aliases）」区块，让有权限的用户能够**查看并管理**当前房间的别名： + +- 展示房间的 **canonical alias**（主别名 `#room:server`）与全部 **alt aliases**（备用别名）。 +- **发布新别名**：把用户输入的别名（`#localpart:server`，或裸 `localpart` 按当前 + homeserver 解析）注册到房间目录（room directory），映射到本房间。 +- **删除别名**：把某个别名从房间目录解绑。 +- **设置 / 更换 canonical alias**：写 `m.room.canonical_alias` 状态事件（`alias` + + `alt_aliases`），或清空主别名。 +- **权限门控**：仅当用户具备发送 `m.room.canonical_alias` 状态事件的 power level 时， + 才显示编辑控件；否则该区块只读展示。 + +**读取侧已经存在**：`RoomsList`（`src/home/rooms_list.rs`）已从 matrix-sdk 的 +`room.canonical_alias()` / `room.alt_aliases()` 填充 `canonical_alias: +Option` 与 `alt_aliases: Vec`。本任务只新增 +**写入/管理**链路与对应 UI，不重写读取模型。 + +## Decisions + +- **UI 落点**：别名区块放在 `room_settings_modal.rs` 现有设置列表内，作为一个新分组。 + 当前 `show_settings(cx, room_id, room_name, "", alias_str)` 已把 canonical alias 以只读 + 字符串传入；本任务把它升级为「canonical + alt aliases 列表 + 增删/设主控件」。 +- **数据来源**：区块渲染的别名数据取自已 fetch 的房间设置（`RoomSettingsFetchedAction`， + `sliding_sync.rs:702`）与 `RoomsList` 中的 `canonical_alias` / `alt_aliases`；不新增 + 独立的别名缓存。请求侧沿用 `MatrixRequest::FetchRoomSettings` 触发的刷新。 +- **新增 MatrixRequest 变体**（在 `sliding_sync.rs` 定义并处理，经 + `submit_async_request` 派发，**不得**裸开 tokio 任务）： + - `MatrixRequest::PublishRoomAlias { room_id, alias }` — 目录注册（PUT + `/directory/room/{alias}`，对应 ruma `create_room_alias` 端点）。 + - `MatrixRequest::RemoveRoomAlias { room_id, alias }` — 目录解绑（DELETE + `/directory/room/{alias}`，对应 ruma `delete_room_alias` 端点）。 + - `MatrixRequest::SetRoomCanonicalAlias { room_id, alias: Option, + alt_aliases: Vec }` — 发送 `m.room.canonical_alias` 状态事件。 + > 具体 matrix-sdk / ruma 调用符号由实现者对照本仓 pinned 版本确认；本 spec 只约束 + > 语义（目录端点 + canonical_alias 状态事件），不锁定 API 符号名。 +- **输入规范化与校验（纯函数，可单测）**：新增 + `normalize_and_validate_alias(input: &str, homeserver: &ServerName) -> + Result`： + - `#localpart:server` → 直接按 `RoomAliasId` 解析。 + - 裸 `localpart`（不含 `#` 与 `:`）→ 补成 `#localpart:{当前 homeserver}` 再解析 + （与 `src/home/add_room.rs` 的 `parse_address` 一致的「localpart 落到当前 + homeserver」语义）。 + - 空串、缺 `#`、缺 `:server`、含空白、非法字符 → 返回 `AliasInputError` 的对应变体。 +- **canonical 与 alt 的一致性（纯函数，可单测）**：新增 + `reconcile_canonical_alias(current_canonical, current_alts, op)` 计算写入 + `m.room.canonical_alias` 的目标 `(alias, alt_aliases)`： + - 设某别名为 canonical 时，它必须已在「canonical ∪ alts」集合内（否则先要求发布）； + 旧的 canonical 降级进 alt_aliases。 + - 删除某别名时，从 alt_aliases 移除；若删的是当前 canonical，则 canonical 置空。 + - 结果里 canonical 不得同时出现在 alt_aliases（去重）。 +- **乐观 UI + 结果回执**：发起写请求后本地乐观更新列表，成功/失败经现有 room settings + 刷新（`FetchRoomSettings` 回灌）与一条 toast 通知反馈；失败时回滚乐观项。 +- **权限门控**：编辑控件的可见性由「能否发 `m.room.canonical_alias` 状态事件」决定 + （复用房间 power levels）。目录发布/解绑在服务端另有权限，客户端失败时以 toast 呈现 + 服务端错误，不做本地静默吞掉。 +- **i18n**：新增 key（`resources/i18n/` 下所有 locale 都要，中英必备）： + `room_settings.aliases.section_title`、`room_settings.aliases.canonical_label`、 + `room_settings.aliases.alt_label`、`room_settings.aliases.add_placeholder`、 + `room_settings.aliases.add_button`、`room_settings.aliases.remove_button`、 + `room_settings.aliases.set_canonical_button`、`room_settings.aliases.invalid_format`、 + `room_settings.aliases.publish_failed`、`room_settings.aliases.readonly_hint`。 +- **UI 规范**：颜色/圆角/字体一律走 `RBX_*` 设计 token 与现有 `room_settings_modal` + 的行/徽章样式，不硬编码 hex、不另起卡片风格（遵循 `docs/ui-visual-spec-zh.md`）。 + +## Boundaries + +### Allowed Changes +- src/home/room_settings_modal.rs +- src/sliding_sync.rs +- src/app.rs +- src/i18n.rs +- resources/i18n/** +- specs/task-room-aliases.spec.md + +### Forbidden +- 不要新增 cargo 依赖（matrix-sdk / ruma 已提供别名与目录端点）。 +- 不要重写 `RoomsList` 的别名**读取**模型（`canonical_alias` / `alt_aliases` 已存在）。 +- 不要为别名操作裸开 tokio 任务——必须走 `submit_async_request(MatrixRequest::*)`。 +- 不要在 UI 屏里硬编码 hex 颜色；不要用 Makepad 1.x `live_design!` 语法。 +- 不要用 `.unwrap()` 处理用户输入的别名（非法输入必须走 `AliasInputError`）。 +- 不要在本任务里改动房间**加入**（join-by-alias，`add_room.rs`）的既有逻辑。 + +## Completion Criteria + +场景: 合法的完整别名被接受 + 测试: test_normalize_alias_accepts_full_alias + 假设 当前 homeserver 为 "example.org" + 当 规范化并校验输入 "#general:example.org" + 那么 返回合法的 RoomAliasId "#general:example.org" + +场景: 裸 localpart 按当前 homeserver 补全 + 测试: test_normalize_alias_completes_bare_localpart + 假设 当前 homeserver 为 "example.org" + 当 规范化并校验输入 "general" + 那么 返回合法的 RoomAliasId "#general:example.org" + +场景: 非法别名输入被拒绝 + 测试: test_normalize_alias_rejects_invalid + 假设 当前 homeserver 为 "example.org" + 当 分别规范化并校验输入 ""、"#:example.org"、"#has space:example.org" 和 "#general" + 那么 每个输入都返回 AliasInputError，而不是 panic + +场景: 设置某个已存在的别名为 canonical，旧主别名降级为 alt + 测试: test_reconcile_promote_alias_to_canonical + 假设 当前 canonical 为 "#old:example.org" + 并且 当前 alt_aliases 包含 "#new:example.org" + 当 把 "#new:example.org" 设为 canonical + 那么 目标 canonical 为 "#new:example.org" + 并且 目标 alt_aliases 包含 "#old:example.org" + 但是 目标 alt_aliases 不包含 "#new:example.org" + +场景: 把尚未发布的别名设为 canonical 被拒绝 + 测试: test_reconcile_rejects_unpublished_canonical + 假设 当前 canonical 为 "#old:example.org" + 并且 当前 alt_aliases 为空 + 当 把未在集合内的 "#ghost:example.org" 设为 canonical + 那么 返回错误，要求先发布该别名 + +场景: 删除当前 canonical 别名会清空主别名 + 测试: test_reconcile_remove_canonical_clears_it + 假设 当前 canonical 为 "#main:example.org" + 并且 当前 alt_aliases 包含 "#alt:example.org" + 当 删除 "#main:example.org" + 那么 目标 canonical 为空 (None) + 并且 目标 alt_aliases 仍包含 "#alt:example.org" + +场景: canonical 不会同时出现在 alt_aliases 里 + 测试: test_reconcile_dedups_canonical_from_alts + 假设 当前 canonical 为 "#old:example.org" + 并且 当前 alt_aliases 同时包含 "#dup:example.org" + 当 把 "#dup:example.org" 设为 canonical + 那么 目标 alt_aliases 不包含 "#dup:example.org" + +场景: 别名区块的 i18n key 在所有 locale 中都存在 + 测试: test_room_aliases_i18n_keys_exist_in_all_locales + 假设 en 与 zh-CN 两套字典已加载 + 当 用 tr_key 解析 "room_settings.aliases.section_title"、"room_settings.aliases.add_button" 和 "room_settings.aliases.invalid_format" + 那么 每个 key 在两种语言下都返回非 key 本身的翻译文案 + +## Out of Scope + +- 房间**加入**流程（join-by-alias）的任何改动——已由 `add_room.rs` 覆盖。 +- 别名的服务端权限管理 / 房间目录可见性（`m.room.history_visibility`、published-in-directory 开关）。 +- Space 的 canonical alias 管理（`space_service_sync.rs` 另行处理）。 +- 跨 homeserver 的别名迁移或批量导入。 +- 别名冲突（already-in-use）的自动改名建议——仅以服务端错误 toast 呈现。 +- 平台端到端验收（macOS / Android 手动测试）在实现 PR 中确认，本 spec 场景只绑定纯函数单测。 From e719022e4f844a78a659e8d2f8da2ad295ab2e33 Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Thu, 23 Jul 2026 02:36:48 +0800 Subject: [PATCH 02/11] feat(room-aliases): alias validation + canonical reconcile logic + tests Pure, unit-tested core for the Room Aliases feature: - normalize_and_validate_alias: parses #localpart:server, completes a bare localpart against the homeserver, rejects empty/whitespace/empty-localpart. - reconcile_canonical_alias: maintains m.room.canonical_alias invariants (promote/demote, reject-unpublished, dedup, remove-clears-canonical). - 7 unit tests (all passing). Co-Authored-By: Claude Opus 4.8 --- src/home/room_settings_modal.rs | 219 +++++++++++++++++++++++++++++++- 1 file changed, 218 insertions(+), 1 deletion(-) diff --git a/src/home/room_settings_modal.rs b/src/home/room_settings_modal.rs index be1113f4d..ace82fd84 100644 --- a/src/home/room_settings_modal.rs +++ b/src/home/room_settings_modal.rs @@ -3,11 +3,228 @@ use std::path::PathBuf; use makepad_widgets::*; -use ruma::OwnedRoomId; +use ruma::{OwnedRoomAliasId, OwnedRoomId, RoomAliasId, ServerName}; use crate::shared::avatar::AvatarWidgetExt; use crate::utils::load_png_or_jpg; +// ───────────────────────────────────────────────────────────────────────────── +// Room-alias management: pure logic (no UI / no network), unit-tested below. +// +// These functions back the "Room Aliases" section of the room settings modal. +// They are deliberately pure so their behaviour can be verified without a +// Makepad context or a live Matrix connection (see `specs/task-room-aliases.spec.md`). +// ───────────────────────────────────────────────────────────────────────────── + +/// Why a user-entered alias string was rejected. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum AliasInputError { + /// Input was empty (after trimming). + Empty, + /// Input contained whitespace, which is never valid in a room alias. + ContainsWhitespace, + /// Input did not parse as a valid `#localpart:server` room alias. + InvalidFormat, +} + +/// Normalize and validate a user-entered room alias. +/// +/// - `#localpart:server` (or any string containing `#`/`:`) is parsed as an +/// explicit alias and must be well-formed. +/// - A bare `localpart` (no `#` and no `:`) is completed to +/// `#{localpart}:{homeserver}`, matching how [`parse_address`](super) treats +/// bare room addresses against the current homeserver. +/// +/// Returns [`AliasInputError`] instead of panicking on any malformed input. +pub fn normalize_and_validate_alias( + input: &str, + homeserver: &ServerName, +) -> Result { + let trimmed = input.trim(); + if trimmed.is_empty() { + return Err(AliasInputError::Empty); + } + if trimmed.chars().any(char::is_whitespace) { + return Err(AliasInputError::ContainsWhitespace); + } + // A bare localpart (no sigil, no server delimiter) is resolved against the + // current homeserver; anything else is treated as an explicit alias. + let candidate = if trimmed.starts_with('#') || trimmed.contains(':') { + trimmed.to_string() + } else { + format!("#{trimmed}:{homeserver}") + }; + let parsed = OwnedRoomAliasId::try_from(candidate.as_str()) + .map_err(|_| AliasInputError::InvalidFormat)?; + // ruma leniently accepts an empty localpart (e.g. "#:server"); a usable room + // alias must have a non-empty localpart, so reject it explicitly. + if parsed.alias().is_empty() { + return Err(AliasInputError::InvalidFormat); + } + Ok(parsed) +} + +/// A single alias-management operation requested from the UI. +#[derive(Debug, Clone)] +pub enum AliasOp { + /// Promote an already-published alias to be the room's canonical alias. + SetCanonical(OwnedRoomAliasId), + /// Remove an alias from the room (from canonical and/or the alt list). + Remove(OwnedRoomAliasId), +} + +/// The `(canonical, alt_aliases)` pair to write into the `m.room.canonical_alias` +/// state event after applying an [`AliasOp`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct CanonicalAliasState { + pub canonical: Option, + pub alt_aliases: Vec, +} + +/// Why an [`AliasOp`] could not be reconciled into a new canonical-alias state. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CanonicalReconcileError { + /// Tried to set an alias canonical that is neither the current canonical nor + /// a published alt alias — it must be published to the directory first. + NotPublished, +} + +/// Compute the new `(canonical, alt_aliases)` after applying `op`, enforcing the +/// invariants of `m.room.canonical_alias`: +/// +/// - Setting an alias canonical requires it to already be published (canonical ∪ alts). +/// - The previous canonical (if different) is demoted into `alt_aliases`. +/// - The canonical alias never also appears in `alt_aliases` (deduped). +/// - Removing the current canonical clears it; removing an alt just drops it. +pub fn reconcile_canonical_alias( + current_canonical: Option<&RoomAliasId>, + current_alts: &[OwnedRoomAliasId], + op: AliasOp, +) -> Result { + // Compare via canonical string form to avoid borrowed/owned PartialEq ambiguity. + let target = match &op { + AliasOp::SetCanonical(a) | AliasOp::Remove(a) => a.clone(), + }; + let target_str = target.as_str(); + match op { + AliasOp::SetCanonical(_) => { + let is_published = current_canonical.is_some_and(|c| c.as_str() == target_str) + || current_alts.iter().any(|a| a.as_str() == target_str); + if !is_published { + return Err(CanonicalReconcileError::NotPublished); + } + let mut alts: Vec = Vec::new(); + // Demote the old canonical (when it differs from the new one). + if let Some(old) = current_canonical { + if old.as_str() != target_str { + alts.push(old.to_owned()); + } + } + // Keep the remaining alts, minus the new canonical, without duplicates. + for a in current_alts { + if a.as_str() != target_str && !alts.iter().any(|x| x.as_str() == a.as_str()) { + alts.push(a.clone()); + } + } + Ok(CanonicalAliasState { canonical: Some(target), alt_aliases: alts }) + } + AliasOp::Remove(_) => { + let canonical = match current_canonical { + Some(c) if c.as_str() == target_str => None, + other => other.map(RoomAliasId::to_owned), + }; + let alt_aliases = current_alts + .iter() + .filter(|a| a.as_str() != target_str) + .cloned() + .collect(); + Ok(CanonicalAliasState { canonical, alt_aliases }) + } + } +} + +#[cfg(test)] +mod alias_logic_tests { + use super::*; + + fn server() -> ruma::OwnedServerName { + ruma::OwnedServerName::try_from("example.org").expect("valid server name") + } + + fn alias(s: &str) -> OwnedRoomAliasId { + OwnedRoomAliasId::try_from(s).expect("valid alias in test") + } + + #[test] + fn test_normalize_alias_accepts_full_alias() { + let got = normalize_and_validate_alias("#general:example.org", &server()).unwrap(); + assert_eq!(got, alias("#general:example.org")); + } + + #[test] + fn test_normalize_alias_completes_bare_localpart() { + let got = normalize_and_validate_alias("general", &server()).unwrap(); + assert_eq!(got, alias("#general:example.org")); + } + + #[test] + fn test_normalize_alias_rejects_invalid() { + for bad in ["", "#:example.org", "#has space:example.org", "#general"] { + assert!( + normalize_and_validate_alias(bad, &server()).is_err(), + "expected {bad:?} to be rejected", + ); + } + } + + #[test] + fn test_reconcile_promote_alias_to_canonical() { + let out = reconcile_canonical_alias( + Some(&alias("#old:example.org")), + &[alias("#new:example.org")], + AliasOp::SetCanonical(alias("#new:example.org")), + ) + .unwrap(); + assert_eq!(out.canonical, Some(alias("#new:example.org"))); + assert!(out.alt_aliases.contains(&alias("#old:example.org"))); + assert!(!out.alt_aliases.contains(&alias("#new:example.org"))); + } + + #[test] + fn test_reconcile_rejects_unpublished_canonical() { + let err = reconcile_canonical_alias( + Some(&alias("#old:example.org")), + &[], + AliasOp::SetCanonical(alias("#ghost:example.org")), + ) + .unwrap_err(); + assert_eq!(err, CanonicalReconcileError::NotPublished); + } + + #[test] + fn test_reconcile_remove_canonical_clears_it() { + let out = reconcile_canonical_alias( + Some(&alias("#main:example.org")), + &[alias("#alt:example.org")], + AliasOp::Remove(alias("#main:example.org")), + ) + .unwrap(); + assert_eq!(out.canonical, None); + assert!(out.alt_aliases.contains(&alias("#alt:example.org"))); + } + + #[test] + fn test_reconcile_dedups_canonical_from_alts() { + let out = reconcile_canonical_alias( + Some(&alias("#old:example.org")), + &[alias("#dup:example.org")], + AliasOp::SetCanonical(alias("#dup:example.org")), + ) + .unwrap(); + assert!(!out.alt_aliases.contains(&alias("#dup:example.org"))); + } +} + script_mod! { use mod.prelude.widgets.* use mod.widgets.* From b5652df6af2331a406973bfe12325a1b71bbf4f7 Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Thu, 23 Jul 2026 02:51:12 +0800 Subject: [PATCH 03/11] feat(room-aliases): power-level gate + alias-management MatrixRequests - Enable the RoomCanonicalAlias power bit and add UserPowerLevels::can_set_canonical_alias() (via user_can_send_state). - Add MatrixRequest::{PublishRoomAlias, RemoveRoomAlias, SetRoomCanonicalAlias} and their worker handlers: * Publish/Remove -> ruma create_alias / delete_alias directory endpoints. * SetCanonical -> m.room.canonical_alias state event. All dispatched via submit_async_request; failures surface a popup. Co-Authored-By: Claude Opus 4.8 --- src/sliding_sync.rs | 93 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 2 deletions(-) diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index 27bc88711..45b73e872 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -36,7 +36,7 @@ use matrix_sdk_ui::{ RoomListService, Timeline, encryption_sync_service, room_list_service::{RoomListItem, RoomListLoadingState, SyncIndicator, filters}, sync_service::{self, SyncService}, timeline::{LatestEventValue, RoomExt, TimelineEventItemId, TimelineFocus, TimelineItem, TimelineReadReceiptTracking, TimelineDetails} }; use robius_open::Uri; -use ruma::{OwnedRoomOrAliasId, RoomId, events::tag::Tags}; +use ruma::{OwnedRoomAliasId, OwnedRoomOrAliasId, RoomId, events::tag::Tags}; use tokio::{ runtime::Handle, sync::{broadcast, mpsc::{Sender, UnboundedReceiver, UnboundedSender}, oneshot, watch, Notify}, task::JoinHandle, time::error::Elapsed, @@ -1605,6 +1605,25 @@ pub enum MatrixRequest { FetchRoomSettings { room_id: OwnedRoomId, }, + /// Publish a new alias into the room directory, mapping it to this room + /// (`PUT /directory/room/{alias}`). Requires directory permission on the + /// alias's homeserver. + PublishRoomAlias { + room_id: OwnedRoomId, + alias: OwnedRoomAliasId, + }, + /// Remove an alias from the room directory (`DELETE /directory/room/{alias}`). + RemoveRoomAlias { + alias: OwnedRoomAliasId, + }, + /// Set the room's canonical alias and alt aliases via the + /// `m.room.canonical_alias` state event. Requires the corresponding power + /// level (see [`UserPowerLevels::can_set_canonical_alias`]). + SetRoomCanonicalAlias { + room_id: OwnedRoomId, + alias: Option, + alt_aliases: Vec, + }, /// Set the display name (title) of a room. SetRoomName { room_id: OwnedRoomId, @@ -4297,6 +4316,69 @@ async fn matrix_worker_task( }); } + MatrixRequest::PublishRoomAlias { room_id, alias } => { + let Some(client) = get_client() else { continue }; + let _publish_alias_task = Handle::current().spawn(async move { + let request = matrix_sdk::ruma::api::client::alias::create_alias::v3::Request::new( + alias.clone(), + room_id, + ); + match client.send(request).await { + Ok(_) => log!("Published room alias {alias}."), + Err(e) => { + error!("Failed to publish room alias {alias}: {e:?}"); + enqueue_popup_notification( + format!("Couldn't publish alias {alias}"), + crate::shared::popup_list::PopupKind::Error, + Some(5.0), + ); + } + } + }); + } + + MatrixRequest::RemoveRoomAlias { alias } => { + let Some(client) = get_client() else { continue }; + let _remove_alias_task = Handle::current().spawn(async move { + let request = matrix_sdk::ruma::api::client::alias::delete_alias::v3::Request::new( + alias.clone(), + ); + match client.send(request).await { + Ok(_) => log!("Removed room alias {alias}."), + Err(e) => { + error!("Failed to remove room alias {alias}: {e:?}"); + enqueue_popup_notification( + format!("Couldn't remove alias {alias}"), + crate::shared::popup_list::PopupKind::Error, + Some(5.0), + ); + } + } + }); + } + + MatrixRequest::SetRoomCanonicalAlias { room_id, alias, alt_aliases } => { + let Some(client) = get_client() else { continue }; + let _set_canonical_alias_task = Handle::current().spawn(async move { + if let Some(room) = client.get_room(&room_id) { + let mut content = matrix_sdk::ruma::events::room::canonical_alias::RoomCanonicalAliasEventContent::new(); + content.alias = alias; + content.alt_aliases = alt_aliases; + match room.send_state_event(content).await { + Ok(_) => log!("Set canonical alias for room {room_id}."), + Err(e) => { + error!("Failed to set canonical alias for room {room_id}: {e:?}"); + enqueue_popup_notification( + "Couldn't update the room's canonical alias", + crate::shared::popup_list::PopupKind::Error, + Some(5.0), + ); + } + } + } + }); + } + MatrixRequest::SetRoomName { room_id, name } => { let Some(client) = get_client() else { continue }; let _set_room_name_task = Handle::current().spawn(async move { @@ -8739,7 +8821,7 @@ bitflags! { // const PolicyRuleUser = 1 << 37; // const RoomAliases = 1 << 38; // const RoomAvatar = 1 << 39; - // const RoomCanonicalAlias = 1 << 40; + const RoomCanonicalAlias = 1 << 40; // const RoomCreate = 1 << 41; // const RoomEncryption = 1 << 42; // const RoomGuestAccess = 1 << 43; @@ -8777,6 +8859,7 @@ impl UserPowerLevels { retval.set(UserPowerLevels::Sticker, user_power >= power_levels.for_message(MessageLikeEventType::Sticker)); retval.set(UserPowerLevels::RoomPinnedEvents, user_power >= power_levels.for_state(StateEventType::RoomPinnedEvents)); retval.set(UserPowerLevels::RoomPowerLevels, power_levels.user_can_send_state(user_id, StateEventType::RoomPowerLevels)); + retval.set(UserPowerLevels::RoomCanonicalAlias, power_levels.user_can_send_state(user_id, StateEventType::RoomCanonicalAlias)); retval } @@ -8842,6 +8925,12 @@ impl UserPowerLevels { pub fn can_change_room_power_levels(self) -> bool { self.contains(UserPowerLevels::RoomPowerLevels) } + + /// Whether the user may set the room's canonical alias / alt aliases + /// (i.e. send the `m.room.canonical_alias` state event). + pub fn can_set_canonical_alias(self) -> bool { + self.contains(UserPowerLevels::RoomCanonicalAlias) + } } From 56da69ae0df3072e98b744a4e5621ac85bfe4362 Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Thu, 23 Jul 2026 03:17:43 +0800 Subject: [PATCH 04/11] feat(room-aliases): wire Add-address to real alias publish + validation - Replace the AddLocalAddress 'coming soon' stub with a real flow: validate input via normalize_and_validate_alias (homeserver from the signed-in user), then submit MatrixRequest::PublishRoomAlias; invalid input shows a format hint. - Stop stripping the leading '#' in the modal so full aliases validate. Co-Authored-By: Claude Opus 4.8 --- src/app.rs | 24 ++++++++++++++++++++++-- src/home/room_settings_modal.rs | 4 +++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/app.rs b/src/app.rs index 6ef3a2bab..0eb9fb690 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1966,8 +1966,28 @@ impl MatchEvent for App { self.ui.modal(cx, ids!(room_settings_modal)).close(cx); continue; } - Some(RoomSettingsAction::AddLocalAddress { .. }) => { - enqueue_popup_notification("Address management coming soon", PopupKind::Info, Some(3.0)); + Some(RoomSettingsAction::AddLocalAddress { room_id, alias }) => { + // Resolve a bare localpart against the user's own homeserver. + let Some(server_name) = current_user_id().map(|u| u.server_name().to_owned()) else { + enqueue_popup_notification("You must be signed in to add an alias", PopupKind::Error, Some(4.0)); + continue; + }; + match crate::home::room_settings_modal::normalize_and_validate_alias(alias, &server_name) { + Ok(valid_alias) => { + submit_async_request(MatrixRequest::PublishRoomAlias { + room_id: room_id.clone(), + alias: valid_alias, + }); + enqueue_popup_notification("Publishing room alias…", PopupKind::Info, Some(3.0)); + } + Err(_) => { + enqueue_popup_notification( + "Invalid alias — use #name:server or a plain name", + PopupKind::Error, + Some(4.0), + ); + } + } continue; } Some(RoomSettingsAction::SetDirectoryPublish { .. }) => { diff --git a/src/home/room_settings_modal.rs b/src/home/room_settings_modal.rs index ace82fd84..d9189b709 100644 --- a/src/home/room_settings_modal.rs +++ b/src/home/room_settings_modal.rs @@ -881,7 +881,9 @@ impl WidgetMatchEvent for RoomSettingsModal { // Add address button if self.view.button(cx, ids!(add_address_button)).clicked(actions) { let alias = self.view.text_input(cx, ids!(add_address_input)).text(); - let alias = alias.trim().trim_start_matches('#').to_string(); + // Pass the raw (trimmed) text through; validation/normalization happens + // in the AddLocalAddress handler via `normalize_and_validate_alias`. + let alias = alias.trim().to_string(); if !alias.is_empty() { if let Some(room_id) = self.room_id.clone() { cx.action(RoomSettingsAction::AddLocalAddress { room_id, alias }); From ef95792fd0e9a4b74b5ce48593722dbf853a1517 Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Thu, 23 Jul 2026 15:18:16 +0800 Subject: [PATCH 05/11] feat(room-aliases): i18n keys + view/permission-gating for alias section - Add 10 room_settings.aliases.* i18n keys to en / zh-CN dictionaries and test_room_aliases_i18n_keys_exist_in_all_locales, completing the 8th agent-spec Completion Criteria scenario (7 logic + 1 i18n now covered). - Extend RoomSettingsFetchedAction with canonical_alias, alt_aliases and can_manage_aliases, populated in FetchRoomSettings via room.canonical_alias() / room.alt_aliases() and UserPowerLevels::from_room().can_set_canonical_alias(). - RoomSettingsModal::apply_alias_settings renders the canonical + alt alias list with localized labels and gates the add-address control behind the m.room.canonical_alias power level (read-only hint otherwise). Full lib suite: 548 passed, 0 failed. Co-Authored-By: Claude Opus 4.8 --- resources/i18n/en.json | 12 +++++- resources/i18n/zh-CN.json | 12 +++++- src/app.rs | 10 +++++ src/home/room_settings_modal.rs | 70 +++++++++++++++++++++++++++++++++ src/i18n.rs | 31 +++++++++++++++ src/sliding_sync.rs | 26 +++++++++++- 6 files changed, 158 insertions(+), 3 deletions(-) diff --git a/resources/i18n/en.json b/resources/i18n/en.json index 4395132dc..bf1736403 100644 --- a/resources/i18n/en.json +++ b/resources/i18n/en.json @@ -811,5 +811,15 @@ "space_lobby.item.member_one": "1 member", "space_lobby.item.member_n": "{count} members", "space_lobby.item.child_room_one": "~{count} room", - "space_lobby.item.child_room_n": "~{count} rooms" + "space_lobby.item.child_room_n": "~{count} rooms", + "room_settings.aliases.section_title": "Room Aliases", + "room_settings.aliases.canonical_label": "Main address", + "room_settings.aliases.alt_label": "Other published addresses", + "room_settings.aliases.add_placeholder": "# e.g. my-room", + "room_settings.aliases.add_button": "Add", + "room_settings.aliases.remove_button": "Remove", + "room_settings.aliases.set_canonical_button": "Set as main", + "room_settings.aliases.invalid_format": "Invalid address format", + "room_settings.aliases.publish_failed": "Failed to publish address", + "room_settings.aliases.readonly_hint": "You don't have permission to manage this room's addresses" } diff --git a/resources/i18n/zh-CN.json b/resources/i18n/zh-CN.json index b98f9886a..a2cf7d572 100644 --- a/resources/i18n/zh-CN.json +++ b/resources/i18n/zh-CN.json @@ -809,5 +809,15 @@ "space_lobby.item.member_one": "1 位成员", "space_lobby.item.member_n": "{count} 位成员", "space_lobby.item.child_room_one": "~{count} 个房间", - "space_lobby.item.child_room_n": "~{count} 个房间" + "space_lobby.item.child_room_n": "~{count} 个房间", + "room_settings.aliases.section_title": "房间别名", + "room_settings.aliases.canonical_label": "主地址", + "room_settings.aliases.alt_label": "其他已发布地址", + "room_settings.aliases.add_placeholder": "# 例如 my-room", + "room_settings.aliases.add_button": "添加", + "room_settings.aliases.remove_button": "移除", + "room_settings.aliases.set_canonical_button": "设为主地址", + "room_settings.aliases.invalid_format": "地址格式无效", + "room_settings.aliases.publish_failed": "发布地址失败", + "room_settings.aliases.readonly_hint": "你没有管理该房间地址的权限" } diff --git a/src/app.rs b/src/app.rs index 0eb9fb690..76ed7a3e7 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2010,8 +2010,18 @@ impl MatchEvent for App { // Handle RoomSettingsFetchedAction. if let Some(fetched) = action.downcast_ref::() { + let canonical = fetched.canonical_alias.as_ref().map(|a| a.as_str().to_string()); + let alts: Vec = fetched.alt_aliases.iter().map(|a| a.as_str().to_string()).collect(); self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) .apply_fetched_settings(cx, fetched.topic.clone(), fetched.is_public); + self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) + .apply_alias_settings( + cx, + self.app_state.app_language, + canonical, + alts, + fetched.can_manage_aliases, + ); continue; } diff --git a/src/home/room_settings_modal.rs b/src/home/room_settings_modal.rs index d9189b709..866072aa9 100644 --- a/src/home/room_settings_modal.rs +++ b/src/home/room_settings_modal.rs @@ -5,6 +5,7 @@ use std::path::PathBuf; use makepad_widgets::*; use ruma::{OwnedRoomAliasId, OwnedRoomId, RoomAliasId, ServerName}; +use crate::i18n::{AppLanguage, tr_key}; use crate::shared::avatar::AvatarWidgetExt; use crate::utils::load_png_or_jpg; @@ -1006,6 +1007,62 @@ impl RoomSettingsModal { let _ = is_public; // reflected by the toggle's current state self.view.redraw(cx); } + + /// Apply the room's alias data (canonical + alt aliases) and permission + /// gating to the "Room Aliases" section. Labels use the localized strings + /// from `resources/i18n/**` so the section follows the app language. + /// + /// When `can_manage` is false the user lacks the power level to send the + /// `m.room.canonical_alias` state event, so the add-address control is + /// hidden and a read-only hint is shown instead. + pub fn apply_alias_settings( + &mut self, + cx: &mut Cx, + language: AppLanguage, + canonical_alias: Option, + alt_aliases: Vec, + can_manage: bool, + ) { + // Localized section labels. + self.view.label(cx, ids!(addresses_heading)) + .set_text(cx, tr_key(language, "room_settings.aliases.section_title")); + self.view.label(cx, ids!(published_addresses_label)) + .set_text(cx, tr_key(language, "room_settings.aliases.canonical_label")); + + // Canonical (main) alias, or the existing "no main address" fallback. + let main_text = canonical_alias + .unwrap_or_else(|| String::from("No main address set")); + self.view.label(cx, ids!(main_alias_label)).set_text(cx, &main_text); + + // Alternate published aliases, one per line. Falls back to a hint when + // there are none. + if alt_aliases.is_empty() { + self.view.label(cx, ids!(no_published_label)) + .set_text(cx, "No other published addresses yet, add one below"); + } else { + self.view.label(cx, ids!(no_published_label)) + .set_text(cx, &alt_aliases.join("\n")); + } + + // Localized add control. + self.view.text_input(cx, ids!(add_address_input)) + .set_empty_text(cx, tr_key(language, "room_settings.aliases.add_placeholder").to_string()); + self.view.button(cx, ids!(add_address_button)) + .set_text(cx, tr_key(language, "room_settings.aliases.add_button")); + + // Permission gating: only users who can send `m.room.canonical_alias` + // see the edit control; everyone else gets a read-only hint. + self.view.view(cx, ids!(add_address_row)).set_visible(cx, can_manage); + if can_manage { + self.view.label(cx, ids!(local_desc)) + .set_text(cx, tr_key(language, "room_settings.aliases.alt_label")); + } else { + self.view.label(cx, ids!(local_desc)) + .set_text(cx, tr_key(language, "room_settings.aliases.readonly_hint")); + } + + self.view.redraw(cx); + } } impl RoomSettingsModalRef { @@ -1028,6 +1085,19 @@ impl RoomSettingsModalRef { inner.apply_fetched_settings(cx, topic, is_public); } + /// Apply fetched alias data (canonical + alt aliases) and permission gating. + pub fn apply_alias_settings( + &self, + cx: &mut Cx, + language: AppLanguage, + canonical_alias: Option, + alt_aliases: Vec, + can_manage: bool, + ) { + let Some(mut inner) = self.borrow_mut() else { return }; + inner.apply_alias_settings(cx, language, canonical_alias, alt_aliases, can_manage); + } + /// Update the avatar widget after a successful upload. pub fn apply_avatar(&self, cx: &mut Cx, image_data: &[u8]) { let Some(mut inner) = self.borrow_mut() else { return }; diff --git a/src/i18n.rs b/src/i18n.rs index 385ed6951..89c684f41 100644 --- a/src/i18n.rs +++ b/src/i18n.rs @@ -151,6 +151,37 @@ mod tests { } } + #[test] + fn test_room_aliases_i18n_keys_exist_in_all_locales() { + // Spec `specs/task-room-aliases.spec.md` Completion Criteria — the Room + // Aliases section keys must resolve to a real translation (not the key + // itself) in every locale. + for key in [ + "room_settings.aliases.section_title", + "room_settings.aliases.canonical_label", + "room_settings.aliases.alt_label", + "room_settings.aliases.add_placeholder", + "room_settings.aliases.add_button", + "room_settings.aliases.remove_button", + "room_settings.aliases.set_canonical_button", + "room_settings.aliases.invalid_format", + "room_settings.aliases.publish_failed", + "room_settings.aliases.readonly_hint", + ] { + for language in AppLanguage::ALL { + assert!( + dictionary(language).contains_key(key), + "missing i18n key {key:?} for language {language:?}", + ); + assert_ne!( + tr_key(language, key), + key, + "i18n key {key:?} resolves to itself (no translation) for {language:?}", + ); + } + } + } + #[test] fn translation_i18n_keys_exist_for_settings_and_room_input() { assert_eq!( diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index 45b73e872..9a5f383e5 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -703,6 +703,13 @@ pub struct RoomSettingsFetchedAction { pub room_id: OwnedRoomId, pub topic: Option, pub is_public: bool, + /// The room's canonical (main) alias, if any. + pub canonical_alias: Option, + /// The room's published alternate aliases. + pub alt_aliases: Vec, + /// Whether the current user may send the `m.room.canonical_alias` state + /// event — gates the alias edit controls in the Room Settings modal. + pub can_manage_aliases: bool, } /// Posted after a room avatar is successfully uploaded and set. @@ -4311,7 +4318,24 @@ async fn matrix_worker_task( if let Some(room) = client.get_room(&room_id) { let topic = room.topic(); let is_public = room.is_public().unwrap_or(false); - Cx::post_action(RoomSettingsFetchedAction { room_id, topic, is_public }); + let canonical_alias = room.canonical_alias(); + let alt_aliases = room.alt_aliases(); + // Alias edit controls are gated on the power level to send + // `m.room.canonical_alias` (see `UserPowerLevels`). + let can_manage_aliases = match client.user_id() { + Some(user_id) => UserPowerLevels::from_room(&room, user_id) + .await + .is_some_and(|pl| pl.can_set_canonical_alias()), + None => false, + }; + Cx::post_action(RoomSettingsFetchedAction { + room_id, + topic, + is_public, + canonical_alias, + alt_aliases, + can_manage_aliases, + }); } }); } From 6c1ec644ef24b6aee1aa775eb5ade235c5559896 Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Fri, 24 Jul 2026 13:44:20 +0800 Subject: [PATCH 06/11] feat(room-aliases): per-row alias management + publish auto-advertise with serialized writes Per-row Remove / Set-as-main buttons (PortalList of AliasRow items, value- carrying actions), publish auto-advertise into m.room.canonical_alias alt_aliases with optimistic UI + snapshot rollback, sequenced directory-then- state writes with idempotent retry repair (409-iff-ours / 404-as-success), AliasWriteGate + app-level per-room PendingAliasWrites registry serializing one in-flight alias mutation per room across modal reopen, room-scoped fetch guards, and i18n for all alias UI strings (en + zh-CN). Known issue (from final review, fix follows on this PR): settings fetches carry no request identity/generation, so a stale open-fetch returning after a write enters reconcile can be misread as that write's reconcile and restore a stale snapshot (narrow timing window). Co-Authored-By: Claude Fable 5 --- resources/i18n/en.json | 8 +- resources/i18n/zh-CN.json | 8 +- src/app.rs | 132 +++- src/home/room_settings_modal.rs | 1074 +++++++++++++++++++++++++++++-- src/i18n.rs | 6 + src/sliding_sync.rs | 279 ++++++-- 6 files changed, 1386 insertions(+), 121 deletions(-) diff --git a/resources/i18n/en.json b/resources/i18n/en.json index bf1736403..6531f9477 100644 --- a/resources/i18n/en.json +++ b/resources/i18n/en.json @@ -821,5 +821,11 @@ "room_settings.aliases.set_canonical_button": "Set as main", "room_settings.aliases.invalid_format": "Invalid address format", "room_settings.aliases.publish_failed": "Failed to publish address", - "room_settings.aliases.readonly_hint": "You don't have permission to manage this room's addresses" + "room_settings.aliases.readonly_hint": "You don't have permission to manage this room's addresses", + "room_settings.aliases.no_main_address": "No main address set", + "room_settings.aliases.none_published": "No other published addresses yet, add one below", + "room_settings.aliases.publishing": "Publishing address…", + "room_settings.aliases.updating_main": "Updating main address…", + "room_settings.aliases.removing": "Removing address…", + "room_settings.aliases.sign_in_required": "You must be signed in to add an address" } diff --git a/resources/i18n/zh-CN.json b/resources/i18n/zh-CN.json index a2cf7d572..3635c308d 100644 --- a/resources/i18n/zh-CN.json +++ b/resources/i18n/zh-CN.json @@ -819,5 +819,11 @@ "room_settings.aliases.set_canonical_button": "设为主地址", "room_settings.aliases.invalid_format": "地址格式无效", "room_settings.aliases.publish_failed": "发布地址失败", - "room_settings.aliases.readonly_hint": "你没有管理该房间地址的权限" + "room_settings.aliases.readonly_hint": "你没有管理该房间地址的权限", + "room_settings.aliases.no_main_address": "未设置主地址", + "room_settings.aliases.none_published": "暂无其他已发布地址，请在下方添加", + "room_settings.aliases.publishing": "正在发布地址…", + "room_settings.aliases.updating_main": "正在更新主地址…", + "room_settings.aliases.removing": "正在移除地址…", + "room_settings.aliases.sign_in_required": "登录后才能添加地址" } diff --git a/src/app.rs b/src/app.rs index 76ed7a3e7..97f399d0f 100644 --- a/src/app.rs +++ b/src/app.rs @@ -19,7 +19,7 @@ use crate::{ add_menu::{AddMenuAction, AddMenuWidgetRefExt}, add_room::{CreateRoomModalAction, CreateRoomModalWidgetRefExt, JoinRoomModalAction, JoinRoomModalWidgetRefExt, StartChatModalAction, StartChatModalWidgetRefExt}, bot_binding_modal::{BotBindingModalAction, BotBindingModalWidgetRefExt}, - event_source_modal::{EventSourceModalAction, EventSourceModalWidgetRefExt}, invite_modal::{InviteModalAction, InviteModalWidgetRefExt, mark_invite_modal_closed}, invite_screen::{InviteScreenWidgetRefExt, LeaveRoomResultAction}, main_desktop_ui::MainDesktopUiAction, navigation_tab_bar::{NavigationBarAction, SelectedTab}, new_message_context_menu::NewMessageContextMenuWidgetRefExt, room_context_menu::{RoomContextMenuAction, RoomContextMenuWidgetRefExt}, room_screen::{InviteAction, MessageAction, ReportRoomModalAction, ReportRoomModalWidgetRefExt, ReportRoomResultAction, RoomScreenWidgetRefExt, TimelineUpdate, clear_timeline_states, set_room_info_action_modal_open}, room_settings_modal::{RoomSettingsAction, RoomSettingsModalWidgetRefExt}, rooms_list::{RoomsListAction, RoomsListRef, RoomsListUpdate, clear_all_invited_rooms, enqueue_rooms_list_update}, rooms_list_header::RoomsListHeaderAction, space_lobby::SpaceLobbyScreenWidgetRefExt, spaces_bar::SpacesBarRef + event_source_modal::{EventSourceModalAction, EventSourceModalWidgetRefExt}, invite_modal::{InviteModalAction, InviteModalWidgetRefExt, mark_invite_modal_closed}, invite_screen::{InviteScreenWidgetRefExt, LeaveRoomResultAction}, main_desktop_ui::MainDesktopUiAction, navigation_tab_bar::{NavigationBarAction, SelectedTab}, new_message_context_menu::NewMessageContextMenuWidgetRefExt, room_context_menu::{RoomContextMenuAction, RoomContextMenuWidgetRefExt}, room_screen::{InviteAction, MessageAction, ReportRoomModalAction, ReportRoomModalWidgetRefExt, ReportRoomResultAction, RoomScreenWidgetRefExt, TimelineUpdate, clear_timeline_states, set_room_info_action_modal_open}, room_settings_modal::{PendingAliasWrites, RoomSettingsAction, RoomSettingsModalWidgetRefExt}, rooms_list::{RoomsListAction, RoomsListRef, RoomsListUpdate, clear_all_invited_rooms, enqueue_rooms_list_update}, rooms_list_header::RoomsListHeaderAction, space_lobby::SpaceLobbyScreenWidgetRefExt, spaces_bar::SpacesBarRef }, i18n::{AppLanguage, tr_fmt, tr_key}, join_leave_room_modal::{ JoinLeaveModalKind, JoinLeaveRoomModalAction, JoinLeaveRoomModalWidgetRefExt }, login::login_screen::LoginAction, logout::logout_confirm_modal::{LogoutAction, LogoutConfirmModalAction, LogoutConfirmModalWidgetRefExt}, persistence, profile::user_profile_cache::clear_user_profile_cache, register::RegisterAction, room::BasicRoomDetails, shared::{confirmation_modal::{ConfirmationModalAction, ConfirmationModalContent, ConfirmationModalWidgetRefExt}, file_upload_modal::{FilePreviewerAction, FileUploadModalWidgetRefExt}, forward_modal::{ForwardMessageModalAction, ForwardMessageModalWidgetRefExt}, image_viewer::{ImageViewerAction, LoadState}, popup_list::{PopupKind, enqueue_popup_notification, enqueue_notification, NotificationItem, NotificationAction, NotifActionStyle}, room_filter_input_bar::FilterAction}, sliding_sync::{DirectMessageRoomAction, MatrixRequest, RemoteDirectorySearchKind, RemoteDirectorySearchResult, RoomSettingsFetchedAction, RoomAvatarUploadedAction, TimelineKind, AccountSwitchAction, current_user_id, get_client, submit_async_request, get_timeline_update_sender}, updater::{UpdateCheckOutcome, check_for_updates, load_skipped_update_version, save_skipped_update_version, update_release_page_url}, utils::RoomNameId, verification::VerificationAction, verification_modal::{ @@ -527,6 +527,10 @@ pub struct App { /// actions batch (i.e., every frame during a scroll), so it must be cheap /// in the common no-change case. `None` forces the next call to re-apply. #[rust] synced_app_language: Option, + /// Per-room registry of in-flight alias writes (P1-2). Lives here, outside + /// the singleton room-settings modal, so a modal close/reopen can't + /// re-enable the alias edit controls while a write is still settling. + #[rust] pending_alias_writes: PendingAliasWrites, } impl ScriptHook for App { @@ -1926,9 +1930,12 @@ impl MatchEvent for App { .unwrap_or_else(|| room_id.as_str().to_string()); let canonical_alias = rooms_list.get_room_canonical_alias(&room_id); let alias_str = canonical_alias.as_ref().map(|a| a.as_str()); + // P1-2: open locked (in the matching stage) if this room has + // an alias write still settling. + let alias_stage = self.pending_alias_writes.stage(&room_id); log!("RoomSettingsAction::Open for {} (name: {})", room_id, room_name); self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) - .show_settings(cx, room_id.clone(), &room_name, "", alias_str); + .show_settings(cx, room_id.clone(), &room_name, "", alias_str, alias_stage); self.ui.modal(cx, ids!(room_settings_modal)).open(cx); submit_async_request(MatrixRequest::FetchRoomSettings { room_id }); continue; @@ -1966,28 +1973,54 @@ impl MatchEvent for App { self.ui.modal(cx, ids!(room_settings_modal)).close(cx); continue; } - Some(RoomSettingsAction::AddLocalAddress { room_id, alias }) => { - // Resolve a bare localpart against the user's own homeserver. - let Some(server_name) = current_user_id().map(|u| u.server_name().to_owned()) else { - enqueue_popup_notification("You must be signed in to add an alias", PopupKind::Error, Some(4.0)); - continue; - }; - match crate::home::room_settings_modal::normalize_and_validate_alias(alias, &server_name) { - Ok(valid_alias) => { - submit_async_request(MatrixRequest::PublishRoomAlias { - room_id: room_id.clone(), - alias: valid_alias, - }); - enqueue_popup_notification("Publishing room alias…", PopupKind::Info, Some(3.0)); - } - Err(_) => { - enqueue_popup_notification( - "Invalid alias — use #name:server or a plain name", - PopupKind::Error, - Some(4.0), - ); - } - } + Some(RoomSettingsAction::PublishAlias { room_id, alias, canonical, alt_aliases }) => { + // The modal already validated the alias and optimistically + // advertised it. A single sequenced request registers it in + // the directory, then advertises it into `m.room.canonical_alias` + // only if that succeeds (see MatrixRequest::PublishRoomAlias). + self.pending_alias_writes.register(room_id.clone()); + submit_async_request(MatrixRequest::PublishRoomAlias { + room_id: room_id.clone(), + alias: alias.clone(), + canonical: canonical.clone(), + alt_aliases: alt_aliases.clone(), + }); + enqueue_popup_notification( + tr_key(self.app_state.app_language, "room_settings.aliases.publishing"), + PopupKind::Info, + Some(3.0), + ); + continue; + } + Some(RoomSettingsAction::SetCanonicalAlias { room_id, canonical, alt_aliases }) => { + self.pending_alias_writes.register(room_id.clone()); + submit_async_request(MatrixRequest::SetRoomCanonicalAlias { + room_id: room_id.clone(), + alias: canonical.clone(), + alt_aliases: alt_aliases.clone(), + }); + enqueue_popup_notification( + tr_key(self.app_state.app_language, "room_settings.aliases.updating_main"), + PopupKind::Info, + Some(3.0), + ); + continue; + } + Some(RoomSettingsAction::RemoveAlias { room_id, alias, canonical, alt_aliases }) => { + // Single sequenced request: unbind from the directory, then + // drop it from `m.room.canonical_alias` only if that succeeds. + self.pending_alias_writes.register(room_id.clone()); + submit_async_request(MatrixRequest::RemoveRoomAlias { + room_id: room_id.clone(), + alias: alias.clone(), + canonical: canonical.clone(), + alt_aliases: alt_aliases.clone(), + }); + enqueue_popup_notification( + tr_key(self.app_state.app_language, "room_settings.aliases.removing"), + PopupKind::Info, + Some(3.0), + ); continue; } Some(RoomSettingsAction::SetDirectoryPublish { .. }) => { @@ -2010,21 +2043,62 @@ impl MatchEvent for App { // Handle RoomSettingsFetchedAction. if let Some(fetched) = action.downcast_ref::() { - let canonical = fetched.canonical_alias.as_ref().map(|a| a.as_str().to_string()); - let alts: Vec = fetched.alt_aliases.iter().map(|a| a.as_str().to_string()).collect(); + // This authoritative fetch reconciles any in-flight alias write + // for the room — clear it from the pending registry (P1-2) so the + // controls re-enable on the next open. + self.pending_alias_writes.on_reconciled(&fetched.room_id); + // The modal is a singleton reused across rooms; it drops the + // response if `fetched.room_id` isn't the room it currently shows + // (P1-B: stale/out-of-order fetch guard). self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) - .apply_fetched_settings(cx, fetched.topic.clone(), fetched.is_public); + .apply_fetched_settings(cx, &fetched.room_id, fetched.topic.clone(), fetched.is_public); self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) .apply_alias_settings( cx, + &fetched.room_id, self.app_state.app_language, - canonical, - alts, + fetched.canonical_alias.clone(), + fetched.alt_aliases.clone(), fetched.can_manage_aliases, ); continue; } + // A reconcile fetch could not produce data (no client / room gone): + // clear the pending entry and release the modal's gate so the alias + // controls never strand disabled (they keep the optimistic state). + if let Some(unavailable) = action.downcast_ref::() { + self.pending_alias_writes.on_reconciled(&unavailable.room_id); + self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) + .release_alias_lock(cx, &unavailable.room_id); + continue; + } + + // Handle the server outcome of an alias write: surface errors as a + // toast (rollback of the optimistic UI is done inside the modal). + // For an *attempted* write, re-fetch authoritative room settings so + // the modal reconciles to server truth (this is the op's own refresh + // that releases its gate). A *preflight* failure (attempted == false: + // nothing sent) skips the fetch and clears its pending entry now. + // + // NOTE: the fetch reads `m.room.canonical_alias` only, not directory + // mappings — so a two-phase partial failure is repaired not by this + // fetch but by the worker's idempotent-retry semantics (a user retry + // treats "already mapped here" / "not found" as success; see + // PublishRoomAlias / RemoveRoomAlias handlers). + if let Some(result) = action.downcast_ref::() { + if let Some(message) = result.error.as_ref() { + enqueue_popup_notification(message.clone(), PopupKind::Error, Some(5.0)); + } + self.pending_alias_writes.on_result(&result.room_id, result.attempted); + if result.attempted { + submit_async_request(MatrixRequest::FetchRoomSettings { + room_id: result.room_id.clone(), + }); + } + continue; + } + // Handle RoomAvatarUploadedAction — refresh the avatar widget. if let Some(uploaded) = action.downcast_ref::() { self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) diff --git a/src/home/room_settings_modal.rs b/src/home/room_settings_modal.rs index 866072aa9..c1a1cb8f3 100644 --- a/src/home/room_settings_modal.rs +++ b/src/home/room_settings_modal.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use makepad_widgets::*; -use ruma::{OwnedRoomAliasId, OwnedRoomId, RoomAliasId, ServerName}; +use ruma::{OwnedRoomAliasId, OwnedRoomId, RoomAliasId, RoomId, ServerName}; use crate::i18n::{AppLanguage, tr_key}; use crate::shared::avatar::AvatarWidgetExt; @@ -144,6 +144,229 @@ pub fn reconcile_canonical_alias( } } +/// Compute the `alt_aliases` list to advertise after publishing `new_alias`. +/// +/// Used by the optimistic "publish → auto-advertise" flow: the freshly +/// published alias is appended to the room's existing alt aliases so it shows +/// up as advertised immediately. The result preserves the `m.room.canonical_alias` +/// invariants: +/// +/// - The canonical alias is never duplicated into `alt_aliases`. +/// - An alias already present (canonical or alt) is not added twice. +/// +/// The canonical alias itself is passed only so it can be excluded; it is never +/// added or removed here (that stays with [`reconcile_canonical_alias`]). +pub fn advertise_alias_into_alts( + current_canonical: Option<&RoomAliasId>, + current_alts: &[OwnedRoomAliasId], + new_alias: &RoomAliasId, +) -> Vec { + let new_str = new_alias.as_str(); + let is_canonical = current_canonical.is_some_and(|c| c.as_str() == new_str); + let already_alt = current_alts.iter().any(|a| a.as_str() == new_str); + let mut alts = current_alts.to_vec(); + if !is_canonical && !already_alt { + alts.push(new_alias.to_owned()); + } + alts +} + +/// The next step of a sequenced alias operation after its directory write +/// (`create_alias` / `delete_alias`) returns. +/// +/// A publish/remove is two server writes — the room-directory write and the +/// `m.room.canonical_alias` write — that must run in sequence, not in parallel: +/// the canonical write happens only if the directory write succeeded. This +/// prevents a partial failure from leaving inconsistent server state (e.g. an +/// alias advertised in `alt_aliases` but never registered in the directory). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SequencedAliasStep { + /// Directory write succeeded — proceed to the `m.room.canonical_alias` write. + WriteCanonical, + /// Directory write failed — abort; leave `m.room.canonical_alias` untouched. + Abort, +} + +/// Decide the next step after the directory write of a sequenced publish/remove. +pub fn next_step_after_directory_write(directory_ok: bool) -> SequencedAliasStep { + if directory_ok { + SequencedAliasStep::WriteCanonical + } else { + SequencedAliasStep::Abort + } +} + +/// Serializes alias mutations for the modal's room: at most one may be in +/// flight at a time. Overlapping mutations are the cross-operation race that +/// can resurrect an unbound alias — each write snapshots the *full* +/// canonical/alt state, so a late-completing write clobbers a newer one. This +/// gate keeps the edit controls disabled from submit until the operation fully +/// settles, so the next mutation always builds on reconciled state. +/// +/// A successful write holds the gate until the authoritative refresh confirms +/// server state; a *failed* write releases immediately, since the modal has +/// already rolled back to the pre-edit snapshot (and the refresh may never +/// arrive, e.g. the client was torn down). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum AliasWriteGate { + /// No mutation in flight — edit controls are live and submits are allowed. + #[default] + Idle, + /// A mutation was submitted; awaiting its server write result. + AwaitingResult, + /// The write succeeded; awaiting the authoritative `FetchRoomSettings` refresh. + AwaitingRefresh, +} + +impl AliasWriteGate { + /// Whether a new mutation may be submitted (only when fully idle). + pub fn can_submit(self) -> bool { + matches!(self, AliasWriteGate::Idle) + } + + /// Record a submitted mutation. Returns `false` (and does nothing) if a + /// mutation is already in flight, so callers can reject the overlap. + pub fn on_submit(&mut self) -> bool { + if self.can_submit() { + *self = AliasWriteGate::AwaitingResult; + true + } else { + false + } + } + + /// Record the write result, keyed on whether the server was actually + /// *attempted*: + /// - `attempted == true` (a request was sent — success OR server-side + /// failure): hold the gate until this op's own reconciliation fetch lands, + /// because a `FetchRoomSettings` is in flight for it. Releasing now would + /// let a new op start while that fetch is outstanding — the stray-refresh + /// race codex flagged. + /// - `attempted == false` (preflight failure, e.g. no client — nothing was + /// sent, no fetch spawned, state unchanged): release straight to `Idle`. + /// + /// Ignores stray results (not in `AwaitingResult`). + pub fn on_result(&mut self, attempted: bool) { + if matches!(self, AliasWriteGate::AwaitingResult) { + *self = if attempted { + AliasWriteGate::AwaitingRefresh + } else { + AliasWriteGate::Idle + }; + } + } + + /// Whether an incoming authoritative refresh should be applied to the modal. + /// A refresh arriving while `AwaitingResult` is a stray fetch (this op's own + /// fetch is only spawned after its result) — applying it would clobber the + /// optimistic state mid-flight, so it is rejected. `Idle` accepts the + /// open/initial fetch; `AwaitingRefresh` accepts this op's own reconcile. + pub fn should_accept_refresh(self) -> bool { + !matches!(self, AliasWriteGate::AwaitingResult) + } + + /// The authoritative refresh landed — release only from `AwaitingRefresh` + /// (this op's own reconciliation). Callers must gate the state overwrite on + /// [`Self::should_accept_refresh`] and call this to advance the enum. Room + /// switches reset to `Idle` directly in `show`, not via this method. + pub fn on_refresh(&mut self) { + if matches!(self, AliasWriteGate::AwaitingRefresh) { + *self = AliasWriteGate::Idle; + } + } +} + +/// Per-room stage of an in-flight alias write, tracked by [`PendingAliasWrites`]. +/// Absence from the map means idle. Mirrors [`AliasWriteGate`] but at app level. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PendingAliasStage { + /// Submitted; the server write result has not returned yet. A settings fetch + /// arriving now is an unrelated open-fetch and must NOT clear the entry (the + /// write outcome is still unknown). + Submitted, + /// The write completed server-side (result returned) and a reconcile fetch is + /// expected. Any settings fetch now reflects that completed write, so it + /// clears the entry. + AwaitingReconcile, +} + +/// App-level (per-room) registry of in-flight alias writes, kept *outside* the +/// singleton modal so a modal close/switch/reopen cannot silently re-enable +/// the edit controls while a write is still settling (P1-2). A room is +/// "pending" from the moment a mutation is submitted until that op reconciles. +/// +/// Ownership: `app.rs` holds the single instance; `show` consults it to decide +/// whether to open a room locked. Transitions survive the modal being torn down +/// and reopened. +#[derive(Debug, Default, Clone)] +pub struct PendingAliasWrites { + rooms: std::collections::HashMap, +} + +impl PendingAliasWrites { + /// A mutation was submitted for `room_id` — mark it pending (`Submitted`). + pub fn register(&mut self, room_id: OwnedRoomId) { + self.rooms.insert(room_id, PendingAliasStage::Submitted); + } + + /// The write result arrived. `attempted == false` (preflight failure) is + /// terminal — clear immediately (no reconcile fetch will follow). + /// `attempted == true` advances to `AwaitingReconcile`: the write completed + /// server-side and a reconcile fetch is expected. + pub fn on_result(&mut self, room_id: &RoomId, attempted: bool) { + if attempted { + if self.rooms.contains_key(room_id) { + self.rooms.insert(room_id.to_owned(), PendingAliasStage::AwaitingReconcile); + } + } else { + self.rooms.remove(room_id); + } + } + + /// A settings fetch for `room_id` landed. It clears the entry only from + /// `AwaitingReconcile` (the write already completed server-side, so the fetch + /// reflects it). While still `Submitted`, an open-fetch from a reopen must + /// not clear the pending write — its outcome is not yet known. + pub fn on_reconciled(&mut self, room_id: &RoomId) { + if self.rooms.get(room_id) == Some(&PendingAliasStage::AwaitingReconcile) { + self.rooms.remove(room_id); + } + } + + /// Whether `room_id` has an alias write still settling (open it locked). + pub fn is_pending(&self, room_id: &RoomId) -> bool { + self.rooms.contains_key(room_id) + } + + /// The in-flight stage for `room_id`, if any — so `show` can open the modal + /// in the matching locked state (reject open-fetches while `Submitted`, + /// accept the reconcile while `AwaitingReconcile`). + pub fn stage(&self, room_id: &RoomId) -> Option { + self.rooms.get(room_id).copied() + } +} + +/// Decide whether a failed `create_alias` (directory publish) should be treated +/// as an idempotent success: only when the server rejected it as a conflict AND +/// the alias already resolves to *this* room. This makes a retry after a +/// step-2 (advertise) failure repair the divergence instead of dying on +/// "alias already in use" (`FetchRoomSettings` can't see directory mappings, so +/// the two-phase write is not otherwise self-healing). +pub fn publish_alias_treat_as_success( + directory_conflict: bool, + existing_maps_to_this_room: bool, +) -> bool { + directory_conflict && existing_maps_to_this_room +} + +/// Decide whether a failed `delete_alias` (directory unbind) should be treated +/// as an idempotent success: a "not found" means the mapping is already gone, +/// so a retry after a partial Remove can proceed to de-advertise instead of +/// dying on "alias not found". +pub fn remove_alias_treat_as_success(directory_not_found: bool) -> bool { + directory_not_found +} + #[cfg(test)] mod alias_logic_tests { use super::*; @@ -224,12 +447,375 @@ mod alias_logic_tests { .unwrap(); assert!(!out.alt_aliases.contains(&alias("#dup:example.org"))); } + + #[test] + fn test_advertise_alias_appends_new_alt() { + let alts = advertise_alias_into_alts( + Some(&alias("#main:example.org")), + &[alias("#one:example.org")], + &alias("#two:example.org"), + ); + assert_eq!( + alts, + vec![alias("#one:example.org"), alias("#two:example.org")], + ); + } + + #[test] + fn test_advertise_alias_never_duplicates_canonical() { + // Advertising the canonical alias must not push it into alt_aliases. + let alts = advertise_alias_into_alts( + Some(&alias("#main:example.org")), + &[alias("#one:example.org")], + &alias("#main:example.org"), + ); + assert!(!alts.contains(&alias("#main:example.org"))); + assert_eq!(alts, vec![alias("#one:example.org")]); + } + + #[test] + fn test_advertise_alias_is_idempotent_for_existing_alt() { + // Re-advertising an already-published alt does not create a duplicate. + let alts = advertise_alias_into_alts( + None, + &[alias("#one:example.org")], + &alias("#one:example.org"), + ); + assert_eq!(alts, vec![alias("#one:example.org")]); + } + + #[test] + fn test_sequenced_op_writes_canonical_only_after_directory_success() { + assert_eq!( + next_step_after_directory_write(true), + SequencedAliasStep::WriteCanonical, + ); + } + + #[test] + fn test_sequenced_op_aborts_when_directory_write_fails() { + // A failed directory write must NOT trigger the canonical_alias write — + // this is the invariant that stops partial/parallel-write divergence. + assert_eq!( + next_step_after_directory_write(false), + SequencedAliasStep::Abort, + ); + } + + #[test] + fn test_alias_gate_blocks_overlapping_mutation() { + let mut gate = AliasWriteGate::default(); + assert!(gate.can_submit()); + assert!(gate.on_submit()); // first submit accepted + assert!(!gate.can_submit()); // controls now gated + assert!(!gate.on_submit()); // overlapping submit rejected + assert_eq!(gate, AliasWriteGate::AwaitingResult); + } + + #[test] + fn test_alias_gate_attempted_result_holds_until_refresh() { + // Both success AND server-attempted failure hold the gate until this + // op's own reconcile fetch lands (a fetch is in flight for it). + for attempted in [true, true] { + let mut gate = AliasWriteGate::default(); + gate.on_submit(); + gate.on_result(attempted); // attempted → awaiting its own refresh + assert_eq!(gate, AliasWriteGate::AwaitingRefresh); + assert!(!gate.can_submit()); + gate.on_refresh(); // this op's authoritative refresh releases it + assert!(gate.can_submit()); + } + } + + #[test] + fn test_alias_gate_preflight_failure_releases_immediately() { + // A preflight failure (attempted == false: nothing sent, no fetch + // spawned, state unchanged) releases straight to Idle. + let mut gate = AliasWriteGate::default(); + gate.on_submit(); + gate.on_result(false); + assert!(gate.can_submit()); + assert_eq!(gate, AliasWriteGate::Idle); + } + + #[test] + fn test_alias_gate_ignores_stray_result() { + let mut gate = AliasWriteGate::default(); + gate.on_result(true); // no submit in flight → ignored + assert_eq!(gate, AliasWriteGate::Idle); + } + + #[test] + fn test_alias_gate_stray_refresh_while_awaiting_result_does_not_release() { + // The op's own fetch is only spawned after its result, so a refresh + // arriving during AwaitingResult is a stray from a prior op: it must + // neither release the gate nor be accepted (would clobber optimism). + let mut gate = AliasWriteGate::default(); + gate.on_submit(); + assert_eq!(gate, AliasWriteGate::AwaitingResult); + assert!(!gate.should_accept_refresh()); // reject stray + gate.on_refresh(); + assert_eq!(gate, AliasWriteGate::AwaitingResult); + assert!(!gate.can_submit()); + } + + #[test] + fn test_alias_gate_accepts_refresh_when_idle_or_awaiting_refresh() { + assert!(AliasWriteGate::Idle.should_accept_refresh()); // open/initial fetch + assert!(AliasWriteGate::AwaitingRefresh.should_accept_refresh()); // own reconcile + assert!(!AliasWriteGate::AwaitingResult.should_accept_refresh()); // stray + } + + // ── PendingAliasWrites (app-level per-room registry, survives reopen) ── + + fn room(s: &str) -> OwnedRoomId { + OwnedRoomId::try_from(s).expect("valid room id in test") + } + + #[test] + fn test_pending_registry_survives_reopen() { + // Submitting marks the room pending; that state must persist across any + // number of reads (a modal close/reopen just re-reads is_pending). + let mut reg = PendingAliasWrites::default(); + let r = room("!a:example.org"); + reg.register(r.clone()); + assert!(reg.is_pending(&r)); + assert!(reg.is_pending(&r)); // reopen consults it again → still pending + } + + #[test] + fn test_pending_registry_cleared_on_reconcile() { + let mut reg = PendingAliasWrites::default(); + let r = room("!a:example.org"); + reg.register(r.clone()); + reg.on_result(&r, true); // attempted → still pending, awaiting reconcile + assert!(reg.is_pending(&r)); + reg.on_reconciled(&r); + assert!(!reg.is_pending(&r)); + } + + #[test] + fn test_pending_registry_open_fetch_does_not_clear_submitted() { + // A settings fetch (e.g. an open-fetch from a reopen) that lands BEFORE + // the write result must not clear a still-Submitted write — the outcome + // is unknown, so the room must stay locked. + let mut reg = PendingAliasWrites::default(); + let r = room("!a:example.org"); + reg.register(r.clone()); // Submitted + reg.on_reconciled(&r); // stray/open fetch while Submitted + assert!(reg.is_pending(&r)); // still pending + reg.on_result(&r, true); // now AwaitingReconcile + reg.on_reconciled(&r); // the real reconcile clears it + assert!(!reg.is_pending(&r)); + } + + #[test] + fn test_pending_registry_preflight_failure_clears_immediately() { + let mut reg = PendingAliasWrites::default(); + let r = room("!a:example.org"); + reg.register(r.clone()); + reg.on_result(&r, false); // preflight failure is terminal + assert!(!reg.is_pending(&r)); + } + + #[test] + fn test_pending_registry_is_per_room() { + let mut reg = PendingAliasWrites::default(); + let a = room("!a:example.org"); + let b = room("!b:example.org"); + reg.register(a.clone()); + assert!(reg.is_pending(&a)); + assert!(!reg.is_pending(&b)); // other rooms unaffected + } + + // ── idempotent two-phase repair decisions ── + + #[test] + fn test_publish_treat_as_success_only_when_conflict_maps_here() { + assert!(publish_alias_treat_as_success(true, true)); // conflict + maps here → repair + assert!(!publish_alias_treat_as_success(true, false)); // conflict, maps elsewhere → real fail + assert!(!publish_alias_treat_as_success(false, true)); // not a conflict → real fail + assert!(!publish_alias_treat_as_success(false, false)); + } + + #[test] + fn test_remove_treat_as_success_on_not_found() { + assert!(remove_alias_treat_as_success(true)); // already gone → success (de-advertise) + assert!(!remove_alias_treat_as_success(false)); // other error → real fail + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// AliasRow — one published-alias row in the "Room Aliases" section. +// +// Each row shows the alias string plus, for users with manage permission, a +// "Set as main" button (hidden on the current canonical) and a "Remove" button. +// The row is a self-contained widget: it stores its own alias and emits an +// [`AliasRowAction`] carrying that alias, so the parent modal routes per-row +// clicks without tracking slot indices (mirrors `DeviceCard` in +// `settings/devices_settings.rs`). Its DSL lives in the shared `script_mod!` +// block below, alongside `RoomSettingsModal`. +// ───────────────────────────────────────────────────────────────────────────── + +/// Per-row action emitted by an [`AliasRow`], carrying the row's alias value so +/// the parent modal can act without knowing which slot fired. +#[derive(Clone, Debug, Default)] +pub enum AliasRowAction { + /// "Set as main" clicked — promote this alias to canonical. + SetCanonical(OwnedRoomAliasId), + /// "Remove" clicked — unpublish this alias / drop it from canonical+alts. + Remove(OwnedRoomAliasId), + #[default] + None, +} + +/// The data for one alias row, handed to an [`AliasRow`] PortalList item via +/// its draw scope's props. Carries everything the row needs to render and to +/// route its clicks. +#[derive(Clone, Debug)] +pub struct AliasRowProps { + pub alias: OwnedRoomAliasId, + /// Whether this is the room's canonical (main) alias. + pub is_canonical: bool, + /// Whether the edit controls (Set-as-main / Remove) are interactive. + pub edit_enabled: bool, + pub language: AppLanguage, +} + +#[derive(Script, ScriptHook, Widget)] +pub struct AliasRow { + #[deref] view: View, + /// The alias this row currently represents (mirrored from props at draw). + #[rust] alias: Option, + /// Whether this row is the room's canonical (main) alias. + #[rust] is_canonical: bool, + /// Whether this row's edit controls are interactive (mirrored from props). + #[rust] edit_enabled: bool, +} + +impl Widget for AliasRow { + fn handle_event(&mut self, cx: &mut Cx, event: &Event, scope: &mut Scope) { + self.view.handle_event(cx, event, scope); + if let Event::Actions(actions) = event { + // Ignore clicks unless this row's controls are interactive (they are + // hidden otherwise; the guard is belt-and-suspenders against a queued + // click while a write is in flight or permission is absent). + if !self.edit_enabled { + return; + } + // "Set as main" is a no-op on the current canonical (its button is + // hidden anyway; the guard keeps it correct if that ever changes). + if self.view.button(cx, ids!(alias_row_set_main_button)).clicked(actions) + && !self.is_canonical + { + if let Some(alias) = self.alias.clone() { + cx.action(AliasRowAction::SetCanonical(alias)); + } + } + if self.view.button(cx, ids!(alias_row_remove_button)).clicked(actions) { + if let Some(alias) = self.alias.clone() { + cx.action(AliasRowAction::Remove(alias)); + } + } + } + } + + fn draw_walk(&mut self, cx: &mut Cx2d, scope: &mut Scope, walk: Walk) -> DrawStep { + // Populate from the item scope's props (set by the parent modal's draw + // loop). Values are mirrored into `#[rust]` fields for `handle_event`. + if let Some(props) = scope.props.get::() { + self.alias = Some(props.alias.clone()); + self.is_canonical = props.is_canonical; + self.edit_enabled = props.edit_enabled; + + self.view.label(cx, ids!(alias_row_label)).set_text(cx, props.alias.as_str()); + // Canonical rows get a "Main" badge (reuses the localized label). + self.view.view(cx, ids!(alias_row_main_badge)).set_visible(cx, props.is_canonical); + if props.is_canonical { + self.view.label(cx, ids!(alias_row_main_badge_label)) + .set_text(cx, tr_key(props.language, "room_settings.aliases.canonical_label")); + } + self.view.button(cx, ids!(alias_row_set_main_button)) + .set_text(cx, tr_key(props.language, "room_settings.aliases.set_canonical_button")); + self.view.button(cx, ids!(alias_row_remove_button)) + .set_text(cx, tr_key(props.language, "room_settings.aliases.remove_button")); + // "Set as main" only when interactive and not already canonical. + self.view.button(cx, ids!(alias_row_set_main_button)) + .set_visible(cx, props.edit_enabled && !props.is_canonical); + self.view.button(cx, ids!(alias_row_remove_button)) + .set_visible(cx, props.edit_enabled); + } + self.view.draw_walk(cx, scope, walk) + } } script_mod! { use mod.prelude.widgets.* use mod.widgets.* + // One published-alias row (see the AliasRow Rust widget above). + // PortalList item: a fixed-height row so the modal can size the list to fit + // its content deterministically (see ALIAS_ROW_PX). Data comes from + // `AliasRowProps` via the item scope at draw time. + mod.widgets.AliasRow = #(AliasRow::register_widget(vm)) { + width: Fill + height: 40 + flow: Right + align: Align{y: 0.5} + margin: Inset{bottom: 6} + spacing: 8 + + alias_row_label := Label { + width: Fill + height: Fit + draw_text +: { + text_style: REGULAR_TEXT {font_size: 10.5} + color: (RBX_FG_PRIMARY) + } + text: "" + } + + alias_row_main_badge := RoundedView { + visible: false + width: Fit + height: Fit + align: Align{y: 0.5} + padding: Inset{left: 8, right: 8, top: 2, bottom: 2} + show_bg: true + draw_bg +: { + color: (RBX_ACCENT_SOFT) + border_radius: (RBX_RADIUS_PILL) + } + alias_row_main_badge_label := Label { + width: Fit + height: Fit + draw_text +: { + text_style: RBX_TEXT_BADGE {} + color: (RBX_ACCENT) + } + text: "" + } + } + + alias_row_set_main_button := RobrixNeutralIconButton { + width: Fit + height: (RBX_CONTROL_H_SM) + padding: Inset{top: 6, bottom: 6, left: 10, right: 10} + icon_walk: Walk{width: 0, height: 0} + draw_bg +: { border_radius: (RBX_RADIUS_XS) } + text: "Set as main" + } + + alias_row_remove_button := RobrixNegativeIconButton { + width: Fit + height: (RBX_CONTROL_H_SM) + padding: Inset{top: 6, bottom: 6, left: 10, right: 10} + icon_walk: Walk{width: 0, height: 0} + draw_bg +: { border_radius: (RBX_RADIUS_XS) } + text: "Remove" + } + } + mod.widgets.RoomSettingsModal = #(RoomSettingsModal::register_widget(vm)) { width: Fill { max: 680 } height: Fit @@ -561,6 +1147,24 @@ script_mod! { } } + // ── Alias rows (canonical + alts) ──────────────── + // A PortalList so EVERY alias gets a real, actionable row + // (Remove / Set-as-main) with no fixed cap — the modal drives + // it from `alias_entries` and sizes its height to fit the + // content (see `render_alias_section`), so short lists don't + // scroll and long ones (rare) scroll internally. + alias_list := PortalList { + width: Fill + height: 0 + flow: Down + grab_key_focus: false + max_pull_down: 0.0 + auto_tail: false + keep_invisible: false + + alias_item := mod.widgets.AliasRow {} + } + publish_toggle_row := View { width: Fill height: Fit @@ -606,7 +1210,11 @@ script_mod! { text: "No other published addresses yet, add one below" } + // Hidden by default (P1-A): the add control only appears once + // the room's power-level fetch confirms manage permission, + // via `render_alias_section`. Never visible before that. add_address_row := View { + visible: false width: Fill height: Fit flow: Right @@ -802,8 +1410,30 @@ pub enum RoomSettingsAction { Cancel, /// Toggle publishing this room to the directory. SetDirectoryPublish { room_id: OwnedRoomId, enabled: bool }, - /// Add a local address alias. - AddLocalAddress { room_id: OwnedRoomId, alias: String }, + /// Publish a new (already-validated) alias and advertise it into + /// `m.room.canonical_alias`'s `alt_aliases`. `canonical`/`alt_aliases` are + /// the reconciled state to write (alt_aliases already includes `alias`). + PublishAlias { + room_id: OwnedRoomId, + alias: OwnedRoomAliasId, + canonical: Option, + alt_aliases: Vec, + }, + /// Promote an existing alias to canonical. `canonical`/`alt_aliases` are the + /// reconciled target of the `m.room.canonical_alias` state event. + SetCanonicalAlias { + room_id: OwnedRoomId, + canonical: Option, + alt_aliases: Vec, + }, + /// Remove an alias: unbind it from the room directory and drop it from + /// `m.room.canonical_alias` (reconciled `canonical`/`alt_aliases`). + RemoveAlias { + room_id: OwnedRoomId, + alias: OwnedRoomAliasId, + canonical: Option, + alt_aliases: Vec, + }, /// Change media visibility preference. SetMediaVisibility { room_id: OwnedRoomId, always_show: bool }, /// Leave the room. @@ -814,6 +1444,14 @@ pub enum RoomSettingsAction { None, } +/// Per-row height (px) used to size the alias `PortalList` to fit its content. +/// Slightly over the DSL row advance (height 40 + margin 6 = 46) so a fitted +/// list has a hair of slack rather than clipping the last row into a scroll. +const ALIAS_ROW_PX: f64 = 48.0; +/// Above this many aliases the list stops growing and scrolls internally (every +/// row stays a real, actionable AliasRow — nothing is stranded). +const ALIAS_LIST_MAX_ROWS: usize = 10; + #[derive(Script, ScriptHook, Widget)] pub struct RoomSettingsModal { #[deref] view: View, @@ -822,6 +1460,23 @@ pub struct RoomSettingsModal { #[rust] original_name: String, #[rust] original_topic: String, #[rust] always_show_media: bool, + /// Language used to (re-)render the alias section after optimistic updates. + #[rust] language: AppLanguage, + /// Current canonical alias (authoritative, plus optimistic edits). + #[rust] current_canonical: Option, + /// Current alt aliases (authoritative, plus optimistic edits). + #[rust] current_alts: Vec, + /// Whether the user may manage aliases (gates the per-row edit controls). + #[rust] can_manage_aliases: bool, + /// Snapshot of `(canonical, alts)` taken before an in-flight optimistic + /// write, restored if the server reports failure. + #[rust] alias_snapshot: Option<(Option, Vec)>, + /// Serializes alias mutations: at most one write in flight per room. Gates + /// the edit controls from submit until the operation fully settles. + #[rust] alias_gate: AliasWriteGate, + /// The alias rows to render, in display order (canonical first). Drives the + /// alias `PortalList` in `draw_walk`; rebuilt by `render_alias_section`. + #[rust] alias_entries: Vec, } impl Widget for RoomSettingsModal { @@ -831,7 +1486,26 @@ impl Widget for RoomSettingsModal { } fn draw_walk(&mut self, cx: &mut Cx2d, scope: &mut Scope, walk: Walk) -> DrawStep { - self.view.draw_walk(cx, scope, walk) + // Drive the alias PortalList: every entry gets a real actionable row, so + // no alias is stranded regardless of count (mirrors the DevicesScreen + // PortalList pattern). + while let Some(widget) = self.view.draw_walk(cx, scope, walk).step() { + let plist = widget.as_portal_list(); + let Some(mut list) = plist.borrow_mut() else { + continue; + }; + let n = self.alias_entries.len(); + list.set_item_range(cx, 0, n); + while let Some(index) = list.next_visible_item(cx) { + if index < n { + let props = self.alias_entries[index].clone(); + let item = list.item(cx, index, id!(alias_item)); + let mut item_scope = Scope::with_props(&props); + item.draw_all(cx, &mut item_scope); + } + } + } + DrawStep::done() } } @@ -879,16 +1553,36 @@ impl WidgetMatchEvent for RoomSettingsModal { } } - // Add address button + // Add address button — validate, optimistically advertise, then publish. if self.view.button(cx, ids!(add_address_button)).clicked(actions) { - let alias = self.view.text_input(cx, ids!(add_address_input)).text(); - // Pass the raw (trimmed) text through; validation/normalization happens - // in the AddLocalAddress handler via `normalize_and_validate_alias`. - let alias = alias.trim().to_string(); - if !alias.is_empty() { + let raw = self.view.text_input(cx, ids!(add_address_input)).text(); + let raw = raw.trim().to_string(); + if !raw.is_empty() { if let Some(room_id) = self.room_id.clone() { - cx.action(RoomSettingsAction::AddLocalAddress { room_id, alias }); - self.view.text_input(cx, ids!(add_address_input)).set_text(cx, ""); + self.add_alias(cx, room_id, &raw); + } + } + } + + // Per-row actions from AliasRow widgets (Set as main / Remove). + for action in actions { + match action.downcast_ref::() { + Some(AliasRowAction::SetCanonical(alias)) => { + self.set_canonical_alias(cx, alias.clone()); + } + Some(AliasRowAction::Remove(alias)) => { + self.remove_alias(cx, alias.clone()); + } + _ => {} + } + } + + // Server outcome of an alias write: commit on success, roll back the + // optimistic UI and surface the server error on failure. + for action in actions { + if let Some(result) = action.downcast_ref::() { + if self.room_id.as_deref() == Some(result.room_id.as_ref()) { + self.apply_write_result(cx, result); } } } @@ -936,7 +1630,11 @@ impl WidgetMatchEvent for RoomSettingsModal { } impl RoomSettingsModal { - /// Populate the modal with room data and prepare for display. + /// Populate the modal with room data and prepare for display. `alias_stage` + /// comes from the app-level [`PendingAliasWrites`] registry: `Some(_)` means + /// this room has an alias write still settling (submitted in a prior modal + /// session), so the section opens locked in the matching gate state until + /// that op's reconcile fetch lands. pub fn show( &mut self, cx: &mut Cx, @@ -944,6 +1642,7 @@ impl RoomSettingsModal { room_name: &str, room_topic: &str, canonical_alias: Option<&str>, + alias_stage: Option, ) { let room_id_text = room_id.as_str().to_string(); self.room_id = Some(room_id); @@ -965,12 +1664,28 @@ impl RoomSettingsModal { self.view.text_input(cx, ids!(room_id_input)) .set_is_read_only(cx, true); - // Canonical alias - let alias_text = canonical_alias - .map(|a| a.to_string()) - .unwrap_or_else(|| String::from("No main address set")); - self.view.label(cx, ids!(main_alias_label)) - .set_text(cx, &alias_text); + // P1-A: reset the alias section to a read-only "loading" state for the + // NEW room. Without this, the singleton modal keeps the previous room's + // aliases, permission, snapshot, and rendered rows — so a click landing + // before this room's fetch returns would mutate the wrong room with a + // stale alias. Edit controls stay disabled (can_manage=false) until the + // matching `FetchRoomSettings` refresh arrives via `apply_alias_settings`. + self.current_canonical = canonical_alias + .and_then(|s| OwnedRoomAliasId::try_from(s).ok()); + self.current_alts = Vec::new(); + self.can_manage_aliases = false; + self.alias_snapshot = None; + // P1-2: if a write for this room is still settling (registry), open locked + // in the matching gate state so a close→reopen can't re-enable controls + // mid-flight. `Submitted` → AwaitingResult (reject an unrelated open-fetch + // until the write result returns); `AwaitingReconcile` → AwaitingRefresh + // (the op's reconcile fetch unlocks it). No pending write → Idle. + self.alias_gate = match alias_stage { + Some(PendingAliasStage::Submitted) => AliasWriteGate::AwaitingResult, + Some(PendingAliasStage::AwaitingReconcile) => AliasWriteGate::AwaitingRefresh, + None => AliasWriteGate::Idle, + }; + self.render_alias_section(cx); // Avatar fallback text (first char of name) let avatar_char = room_name.chars().next().unwrap_or('?').to_string(); @@ -991,13 +1706,25 @@ impl RoomSettingsModal { self.view.redraw(cx); } + /// Whether `room_id` matches the room this modal is currently showing. + /// Used to drop stale/out-of-order async responses for a previous room + /// (P1-B), so they never overwrite the current room's modal. + fn is_current_room(&self, room_id: &RoomId) -> bool { + self.room_id.as_deref() == Some(room_id) + } + /// Apply fetched settings (topic, is_public) that arrived asynchronously. + /// Ignored if `room_id` is not the room currently shown (stale response). pub fn apply_fetched_settings( &mut self, cx: &mut Cx, + room_id: &RoomId, topic: Option, is_public: bool, ) { + if !self.is_current_room(room_id) { + return; + } if let Some(t) = topic { self.original_topic = t.clone(); self.view.text_input(cx, ids!(room_topic_input)).set_text(cx, &t); @@ -1012,36 +1739,106 @@ impl RoomSettingsModal { /// gating to the "Room Aliases" section. Labels use the localized strings /// from `resources/i18n/**` so the section follows the app language. /// - /// When `can_manage` is false the user lacks the power level to send the - /// `m.room.canonical_alias` state event, so the add-address control is - /// hidden and a read-only hint is shown instead. + /// This is the authoritative refresh: it overwrites any optimistic state + /// and clears the rollback snapshot. When `can_manage` is false the user + /// lacks the power level to send the `m.room.canonical_alias` state event, + /// so the add-address control is hidden and a read-only hint is shown. + /// + /// Ignored if `room_id` is not the room currently shown (P1-B): out-of-order + /// fetches for a previous room must never overwrite the current modal. pub fn apply_alias_settings( &mut self, cx: &mut Cx, + room_id: &RoomId, language: AppLanguage, - canonical_alias: Option, - alt_aliases: Vec, + canonical_alias: Option, + alt_aliases: Vec, can_manage: bool, ) { + if !self.is_current_room(room_id) { + return; + } + // P1-1: route the gate decision BEFORE touching state. A refresh arriving + // while a mutation's result is still pending (AwaitingResult) is a stray + // fetch from a prior op — reject it so it can't clobber optimistic state + // or the rollback snapshot. Otherwise accept and advance the gate. + if !self.alias_gate.should_accept_refresh() { + return; + } + self.alias_gate.on_refresh(); + // Store authoritative state; a fresh fetch supersedes optimism. + self.language = language; + self.current_canonical = canonical_alias; + self.current_alts = alt_aliases; + self.can_manage_aliases = can_manage; + self.alias_snapshot = None; + self.render_alias_section(cx); + } + + /// Release a waiting alias write's gate when its reconcile fetch could not + /// produce data (no client / room unavailable). Unlike `apply_alias_settings` + /// this does NOT overwrite state — it keeps the current (optimistic) aliases + /// and just re-enables the controls, so the gate can never strand disabled. + /// Guarded like a refresh: a stray release during `AwaitingResult` is ignored. + pub fn release_alias_lock(&mut self, cx: &mut Cx, room_id: &RoomId) { + if !self.is_current_room(room_id) || !self.alias_gate.should_accept_refresh() { + return; + } + self.alias_gate.on_refresh(); + self.render_alias_section(cx); + } + + /// Render the whole alias section (labels, per-row list, gating) from the + /// modal's current stored state. Called on authoritative refresh and after + /// every optimistic edit. + fn render_alias_section(&mut self, cx: &mut Cx) { + let language = self.language; + let can_manage = self.can_manage_aliases; + // Edit controls are interactive only when the user has permission AND no + // alias write is in flight (P1-C: one mutation per room). `can_manage` + // still drives the read-only-vs-editable hint text, so a manager who is + // mid-write doesn't briefly see the "no permission" message. + let edit_enabled = can_manage && self.alias_gate.can_submit(); + // Localized section labels. self.view.label(cx, ids!(addresses_heading)) .set_text(cx, tr_key(language, "room_settings.aliases.section_title")); self.view.label(cx, ids!(published_addresses_label)) .set_text(cx, tr_key(language, "room_settings.aliases.canonical_label")); - // Canonical (main) alias, or the existing "no main address" fallback. - let main_text = canonical_alias - .unwrap_or_else(|| String::from("No main address set")); - self.view.label(cx, ids!(main_alias_label)).set_text(cx, &main_text); - - // Alternate published aliases, one per line. Falls back to a hint when - // there are none. - if alt_aliases.is_empty() { - self.view.label(cx, ids!(no_published_label)) - .set_text(cx, "No other published addresses yet, add one below"); - } else { + // The canonical alias is now shown as a badged, actionable row in the + // list below, so hide the old separate summary line to avoid showing it + // twice (review finding P2-cosmetic). + self.view.view(cx, ids!(main_alias_row)).set_visible(cx, false); + + // Build the ordered row list for the PortalList: canonical first + // (flagged), then alts. Every entry becomes a real, actionable row — no + // cap, so nothing is stranded (P2). + let mut entries: Vec = Vec::new(); + if let Some(c) = self.current_canonical.clone() { + entries.push(AliasRowProps { alias: c, is_canonical: true, edit_enabled, language }); + } + for a in self.current_alts.clone() { + entries.push(AliasRowProps { alias: a, is_canonical: false, edit_enabled, language }); + } + let row_count = entries.len(); + self.alias_entries = entries; + + // Size the list to fit its content (up to a cap, beyond which it scrolls + // internally). A fitted list never scroll-captures inside the modal. + let visible_rows = row_count.min(ALIAS_LIST_MAX_ROWS); + let list_height = visible_rows as f64 * ALIAS_ROW_PX; + let mut alias_list = self.view.portal_list(cx, ids!(alias_list)); + script_apply_eval!(cx, alias_list, { + height: #(list_height) + }); + + // Empty-state hint when there are no aliases at all. + let no_aliases = row_count == 0; + self.view.label(cx, ids!(no_published_label)).set_visible(cx, no_aliases); + if no_aliases { self.view.label(cx, ids!(no_published_label)) - .set_text(cx, &alt_aliases.join("\n")); + .set_text(cx, tr_key(language, "room_settings.aliases.none_published")); } // Localized add control. @@ -1051,8 +1848,8 @@ impl RoomSettingsModal { .set_text(cx, tr_key(language, "room_settings.aliases.add_button")); // Permission gating: only users who can send `m.room.canonical_alias` - // see the edit control; everyone else gets a read-only hint. - self.view.view(cx, ids!(add_address_row)).set_visible(cx, can_manage); + // see the add control; it is also hidden while a write is in flight. + self.view.view(cx, ids!(add_address_row)).set_visible(cx, edit_enabled); if can_manage { self.view.label(cx, ids!(local_desc)) .set_text(cx, tr_key(language, "room_settings.aliases.alt_label")); @@ -1063,6 +1860,177 @@ impl RoomSettingsModal { self.view.redraw(cx); } + + /// Validate a raw address string and, on success, optimistically advertise + /// it and emit [`RoomSettingsAction::PublishAlias`]. + fn add_alias(&mut self, cx: &mut Cx, room_id: OwnedRoomId, raw: &str) { + use crate::shared::popup_list::{PopupKind, enqueue_popup_notification}; + + // P1-C: reject if a mutation is already in flight (controls are hidden + // while gated, but a queued click could still reach here). + if !self.alias_gate.can_submit() { + return; + } + + let Some(server_name) = crate::sliding_sync::current_user_id() + .map(|u| u.server_name().to_owned()) + else { + enqueue_popup_notification( + tr_key(self.language, "room_settings.aliases.sign_in_required").to_string(), + PopupKind::Error, + Some(4.0), + ); + return; + }; + + let valid_alias = match normalize_and_validate_alias(raw, &server_name) { + Ok(alias) => alias, + Err(_) => { + enqueue_popup_notification( + tr_key(self.language, "room_settings.aliases.invalid_format").to_string(), + PopupKind::Error, + Some(4.0), + ); + return; + } + }; + + // Optimistically advertise the new alias into alt_aliases. + let new_alts = advertise_alias_into_alts( + self.current_canonical.as_deref(), + &self.current_alts, + &valid_alias, + ); + self.snapshot_alias_state(); + self.current_alts = new_alts.clone(); + self.alias_gate.on_submit(); + self.render_alias_section(cx); + self.view.text_input(cx, ids!(add_address_input)).set_text(cx, ""); + + cx.action(RoomSettingsAction::PublishAlias { + room_id, + alias: valid_alias, + canonical: self.current_canonical.clone(), + alt_aliases: new_alts, + }); + } + + /// Promote `alias` to canonical: reconcile, optimistically update, emit. + fn set_canonical_alias(&mut self, cx: &mut Cx, alias: OwnedRoomAliasId) { + use crate::shared::popup_list::{PopupKind, enqueue_popup_notification}; + let Some(room_id) = self.room_id.clone() else { return }; + if !self.alias_gate.can_submit() { + return; + } + + match reconcile_canonical_alias( + self.current_canonical.as_deref(), + &self.current_alts, + AliasOp::SetCanonical(alias), + ) { + Ok(state) => { + self.snapshot_alias_state(); + self.current_canonical = state.canonical.clone(); + self.current_alts = state.alt_aliases.clone(); + self.alias_gate.on_submit(); + self.render_alias_section(cx); + cx.action(RoomSettingsAction::SetCanonicalAlias { + room_id, + canonical: state.canonical, + alt_aliases: state.alt_aliases, + }); + } + Err(CanonicalReconcileError::NotPublished) => { + enqueue_popup_notification( + tr_key(self.language, "room_settings.aliases.publish_failed").to_string(), + PopupKind::Error, + Some(4.0), + ); + } + } + } + + /// Remove `alias`: reconcile out of canonical/alts, optimistically update, emit. + fn remove_alias(&mut self, cx: &mut Cx, alias: OwnedRoomAliasId) { + let Some(room_id) = self.room_id.clone() else { return }; + if !self.alias_gate.can_submit() { + return; + } + // Defensive: only act on an alias that belongs to the room currently + // shown, so a stale per-row click that somehow survives a room switch + // can't unbind a foreign alias from the directory. (Set-as-main is + // already covered by `reconcile`'s `NotPublished`.) + let known = self.current_canonical.as_deref().is_some_and(|c| c.as_str() == alias.as_str()) + || self.current_alts.iter().any(|a| a.as_str() == alias.as_str()); + if !known { + return; + } + + // Remove never fails (see `reconcile_canonical_alias`). + if let Ok(state) = reconcile_canonical_alias( + self.current_canonical.as_deref(), + &self.current_alts, + AliasOp::Remove(alias.clone()), + ) { + self.snapshot_alias_state(); + self.current_canonical = state.canonical.clone(); + self.current_alts = state.alt_aliases.clone(); + self.alias_gate.on_submit(); + self.render_alias_section(cx); + cx.action(RoomSettingsAction::RemoveAlias { + room_id, + alias, + canonical: state.canonical, + alt_aliases: state.alt_aliases, + }); + } + } + + /// Snapshot the current alias state before an optimistic write, so it can + /// be restored if the server reports failure. Captured just before each + /// user-initiated edit as its pre-edit baseline. A single publish fans out + /// into two writes (directory + canonical_alias) that share this one + /// baseline, so a failure from either rolls back to the same pre-edit state. + /// + /// This is a single overwritable slot: if a user starts a second edit before + /// the first write's result returns, a late failure would roll back to the + /// wrong baseline. That transient case is self-healing — every write result + /// triggers a `FetchRoomSettings` in `app.rs`, whose authoritative + /// `apply_alias_settings` refresh overwrites the optimistic state and clears + /// this snapshot regardless of which write failed. + fn snapshot_alias_state(&mut self) { + self.alias_snapshot = + Some((self.current_canonical.clone(), self.current_alts.clone())); + } + + /// React to a server outcome for an alias write. On failure, roll back the + /// optimistic state to the pre-write snapshot; on success, commit it. The + /// user-facing error toast is raised by `app.rs` so it fires even when this + /// modal has already been closed. + fn apply_write_result( + &mut self, + cx: &mut Cx, + result: &crate::sliding_sync::RoomAliasWriteResultAction, + ) { + // Advance the in-flight gate keyed on whether the server was attempted. + // Attempted (success OR server-side failure): hold until this op's own + // reconcile fetch lands. Preflight failure: release now (no fetch coming). + self.alias_gate.on_result(result.attempted); + if result.error.is_some() { + // Roll back optimistic UI to the pre-write baseline. For an attempted + // failure the gate stays held (AwaitingRefresh) so controls remain + // locked until this op's reconcile fetch; for a preflight failure the + // gate is now Idle so the re-render re-enables the controls. + if let Some((canonical, alts)) = self.alias_snapshot.clone() { + self.current_canonical = canonical; + self.current_alts = alts; + } + self.render_alias_section(cx); + } + // On success we leave the optimistic state in place with the gate still + // held (AwaitingRefresh); this op's own `FetchRoomSettings` refresh + // reconciles it with authoritative server state and releases the gate. + } } impl RoomSettingsModalRef { @@ -1074,28 +2042,44 @@ impl RoomSettingsModalRef { room_name: &str, room_topic: &str, canonical_alias: Option<&str>, + alias_stage: Option, ) { let Some(mut inner) = self.borrow_mut() else { return }; - inner.show(cx, room_id, room_name, room_topic, canonical_alias); + inner.show(cx, room_id, room_name, room_topic, canonical_alias, alias_stage); } - /// Apply asynchronously-fetched settings (topic, is_public). - pub fn apply_fetched_settings(&self, cx: &mut Cx, topic: Option, is_public: bool) { + /// Apply asynchronously-fetched settings (topic, is_public). Dropped if the + /// response is for a room other than the one currently shown (P1-B). + pub fn apply_fetched_settings( + &self, + cx: &mut Cx, + room_id: &RoomId, + topic: Option, + is_public: bool, + ) { let Some(mut inner) = self.borrow_mut() else { return }; - inner.apply_fetched_settings(cx, topic, is_public); + inner.apply_fetched_settings(cx, room_id, topic, is_public); } /// Apply fetched alias data (canonical + alt aliases) and permission gating. + /// Dropped if the response is for a room other than the one shown (P1-B). pub fn apply_alias_settings( &self, cx: &mut Cx, + room_id: &RoomId, language: AppLanguage, - canonical_alias: Option, - alt_aliases: Vec, + canonical_alias: Option, + alt_aliases: Vec, can_manage: bool, ) { let Some(mut inner) = self.borrow_mut() else { return }; - inner.apply_alias_settings(cx, language, canonical_alias, alt_aliases, can_manage); + inner.apply_alias_settings(cx, room_id, language, canonical_alias, alt_aliases, can_manage); + } + + /// Release a stranded alias gate when its reconcile fetch was unavailable. + pub fn release_alias_lock(&self, cx: &mut Cx, room_id: &RoomId) { + let Some(mut inner) = self.borrow_mut() else { return }; + inner.release_alias_lock(cx, room_id); } /// Update the avatar widget after a successful upload. diff --git a/src/i18n.rs b/src/i18n.rs index 89c684f41..73281e6df 100644 --- a/src/i18n.rs +++ b/src/i18n.rs @@ -167,6 +167,12 @@ mod tests { "room_settings.aliases.invalid_format", "room_settings.aliases.publish_failed", "room_settings.aliases.readonly_hint", + "room_settings.aliases.no_main_address", + "room_settings.aliases.none_published", + "room_settings.aliases.publishing", + "room_settings.aliases.updating_main", + "room_settings.aliases.removing", + "room_settings.aliases.sign_in_required", ] { for language in AppLanguage::ALL { assert!( diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index 9a5f383e5..9d4746fef 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -36,7 +36,7 @@ use matrix_sdk_ui::{ RoomListService, Timeline, encryption_sync_service, room_list_service::{RoomListItem, RoomListLoadingState, SyncIndicator, filters}, sync_service::{self, SyncService}, timeline::{LatestEventValue, RoomExt, TimelineEventItemId, TimelineFocus, TimelineItem, TimelineReadReceiptTracking, TimelineDetails} }; use robius_open::Uri; -use ruma::{OwnedRoomAliasId, OwnedRoomOrAliasId, RoomId, events::tag::Tags}; +use ruma::{OwnedRoomAliasId, OwnedRoomOrAliasId, RoomAliasId, RoomId, events::tag::Tags}; use tokio::{ runtime::Handle, sync::{broadcast, mpsc::{Sender, UnboundedReceiver, UnboundedSender}, oneshot, watch, Notify}, task::JoinHandle, time::error::Elapsed, @@ -712,6 +712,15 @@ pub struct RoomSettingsFetchedAction { pub can_manage_aliases: bool, } +/// Posted when a [`MatrixRequest::FetchRoomSettings`] could not produce data +/// (no client, or the room isn't currently available). It carries no settings — +/// its sole job is to let a waiting alias write release its in-flight gate / +/// pending registry instead of stranding the edit controls disabled. +#[derive(Clone, Debug)] +pub struct RoomSettingsFetchUnavailableAction { + pub room_id: OwnedRoomId, +} + /// Posted after a room avatar is successfully uploaded and set. #[derive(Clone, Debug)] pub struct RoomAvatarUploadedAction { @@ -720,6 +729,76 @@ pub struct RoomAvatarUploadedAction { pub image_data: Arc<[u8]>, } +/// Which alias write a [`RoomAliasWriteResultAction`] reports the outcome of. +#[derive(Clone, Debug)] +pub enum AliasWriteKind { + /// `MatrixRequest::PublishRoomAlias` (directory registration). + Publish, + /// `MatrixRequest::RemoveRoomAlias` (directory unbind). + Remove, + /// `MatrixRequest::SetRoomCanonicalAlias` (`m.room.canonical_alias`). + SetCanonical, +} + +/// Server outcome of an alias write, used by the Room Settings modal to commit +/// or roll back its optimistic UI and to surface server errors as a toast. +#[derive(Clone, Debug)] +pub struct RoomAliasWriteResultAction { + pub room_id: OwnedRoomId, + pub kind: AliasWriteKind, + /// The alias involved, when the write targets a specific one. + pub alias: Option, + /// `None` on success; a human-readable server error message on failure. + pub error: Option, + /// Whether the server was actually contacted. `false` only for preflight + /// failures (e.g. no client) where nothing was sent and state is unchanged; + /// app.rs skips the reconcile fetch for these, and the modal/registry + /// release their in-flight gate immediately instead of awaiting a refresh. + pub attempted: bool, +} + +/// HTTP status code of a failed matrix request, if it carries a client-API error. +fn alias_error_status(e: &matrix_sdk::HttpError) -> Option { + e.as_client_api_error().map(|api| api.status_code.as_u16()) +} + +/// Whether `alias` currently resolves to `room_id` in the room directory. Used +/// on the publish-conflict path to decide idempotent success (the mapping we +/// wanted already exists). Any resolution failure counts as "not this room". +async fn alias_maps_to_room(client: &Client, alias: &RoomAliasId, room_id: &RoomId) -> bool { + let request = matrix_sdk::ruma::api::client::alias::get_alias::v3::Request::new(alias.to_owned()); + matches!(client.send(request).await, Ok(resp) if resp.room_id.as_str() == room_id.as_str()) +} + +/// Send the room's `m.room.canonical_alias` state event (`alias` + `alt_aliases`). +/// +/// Shared by the sequenced publish/remove flows (as their second, gated step) +/// and by the standalone Set-as-main request. Returns a human-readable error +/// string on failure so callers can surface it in a toast. +async fn set_room_canonical_alias( + client: &Client, + room_id: &RoomId, + alias: Option, + alt_aliases: Vec, +) -> Result<(), String> { + let Some(room) = client.get_room(room_id) else { + return Err(format!("room {room_id} not found")); + }; + let mut content = matrix_sdk::ruma::events::room::canonical_alias::RoomCanonicalAliasEventContent::new(); + content.alias = alias; + content.alt_aliases = alt_aliases; + match room.send_state_event(content).await { + Ok(_) => { + log!("Set canonical alias for room {room_id}."); + Ok(()) + } + Err(e) => { + error!("Failed to set canonical alias for room {room_id}: {e:?}"); + Err(e.to_string()) + } + } +} + /// Actions emitted in response to a [`MatrixRequest::GenerateMatrixLink`]. #[derive(Clone, Debug)] pub enum MatrixLinkAction { @@ -1613,15 +1692,29 @@ pub enum MatrixRequest { room_id: OwnedRoomId, }, /// Publish a new alias into the room directory, mapping it to this room - /// (`PUT /directory/room/{alias}`). Requires directory permission on the - /// alias's homeserver. + /// (`PUT /directory/room/{alias}`), then — only on success — advertise it + /// into `m.room.canonical_alias` using `canonical`/`alt_aliases`. The two + /// writes are sequenced (not parallel) so a directory failure never leaves + /// an advertised-but-unregistered alias. Requires directory permission on + /// the alias's homeserver. PublishRoomAlias { room_id: OwnedRoomId, alias: OwnedRoomAliasId, + /// Reconciled canonical alias to write once the directory step succeeds. + canonical: Option, + /// Reconciled alt_aliases (already including `alias`) to advertise. + alt_aliases: Vec, }, - /// Remove an alias from the room directory (`DELETE /directory/room/{alias}`). + /// Remove an alias from the room directory (`DELETE /directory/room/{alias}`), + /// then — only on success — de-advertise it from `m.room.canonical_alias` + /// using `canonical`/`alt_aliases`. Sequenced like [`Self::PublishRoomAlias`]. RemoveRoomAlias { + room_id: OwnedRoomId, alias: OwnedRoomAliasId, + /// Reconciled canonical alias to write once the directory step succeeds. + canonical: Option, + /// Reconciled alt_aliases (already excluding `alias`) to write. + alt_aliases: Vec, }, /// Set the room's canonical alias and alt aliases via the /// `m.room.canonical_alias` state event. Requires the corresponding power @@ -4313,7 +4406,13 @@ async fn matrix_worker_task( } MatrixRequest::FetchRoomSettings { room_id } => { - let Some(client) = get_client() else { continue }; + // No client → still signal "unavailable" so a waiting alias write + // releases its gate/registry instead of stranding (never silently + // drop this fetch — the alias reconcile depends on a reply). + let Some(client) = get_client() else { + Cx::post_action(RoomSettingsFetchUnavailableAction { room_id }); + continue; + }; let _fetch_room_settings_task = Handle::current().spawn(async move { if let Some(room) = client.get_room(&room_id) { let topic = room.topic(); @@ -4336,70 +4435,160 @@ async fn matrix_worker_task( alt_aliases, can_manage_aliases, }); + } else { + // Room not currently available — release any waiting gate. + Cx::post_action(RoomSettingsFetchUnavailableAction { room_id }); } }); } - MatrixRequest::PublishRoomAlias { room_id, alias } => { - let Some(client) = get_client() else { continue }; + MatrixRequest::PublishRoomAlias { room_id, alias, canonical, alt_aliases } => { + // P2 preflight: if the client is gone nothing is sent (attempted: + // false), so the modal/registry release immediately with no fetch. + let Some(client) = get_client() else { + let _ = (canonical, alt_aliases); + Cx::post_action(RoomAliasWriteResultAction { + room_id, + kind: AliasWriteKind::Publish, + alias: Some(alias), + error: Some("Not signed in — couldn't publish the address".to_string()), + attempted: false, + }); + continue; + }; let _publish_alias_task = Handle::current().spawn(async move { + use crate::home::room_settings_modal::{ + next_step_after_directory_write, publish_alias_treat_as_success, SequencedAliasStep, + }; + // Step 1 — register the alias in the room directory. Idempotent + // repair (P1-3): a 409 conflict is success iff the alias already + // maps to THIS room, so a retry after a prior partial publish + // (registered-but-unadvertised) proceeds to re-advertise instead + // of dying on "already in use". let request = matrix_sdk::ruma::api::client::alias::create_alias::v3::Request::new( alias.clone(), - room_id, + room_id.clone(), ); - match client.send(request).await { - Ok(_) => log!("Published room alias {alias}."), + let dir_outcome: Result<(), String> = match client.send(request).await { + Ok(_) => Ok(()), Err(e) => { - error!("Failed to publish room alias {alias}: {e:?}"); - enqueue_popup_notification( - format!("Couldn't publish alias {alias}"), - crate::shared::popup_list::PopupKind::Error, - Some(5.0), - ); + let conflict = alias_error_status(&e) == Some(409); + let maps_here = conflict && alias_maps_to_room(&client, &alias, &room_id).await; + if publish_alias_treat_as_success(conflict, maps_here) { + log!("Alias {alias} already maps to this room; treating publish as success."); + Ok(()) + } else { + error!("Failed to publish room alias {alias}: {e:?}"); + Err(format!("Couldn't publish address {alias}: {e}")) + } } - } + }; + let error = match next_step_after_directory_write(dir_outcome.is_ok()) { + SequencedAliasStep::Abort => Some(dir_outcome.unwrap_err()), + // Step 2 — only after a successful (or repaired) directory + // write, advertise the alias into `m.room.canonical_alias`. + SequencedAliasStep::WriteCanonical => { + log!("Published room alias {alias}."); + set_room_canonical_alias(&client, &room_id, canonical, alt_aliases) + .await + .err() + .map(|e| format!("Couldn't advertise address {alias}: {e}")) + } + }; + Cx::post_action(RoomAliasWriteResultAction { + room_id, + kind: AliasWriteKind::Publish, + alias: Some(alias), + error, + attempted: true, + }); }); } - MatrixRequest::RemoveRoomAlias { alias } => { - let Some(client) = get_client() else { continue }; + MatrixRequest::RemoveRoomAlias { room_id, alias, canonical, alt_aliases } => { + // P2 preflight: nothing sent → attempted: false. + let Some(client) = get_client() else { + let _ = (canonical, alt_aliases); + Cx::post_action(RoomAliasWriteResultAction { + room_id, + kind: AliasWriteKind::Remove, + alias: Some(alias), + error: Some("Not signed in — couldn't remove the address".to_string()), + attempted: false, + }); + continue; + }; let _remove_alias_task = Handle::current().spawn(async move { + use crate::home::room_settings_modal::{ + next_step_after_directory_write, remove_alias_treat_as_success, SequencedAliasStep, + }; + // Step 1 — unbind the alias from the room directory. Idempotent + // repair (P1-3): a 404 "not found" is success (already unbound), + // so a retry after a prior partial remove + // (unbound-but-still-advertised) proceeds to de-advertise. let request = matrix_sdk::ruma::api::client::alias::delete_alias::v3::Request::new( alias.clone(), ); - match client.send(request).await { - Ok(_) => log!("Removed room alias {alias}."), + let dir_outcome: Result<(), String> = match client.send(request).await { + Ok(_) => Ok(()), Err(e) => { - error!("Failed to remove room alias {alias}: {e:?}"); - enqueue_popup_notification( - format!("Couldn't remove alias {alias}"), - crate::shared::popup_list::PopupKind::Error, - Some(5.0), - ); + let not_found = alias_error_status(&e) == Some(404); + if remove_alias_treat_as_success(not_found) { + log!("Alias {alias} already unbound; treating remove as success."); + Ok(()) + } else { + error!("Failed to remove room alias {alias}: {e:?}"); + Err(format!("Couldn't remove address {alias}: {e}")) + } } - } + }; + let error = match next_step_after_directory_write(dir_outcome.is_ok()) { + SequencedAliasStep::Abort => Some(dir_outcome.unwrap_err()), + // Step 2 — only after a successful (or repaired) unbind, drop + // the alias from `m.room.canonical_alias`. + SequencedAliasStep::WriteCanonical => { + log!("Removed room alias {alias}."); + set_room_canonical_alias(&client, &room_id, canonical, alt_aliases) + .await + .err() + .map(|e| format!("Couldn't update the room's addresses: {e}")) + } + }; + Cx::post_action(RoomAliasWriteResultAction { + room_id, + kind: AliasWriteKind::Remove, + alias: Some(alias), + error, + attempted: true, + }); }); } MatrixRequest::SetRoomCanonicalAlias { room_id, alias, alt_aliases } => { - let Some(client) = get_client() else { continue }; + // P2 preflight: nothing sent → attempted: false. + let Some(client) = get_client() else { + let _ = (alias, alt_aliases); + Cx::post_action(RoomAliasWriteResultAction { + room_id, + kind: AliasWriteKind::SetCanonical, + alias: None, + error: Some("Not signed in — couldn't update the room's main address".to_string()), + attempted: false, + }); + continue; + }; let _set_canonical_alias_task = Handle::current().spawn(async move { - if let Some(room) = client.get_room(&room_id) { - let mut content = matrix_sdk::ruma::events::room::canonical_alias::RoomCanonicalAliasEventContent::new(); - content.alias = alias; - content.alt_aliases = alt_aliases; - match room.send_state_event(content).await { - Ok(_) => log!("Set canonical alias for room {room_id}."), - Err(e) => { - error!("Failed to set canonical alias for room {room_id}: {e:?}"); - enqueue_popup_notification( - "Couldn't update the room's canonical alias", - crate::shared::popup_list::PopupKind::Error, - Some(5.0), - ); - } - } - } + let error = set_room_canonical_alias(&client, &room_id, alias, alt_aliases) + .await + .err() + .map(|e| format!("Couldn't update the room's main address: {e}")); + Cx::post_action(RoomAliasWriteResultAction { + room_id, + kind: AliasWriteKind::SetCanonical, + alias: None, + error, + attempted: true, + }); }); } From 7249d920c1f1906b92a2a695926fc5007ab949e3 Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Sun, 26 Jul 2026 13:20:51 +0800 Subject: [PATCH 07/11] fix(room-aliases): generation-tag settings fetches so only a write's own reconcile releases its gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Settings fetches carried only room_id, so a stale or unrelated fetch could be mistaken for a specific alias write's reconcile — releasing the gate / clearing the pending registry early and repainting stale full-state (codex P1, msg_0201). Every FetchRoomSettings now carries a RoomSettingsFetchReason (Open vs AliasReconcile(generation)), echoed through both RoomSettingsFetchedAction and RoomSettingsFetchUnavailableAction: - Each mutation takes a monotonic generation (modal-owned); the gate becomes AwaitingResult(gen)/AwaitingRefresh(gen) and the registry stores that generation. - A fetch releases the gate / clears the registry ONLY on AliasReconcile with a matching generation while AwaitingRefresh(gen)/AwaitingReconcile. An open-fetch never consumes a write reconcile, and a mismatched generation is ignored — independent of arrival timing. - apply_alias_settings routes the disposition (Ignore / Apply / ApplyAndRelease) before touching state, so a stale fetch can't clobber optimistic state or the rollback snapshot. release_alias_lock (unavailable path) releases only on the matching reconcile. - Registry and gate stay in lockstep for same-Actions-batch ordering (App-then- UI) because both decide by generation+purpose, not order. - show() reopens a pending room in the matching gate state carrying the original generation, and keeps the local generation source ahead of it. Adds 2 codex-named regression tests (two open-fetches where the stale one returns after the write result; write result + stale fetch in one batch), plus generation-aware gate/registry unit tests. cargo test --lib: 569 passed / 0 failed. agent-spec lint: 100%. Co-Authored-By: Claude Fable 5 --- src/app.rs | 54 +++-- src/home/room_settings_modal.rs | 415 +++++++++++++++++++++++--------- src/sliding_sync.rs | 22 +- 3 files changed, 350 insertions(+), 141 deletions(-) diff --git a/src/app.rs b/src/app.rs index 97f399d0f..c96bbd67a 100644 --- a/src/app.rs +++ b/src/app.rs @@ -19,7 +19,7 @@ use crate::{ add_menu::{AddMenuAction, AddMenuWidgetRefExt}, add_room::{CreateRoomModalAction, CreateRoomModalWidgetRefExt, JoinRoomModalAction, JoinRoomModalWidgetRefExt, StartChatModalAction, StartChatModalWidgetRefExt}, bot_binding_modal::{BotBindingModalAction, BotBindingModalWidgetRefExt}, - event_source_modal::{EventSourceModalAction, EventSourceModalWidgetRefExt}, invite_modal::{InviteModalAction, InviteModalWidgetRefExt, mark_invite_modal_closed}, invite_screen::{InviteScreenWidgetRefExt, LeaveRoomResultAction}, main_desktop_ui::MainDesktopUiAction, navigation_tab_bar::{NavigationBarAction, SelectedTab}, new_message_context_menu::NewMessageContextMenuWidgetRefExt, room_context_menu::{RoomContextMenuAction, RoomContextMenuWidgetRefExt}, room_screen::{InviteAction, MessageAction, ReportRoomModalAction, ReportRoomModalWidgetRefExt, ReportRoomResultAction, RoomScreenWidgetRefExt, TimelineUpdate, clear_timeline_states, set_room_info_action_modal_open}, room_settings_modal::{PendingAliasWrites, RoomSettingsAction, RoomSettingsModalWidgetRefExt}, rooms_list::{RoomsListAction, RoomsListRef, RoomsListUpdate, clear_all_invited_rooms, enqueue_rooms_list_update}, rooms_list_header::RoomsListHeaderAction, space_lobby::SpaceLobbyScreenWidgetRefExt, spaces_bar::SpacesBarRef + event_source_modal::{EventSourceModalAction, EventSourceModalWidgetRefExt}, invite_modal::{InviteModalAction, InviteModalWidgetRefExt, mark_invite_modal_closed}, invite_screen::{InviteScreenWidgetRefExt, LeaveRoomResultAction}, main_desktop_ui::MainDesktopUiAction, navigation_tab_bar::{NavigationBarAction, SelectedTab}, new_message_context_menu::NewMessageContextMenuWidgetRefExt, room_context_menu::{RoomContextMenuAction, RoomContextMenuWidgetRefExt}, room_screen::{InviteAction, MessageAction, ReportRoomModalAction, ReportRoomModalWidgetRefExt, ReportRoomResultAction, RoomScreenWidgetRefExt, TimelineUpdate, clear_timeline_states, set_room_info_action_modal_open}, room_settings_modal::{PendingAliasWrites, RoomSettingsAction, RoomSettingsFetchReason, RoomSettingsModalWidgetRefExt}, rooms_list::{RoomsListAction, RoomsListRef, RoomsListUpdate, clear_all_invited_rooms, enqueue_rooms_list_update}, rooms_list_header::RoomsListHeaderAction, space_lobby::SpaceLobbyScreenWidgetRefExt, spaces_bar::SpacesBarRef }, i18n::{AppLanguage, tr_fmt, tr_key}, join_leave_room_modal::{ JoinLeaveModalKind, JoinLeaveRoomModalAction, JoinLeaveRoomModalWidgetRefExt }, login::login_screen::LoginAction, logout::logout_confirm_modal::{LogoutAction, LogoutConfirmModalAction, LogoutConfirmModalWidgetRefExt}, persistence, profile::user_profile_cache::clear_user_profile_cache, register::RegisterAction, room::BasicRoomDetails, shared::{confirmation_modal::{ConfirmationModalAction, ConfirmationModalContent, ConfirmationModalWidgetRefExt}, file_upload_modal::{FilePreviewerAction, FileUploadModalWidgetRefExt}, forward_modal::{ForwardMessageModalAction, ForwardMessageModalWidgetRefExt}, image_viewer::{ImageViewerAction, LoadState}, popup_list::{PopupKind, enqueue_popup_notification, enqueue_notification, NotificationItem, NotificationAction, NotifActionStyle}, room_filter_input_bar::FilterAction}, sliding_sync::{DirectMessageRoomAction, MatrixRequest, RemoteDirectorySearchKind, RemoteDirectorySearchResult, RoomSettingsFetchedAction, RoomAvatarUploadedAction, TimelineKind, AccountSwitchAction, current_user_id, get_client, submit_async_request, get_timeline_update_sender}, updater::{UpdateCheckOutcome, check_for_updates, load_skipped_update_version, save_skipped_update_version, update_release_page_url}, utils::RoomNameId, verification::VerificationAction, verification_modal::{ @@ -1937,7 +1937,12 @@ impl MatchEvent for App { self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) .show_settings(cx, room_id.clone(), &room_name, "", alias_str, alias_stage); self.ui.modal(cx, ids!(room_settings_modal)).open(cx); - submit_async_request(MatrixRequest::FetchRoomSettings { room_id }); + // Open-fetch: not tied to any write, so it can never consume a + // write reconcile (purpose = Open). + submit_async_request(MatrixRequest::FetchRoomSettings { + room_id, + reason: RoomSettingsFetchReason::Open, + }); continue; } Some(RoomSettingsAction::Close) | Some(RoomSettingsAction::Cancel) => { @@ -1973,12 +1978,12 @@ impl MatchEvent for App { self.ui.modal(cx, ids!(room_settings_modal)).close(cx); continue; } - Some(RoomSettingsAction::PublishAlias { room_id, alias, canonical, alt_aliases }) => { + Some(RoomSettingsAction::PublishAlias { room_id, alias, canonical, alt_aliases, generation }) => { // The modal already validated the alias and optimistically // advertised it. A single sequenced request registers it in // the directory, then advertises it into `m.room.canonical_alias` // only if that succeeds (see MatrixRequest::PublishRoomAlias). - self.pending_alias_writes.register(room_id.clone()); + self.pending_alias_writes.register(room_id.clone(), *generation); submit_async_request(MatrixRequest::PublishRoomAlias { room_id: room_id.clone(), alias: alias.clone(), @@ -1992,8 +1997,8 @@ impl MatchEvent for App { ); continue; } - Some(RoomSettingsAction::SetCanonicalAlias { room_id, canonical, alt_aliases }) => { - self.pending_alias_writes.register(room_id.clone()); + Some(RoomSettingsAction::SetCanonicalAlias { room_id, canonical, alt_aliases, generation }) => { + self.pending_alias_writes.register(room_id.clone(), *generation); submit_async_request(MatrixRequest::SetRoomCanonicalAlias { room_id: room_id.clone(), alias: canonical.clone(), @@ -2006,10 +2011,10 @@ impl MatchEvent for App { ); continue; } - Some(RoomSettingsAction::RemoveAlias { room_id, alias, canonical, alt_aliases }) => { + Some(RoomSettingsAction::RemoveAlias { room_id, alias, canonical, alt_aliases, generation }) => { // Single sequenced request: unbind from the directory, then // drop it from `m.room.canonical_alias` only if that succeeds. - self.pending_alias_writes.register(room_id.clone()); + self.pending_alias_writes.register(room_id.clone(), *generation); submit_async_request(MatrixRequest::RemoveRoomAlias { room_id: room_id.clone(), alias: alias.clone(), @@ -2043,10 +2048,13 @@ impl MatchEvent for App { // Handle RoomSettingsFetchedAction. if let Some(fetched) = action.downcast_ref::() { - // This authoritative fetch reconciles any in-flight alias write - // for the room — clear it from the pending registry (P1-2) so the - // controls re-enable on the next open. - self.pending_alias_writes.on_reconciled(&fetched.room_id); + // Clear the pending registry entry ONLY on the matching write + // reconcile (generation + purpose). An open-fetch or a stale/ + // mismatched reconcile is ignored, keeping registry and gate in + // lockstep regardless of same-batch processing order (P1). + if let RoomSettingsFetchReason::AliasReconcile(generation) = fetched.reason { + self.pending_alias_writes.on_reconciled(&fetched.room_id, generation); + } // The modal is a singleton reused across rooms; it drops the // response if `fetched.room_id` isn't the room it currently shows // (P1-B: stale/out-of-order fetch guard). @@ -2056,6 +2064,7 @@ impl MatchEvent for App { .apply_alias_settings( cx, &fetched.room_id, + fetched.reason, self.app_state.app_language, fetched.canonical_alias.clone(), fetched.alt_aliases.clone(), @@ -2067,10 +2076,14 @@ impl MatchEvent for App { // A reconcile fetch could not produce data (no client / room gone): // clear the pending entry and release the modal's gate so the alias // controls never strand disabled (they keep the optimistic state). + // Matched by generation + purpose, so only the write's own reconcile + // releases it — an open-fetch failure never does. if let Some(unavailable) = action.downcast_ref::() { - self.pending_alias_writes.on_reconciled(&unavailable.room_id); + if let RoomSettingsFetchReason::AliasReconcile(generation) = unavailable.reason { + self.pending_alias_writes.on_reconciled(&unavailable.room_id, generation); + } self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) - .release_alias_lock(cx, &unavailable.room_id); + .release_alias_lock(cx, &unavailable.room_id, unavailable.reason); continue; } @@ -2091,10 +2104,17 @@ impl MatchEvent for App { enqueue_popup_notification(message.clone(), PopupKind::Error, Some(5.0)); } self.pending_alias_writes.on_result(&result.room_id, result.attempted); + // Reconcile fetch tagged with THIS write's generation (from the + // registry, which on_result kept for an attempted write), so only + // this fetch can release the write's gate. Skip if there's no + // pending entry (already reconciled) or it was a preflight failure. if result.attempted { - submit_async_request(MatrixRequest::FetchRoomSettings { - room_id: result.room_id.clone(), - }); + if let Some((_, generation)) = self.pending_alias_writes.stage(&result.room_id) { + submit_async_request(MatrixRequest::FetchRoomSettings { + room_id: result.room_id.clone(), + reason: RoomSettingsFetchReason::AliasReconcile(generation), + }); + } } continue; } diff --git a/src/home/room_settings_modal.rs b/src/home/room_settings_modal.rs index c1a1cb8f3..c523d925e 100644 --- a/src/home/room_settings_modal.rs +++ b/src/home/room_settings_modal.rs @@ -196,6 +196,20 @@ pub fn next_step_after_directory_write(directory_ok: bool) -> SequencedAliasStep } } +/// Why a `FetchRoomSettings` was issued — carried through the request and echoed +/// back on both the fetched and unavailable responses so a stale or unrelated +/// fetch can never be mistaken for a specific write's reconcile (the race codex +/// flagged: settings fetches previously carried only `room_id`). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RoomSettingsFetchReason { + /// Opened the modal (or another non-write refresh). Not tied to any alias + /// write, so it must NEVER consume a write's reconcile gate/registry entry. + Open, + /// The authoritative reconcile for the alias write of this generation. Only a + /// matching generation may release that write's gate / clear its registry. + AliasReconcile(u64), +} + /// Serializes alias mutations for the modal's room: at most one may be in /// flight at a time. Overlapping mutations are the cross-operation race that /// can resurrect an unbound alias — each write snapshots the *full* @@ -203,19 +217,31 @@ pub fn next_step_after_directory_write(directory_ok: bool) -> SequencedAliasStep /// gate keeps the edit controls disabled from submit until the operation fully /// settles, so the next mutation always builds on reconciled state. /// -/// A successful write holds the gate until the authoritative refresh confirms -/// server state; a *failed* write releases immediately, since the modal has -/// already rolled back to the pre-edit snapshot (and the refresh may never -/// arrive, e.g. the client was torn down). +/// Each in-flight write carries a monotonic *generation*. Only its own +/// reconcile fetch — matched by generation AND purpose — releases the gate, so a +/// stale open-fetch or a mismatched reconcile can neither release it nor clobber +/// the optimistic state, independent of arrival timing. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum AliasWriteGate { /// No mutation in flight — edit controls are live and submits are allowed. #[default] Idle, - /// A mutation was submitted; awaiting its server write result. - AwaitingResult, - /// The write succeeded; awaiting the authoritative `FetchRoomSettings` refresh. - AwaitingRefresh, + /// A mutation (this generation) was submitted; awaiting its server result. + AwaitingResult(u64), + /// The write (this generation) reached the server; awaiting its reconcile. + AwaitingRefresh(u64), +} + +/// What the modal should do with an incoming settings fetch, decided purely from +/// the gate state and the fetch's [`RoomSettingsFetchReason`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum FetchDisposition { + /// Ignore it (stale/unrelated) — do not touch state or the gate. + Ignore, + /// Apply its data (authoritative load) but leave the gate as-is. + Apply, + /// Apply its data AND release the gate — this is the matching reconcile. + ApplyAndRelease, } impl AliasWriteGate { @@ -224,11 +250,11 @@ impl AliasWriteGate { matches!(self, AliasWriteGate::Idle) } - /// Record a submitted mutation. Returns `false` (and does nothing) if a - /// mutation is already in flight, so callers can reject the overlap. - pub fn on_submit(&mut self) -> bool { + /// Record a submitted mutation of `generation`. Returns `false` (and does + /// nothing) if a mutation is already in flight, so callers reject the overlap. + pub fn on_submit(&mut self, generation: u64) -> bool { if self.can_submit() { - *self = AliasWriteGate::AwaitingResult; + *self = AliasWriteGate::AwaitingResult(generation); true } else { false @@ -237,42 +263,66 @@ impl AliasWriteGate { /// Record the write result, keyed on whether the server was actually /// *attempted*: - /// - `attempted == true` (a request was sent — success OR server-side - /// failure): hold the gate until this op's own reconciliation fetch lands, - /// because a `FetchRoomSettings` is in flight for it. Releasing now would - /// let a new op start while that fetch is outstanding — the stray-refresh - /// race codex flagged. - /// - `attempted == false` (preflight failure, e.g. no client — nothing was - /// sent, no fetch spawned, state unchanged): release straight to `Idle`. + /// - `attempted == true` (request sent — success OR server-side failure): + /// hold the gate at `AwaitingRefresh(gen)` until this op's own reconcile + /// (matched by `gen`) lands. Releasing now would let a new op start while + /// that fetch is outstanding. + /// - `attempted == false` (preflight failure — nothing sent, no fetch, state + /// unchanged): release straight to `Idle`. /// /// Ignores stray results (not in `AwaitingResult`). pub fn on_result(&mut self, attempted: bool) { - if matches!(self, AliasWriteGate::AwaitingResult) { + if let AliasWriteGate::AwaitingResult(generation) = *self { *self = if attempted { - AliasWriteGate::AwaitingRefresh + AliasWriteGate::AwaitingRefresh(generation) } else { AliasWriteGate::Idle }; } } - /// Whether an incoming authoritative refresh should be applied to the modal. - /// A refresh arriving while `AwaitingResult` is a stray fetch (this op's own - /// fetch is only spawned after its result) — applying it would clobber the - /// optimistic state mid-flight, so it is rejected. `Idle` accepts the - /// open/initial fetch; `AwaitingRefresh` accepts this op's own reconcile. - pub fn should_accept_refresh(self) -> bool { - !matches!(self, AliasWriteGate::AwaitingResult) + /// Decide what to do with a settings fetch, matched by generation + purpose: + /// - An `Open` fetch applies its data ONLY while `Idle` (initial/reopen load); + /// during any in-flight write it is stale → `Ignore` (never clobbers, never + /// releases). + /// - An `AliasReconcile(g)` releases and applies ONLY when the gate is + /// `AwaitingRefresh(g)` with the same generation; otherwise `Ignore`. + pub fn disposition(self, reason: RoomSettingsFetchReason) -> FetchDisposition { + match (self, reason) { + (AliasWriteGate::Idle, RoomSettingsFetchReason::Open) => FetchDisposition::Apply, + (AliasWriteGate::AwaitingRefresh(g), RoomSettingsFetchReason::AliasReconcile(r)) + if g == r => + { + FetchDisposition::ApplyAndRelease + } + _ => FetchDisposition::Ignore, + } } - /// The authoritative refresh landed — release only from `AwaitingRefresh` - /// (this op's own reconciliation). Callers must gate the state overwrite on - /// [`Self::should_accept_refresh`] and call this to advance the enum. Room - /// switches reset to `Idle` directly in `show`, not via this method. - pub fn on_refresh(&mut self) { - if matches!(self, AliasWriteGate::AwaitingRefresh) { + /// Apply the disposition of a fetch, releasing the gate iff it is the + /// matching reconcile. Returns the disposition so the caller can decide + /// whether to overwrite state. + pub fn on_fetch(&mut self, reason: RoomSettingsFetchReason) -> FetchDisposition { + let disposition = self.disposition(reason); + if disposition == FetchDisposition::ApplyAndRelease { *self = AliasWriteGate::Idle; } + disposition + } + + /// A settings fetch could not produce data (no client / room gone). Release + /// the gate ONLY if it is the matching reconcile for a write awaiting one + /// (so the controls never strand disabled). Returns whether it released. + pub fn on_fetch_unavailable(&mut self, reason: RoomSettingsFetchReason) -> bool { + if let (AliasWriteGate::AwaitingRefresh(g), RoomSettingsFetchReason::AliasReconcile(r)) = + (*self, reason) + { + if g == r { + *self = AliasWriteGate::Idle; + return true; + } + } + false } } @@ -298,38 +348,53 @@ pub enum PendingAliasStage { /// Ownership: `app.rs` holds the single instance; `show` consults it to decide /// whether to open a room locked. Transitions survive the modal being torn down /// and reopened. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct PendingAliasEntry { + stage: PendingAliasStage, + /// Generation of the in-flight write; only a reconcile with this generation + /// may clear the entry. + generation: u64, +} + #[derive(Debug, Default, Clone)] pub struct PendingAliasWrites { - rooms: std::collections::HashMap, + rooms: std::collections::HashMap, } impl PendingAliasWrites { - /// A mutation was submitted for `room_id` — mark it pending (`Submitted`). - pub fn register(&mut self, room_id: OwnedRoomId) { - self.rooms.insert(room_id, PendingAliasStage::Submitted); + /// A mutation of `generation` was submitted for `room_id` — mark it pending. + pub fn register(&mut self, room_id: OwnedRoomId, generation: u64) { + self.rooms.insert( + room_id, + PendingAliasEntry { stage: PendingAliasStage::Submitted, generation }, + ); } /// The write result arrived. `attempted == false` (preflight failure) is /// terminal — clear immediately (no reconcile fetch will follow). /// `attempted == true` advances to `AwaitingReconcile`: the write completed - /// server-side and a reconcile fetch is expected. + /// server-side and a reconcile fetch (matching generation) is expected. pub fn on_result(&mut self, room_id: &RoomId, attempted: bool) { if attempted { - if self.rooms.contains_key(room_id) { - self.rooms.insert(room_id.to_owned(), PendingAliasStage::AwaitingReconcile); + if let Some(entry) = self.rooms.get_mut(room_id) { + entry.stage = PendingAliasStage::AwaitingReconcile; } } else { self.rooms.remove(room_id); } } - /// A settings fetch for `room_id` landed. It clears the entry only from - /// `AwaitingReconcile` (the write already completed server-side, so the fetch - /// reflects it). While still `Submitted`, an open-fetch from a reopen must - /// not clear the pending write — its outcome is not yet known. - pub fn on_reconciled(&mut self, room_id: &RoomId) { - if self.rooms.get(room_id) == Some(&PendingAliasStage::AwaitingReconcile) { - self.rooms.remove(room_id); + /// A reconcile fetch of `generation` for `room_id` landed. It clears the entry + /// ONLY when the entry is `AwaitingReconcile` AND the generation matches — so + /// a stale/open fetch (wrong or absent generation) never clears a pending + /// write, independent of arrival timing. + pub fn on_reconciled(&mut self, room_id: &RoomId, generation: u64) { + if let Some(entry) = self.rooms.get(room_id) { + if entry.stage == PendingAliasStage::AwaitingReconcile + && entry.generation == generation + { + self.rooms.remove(room_id); + } } } @@ -338,11 +403,10 @@ impl PendingAliasWrites { self.rooms.contains_key(room_id) } - /// The in-flight stage for `room_id`, if any — so `show` can open the modal - /// in the matching locked state (reject open-fetches while `Submitted`, - /// accept the reconcile while `AwaitingReconcile`). - pub fn stage(&self, room_id: &RoomId) -> Option { - self.rooms.get(room_id).copied() + /// The in-flight `(stage, generation)` for `room_id`, if any — so `show` can + /// open the modal locked in the matching gate state with the right generation. + pub fn stage(&self, room_id: &RoomId) -> Option<(PendingAliasStage, u64)> { + self.rooms.get(room_id).map(|e| (e.stage, e.generation)) } } @@ -506,25 +570,26 @@ mod alias_logic_tests { fn test_alias_gate_blocks_overlapping_mutation() { let mut gate = AliasWriteGate::default(); assert!(gate.can_submit()); - assert!(gate.on_submit()); // first submit accepted + assert!(gate.on_submit(1)); // first submit accepted assert!(!gate.can_submit()); // controls now gated - assert!(!gate.on_submit()); // overlapping submit rejected - assert_eq!(gate, AliasWriteGate::AwaitingResult); + assert!(!gate.on_submit(2)); // overlapping submit rejected + assert_eq!(gate, AliasWriteGate::AwaitingResult(1)); } #[test] - fn test_alias_gate_attempted_result_holds_until_refresh() { - // Both success AND server-attempted failure hold the gate until this - // op's own reconcile fetch lands (a fetch is in flight for it). - for attempted in [true, true] { - let mut gate = AliasWriteGate::default(); - gate.on_submit(); - gate.on_result(attempted); // attempted → awaiting its own refresh - assert_eq!(gate, AliasWriteGate::AwaitingRefresh); - assert!(!gate.can_submit()); - gate.on_refresh(); // this op's authoritative refresh releases it - assert!(gate.can_submit()); - } + fn test_alias_gate_attempted_result_holds_until_matching_reconcile() { + // Success AND server-attempted failure hold the gate until this op's own + // reconcile fetch (matched by generation) lands. + let mut gate = AliasWriteGate::default(); + gate.on_submit(7); + gate.on_result(true); // attempted → awaiting its own refresh + assert_eq!(gate, AliasWriteGate::AwaitingRefresh(7)); + assert!(!gate.can_submit()); + assert_eq!( + gate.on_fetch(RoomSettingsFetchReason::AliasReconcile(7)), + FetchDisposition::ApplyAndRelease, + ); + assert!(gate.can_submit()); } #[test] @@ -532,7 +597,7 @@ mod alias_logic_tests { // A preflight failure (attempted == false: nothing sent, no fetch // spawned, state unchanged) releases straight to Idle. let mut gate = AliasWriteGate::default(); - gate.on_submit(); + gate.on_submit(3); gate.on_result(false); assert!(gate.can_submit()); assert_eq!(gate, AliasWriteGate::Idle); @@ -546,24 +611,102 @@ mod alias_logic_tests { } #[test] - fn test_alias_gate_stray_refresh_while_awaiting_result_does_not_release() { - // The op's own fetch is only spawned after its result, so a refresh - // arriving during AwaitingResult is a stray from a prior op: it must - // neither release the gate nor be accepted (would clobber optimism). + fn test_alias_gate_open_fetch_applies_only_when_idle() { + // An open-fetch loads state only when idle; during any in-flight write it + // is stale and must be ignored (never clobber, never release). + assert_eq!( + AliasWriteGate::Idle.disposition(RoomSettingsFetchReason::Open), + FetchDisposition::Apply, + ); + assert_eq!( + AliasWriteGate::AwaitingResult(1).disposition(RoomSettingsFetchReason::Open), + FetchDisposition::Ignore, + ); + assert_eq!( + AliasWriteGate::AwaitingRefresh(1).disposition(RoomSettingsFetchReason::Open), + FetchDisposition::Ignore, + ); + } + + #[test] + fn test_alias_gate_reconcile_requires_matching_generation() { + let mut gate = AliasWriteGate::AwaitingRefresh(2); + // Mismatched generation → ignored, gate unchanged. + assert_eq!( + gate.on_fetch(RoomSettingsFetchReason::AliasReconcile(3)), + FetchDisposition::Ignore, + ); + assert_eq!(gate, AliasWriteGate::AwaitingRefresh(2)); + // A reconcile arriving while still AwaitingResult is stale (result not + // back yet) → ignored. + assert_eq!( + AliasWriteGate::AwaitingResult(2).disposition(RoomSettingsFetchReason::AliasReconcile(2)), + FetchDisposition::Ignore, + ); + } + + #[test] + fn test_alias_gate_unavailable_releases_only_matching_reconcile() { + let mut gate = AliasWriteGate::AwaitingRefresh(4); + assert!(!gate.on_fetch_unavailable(RoomSettingsFetchReason::Open)); // open-fetch never releases + assert!(!gate.on_fetch_unavailable(RoomSettingsFetchReason::AliasReconcile(5))); // wrong gen + assert_eq!(gate, AliasWriteGate::AwaitingRefresh(4)); // still held + assert!(gate.on_fetch_unavailable(RoomSettingsFetchReason::AliasReconcile(4))); // matching → release + assert_eq!(gate, AliasWriteGate::Idle); + } + + // ── codex-named regression tests (generation/purpose correlation) ── + + #[test] + fn test_regression_two_open_fetches_stale_after_write_result() { + // Repro (a): W1 completes → AwaitingRefresh; a stale open-fetch (from a + // second open) returns AFTER the write result. It must NOT release the + // gate or apply its (pre-W1) state; only W1's own reconcile releases it. let mut gate = AliasWriteGate::default(); - gate.on_submit(); - assert_eq!(gate, AliasWriteGate::AwaitingResult); - assert!(!gate.should_accept_refresh()); // reject stray - gate.on_refresh(); - assert_eq!(gate, AliasWriteGate::AwaitingResult); - assert!(!gate.can_submit()); + gate.on_submit(1); // W1 + gate.on_result(true); // AwaitingRefresh(1) + assert_eq!( + gate.on_fetch(RoomSettingsFetchReason::Open), + FetchDisposition::Ignore, + ); + assert_eq!(gate, AliasWriteGate::AwaitingRefresh(1)); // still held, not clobbered + assert_eq!( + gate.on_fetch(RoomSettingsFetchReason::AliasReconcile(1)), + FetchDisposition::ApplyAndRelease, + ); + assert_eq!(gate, AliasWriteGate::Idle); } #[test] - fn test_alias_gate_accepts_refresh_when_idle_or_awaiting_refresh() { - assert!(AliasWriteGate::Idle.should_accept_refresh()); // open/initial fetch - assert!(AliasWriteGate::AwaitingRefresh.should_accept_refresh()); // own reconcile - assert!(!AliasWriteGate::AwaitingResult.should_accept_refresh()); // stray + fn test_regression_write_result_and_stale_fetch_same_batch_consistent() { + // Repro (b): a write result and a STALE fetch land in one Actions batch. + // App (registry) processes before UI (gate); both must reach the SAME + // decision for the stale fetch — registry and gate stay in lockstep — + // because both match on generation+purpose, independent of order. + let mut reg = PendingAliasWrites::default(); + let mut gate = AliasWriteGate::default(); + let r = room("!a:example.org"); + reg.register(r.clone(), 1); + gate.on_submit(1); + reg.on_result(&r, true); // AwaitingReconcile(gen 1) + gate.on_result(true); // AwaitingRefresh(1) + + // Stale fetch (older generation 0). App side: + reg.on_reconciled(&r, 0); + assert!(reg.is_pending(&r)); // registry NOT cleared (gen mismatch) + // UI side, same batch: + assert_eq!( + gate.on_fetch(RoomSettingsFetchReason::AliasReconcile(0)), + FetchDisposition::Ignore, + ); + assert_eq!(gate, AliasWriteGate::AwaitingRefresh(1)); // gate NOT released + // Consistent: both still in-flight. The real reconcile clears both. + reg.on_reconciled(&r, 1); + assert!(!reg.is_pending(&r)); + assert_eq!( + gate.on_fetch(RoomSettingsFetchReason::AliasReconcile(1)), + FetchDisposition::ApplyAndRelease, + ); } // ── PendingAliasWrites (app-level per-room registry, survives reopen) ── @@ -578,19 +721,21 @@ mod alias_logic_tests { // number of reads (a modal close/reopen just re-reads is_pending). let mut reg = PendingAliasWrites::default(); let r = room("!a:example.org"); - reg.register(r.clone()); + reg.register(r.clone(), 1); assert!(reg.is_pending(&r)); - assert!(reg.is_pending(&r)); // reopen consults it again → still pending + assert_eq!(reg.stage(&r), Some((PendingAliasStage::Submitted, 1))); } #[test] - fn test_pending_registry_cleared_on_reconcile() { + fn test_pending_registry_cleared_on_matching_reconcile() { let mut reg = PendingAliasWrites::default(); let r = room("!a:example.org"); - reg.register(r.clone()); + reg.register(r.clone(), 1); reg.on_result(&r, true); // attempted → still pending, awaiting reconcile assert!(reg.is_pending(&r)); - reg.on_reconciled(&r); + reg.on_reconciled(&r, 2); // mismatched generation → does NOT clear + assert!(reg.is_pending(&r)); + reg.on_reconciled(&r, 1); // matching → clears assert!(!reg.is_pending(&r)); } @@ -598,14 +743,15 @@ mod alias_logic_tests { fn test_pending_registry_open_fetch_does_not_clear_submitted() { // A settings fetch (e.g. an open-fetch from a reopen) that lands BEFORE // the write result must not clear a still-Submitted write — the outcome - // is unknown, so the room must stay locked. + // is unknown, so the room must stay locked. Even a matching generation is + // ignored while Submitted. let mut reg = PendingAliasWrites::default(); let r = room("!a:example.org"); - reg.register(r.clone()); // Submitted - reg.on_reconciled(&r); // stray/open fetch while Submitted + reg.register(r.clone(), 1); // Submitted + reg.on_reconciled(&r, 1); // fetch while Submitted (matching gen) assert!(reg.is_pending(&r)); // still pending reg.on_result(&r, true); // now AwaitingReconcile - reg.on_reconciled(&r); // the real reconcile clears it + reg.on_reconciled(&r, 1); // the real reconcile clears it assert!(!reg.is_pending(&r)); } @@ -613,7 +759,7 @@ mod alias_logic_tests { fn test_pending_registry_preflight_failure_clears_immediately() { let mut reg = PendingAliasWrites::default(); let r = room("!a:example.org"); - reg.register(r.clone()); + reg.register(r.clone(), 1); reg.on_result(&r, false); // preflight failure is terminal assert!(!reg.is_pending(&r)); } @@ -623,7 +769,7 @@ mod alias_logic_tests { let mut reg = PendingAliasWrites::default(); let a = room("!a:example.org"); let b = room("!b:example.org"); - reg.register(a.clone()); + reg.register(a.clone(), 1); assert!(reg.is_pending(&a)); assert!(!reg.is_pending(&b)); // other rooms unaffected } @@ -1413,11 +1559,13 @@ pub enum RoomSettingsAction { /// Publish a new (already-validated) alias and advertise it into /// `m.room.canonical_alias`'s `alt_aliases`. `canonical`/`alt_aliases` are /// the reconciled state to write (alt_aliases already includes `alias`). + /// `generation` is the modal-allocated write generation (see registry match). PublishAlias { room_id: OwnedRoomId, alias: OwnedRoomAliasId, canonical: Option, alt_aliases: Vec, + generation: u64, }, /// Promote an existing alias to canonical. `canonical`/`alt_aliases` are the /// reconciled target of the `m.room.canonical_alias` state event. @@ -1425,6 +1573,7 @@ pub enum RoomSettingsAction { room_id: OwnedRoomId, canonical: Option, alt_aliases: Vec, + generation: u64, }, /// Remove an alias: unbind it from the room directory and drop it from /// `m.room.canonical_alias` (reconciled `canonical`/`alt_aliases`). @@ -1433,6 +1582,7 @@ pub enum RoomSettingsAction { alias: OwnedRoomAliasId, canonical: Option, alt_aliases: Vec, + generation: u64, }, /// Change media visibility preference. SetMediaVisibility { room_id: OwnedRoomId, always_show: bool }, @@ -1474,6 +1624,10 @@ pub struct RoomSettingsModal { /// Serializes alias mutations: at most one write in flight per room. Gates /// the edit controls from submit until the operation fully settles. #[rust] alias_gate: AliasWriteGate, + /// Monotonic source of write generations. Each mutation takes the next value + /// so its reconcile fetch can be matched by generation (never confused with + /// an open-fetch or a stale reconcile). Persists across close/reopen. + #[rust] next_alias_generation: u64, /// The alias rows to render, in display order (canonical first). Drives the /// alias `PortalList` in `draw_walk`; rebuilt by `render_alias_section`. #[rust] alias_entries: Vec, @@ -1642,7 +1796,7 @@ impl RoomSettingsModal { room_name: &str, room_topic: &str, canonical_alias: Option<&str>, - alias_stage: Option, + alias_stage: Option<(PendingAliasStage, u64)>, ) { let room_id_text = room_id.as_str().to_string(); self.room_id = Some(room_id); @@ -1676,15 +1830,21 @@ impl RoomSettingsModal { self.can_manage_aliases = false; self.alias_snapshot = None; // P1-2: if a write for this room is still settling (registry), open locked - // in the matching gate state so a close→reopen can't re-enable controls - // mid-flight. `Submitted` → AwaitingResult (reject an unrelated open-fetch - // until the write result returns); `AwaitingReconcile` → AwaitingRefresh - // (the op's reconcile fetch unlocks it). No pending write → Idle. + // in the matching gate state — carrying the SAME generation — so a + // close→reopen can't re-enable controls mid-flight and only that write's + // own reconcile (matching generation) unlocks it. `Submitted` → + // AwaitingResult (reject an unrelated open-fetch until the result + // returns); `AwaitingReconcile` → AwaitingRefresh. No pending write → Idle. self.alias_gate = match alias_stage { - Some(PendingAliasStage::Submitted) => AliasWriteGate::AwaitingResult, - Some(PendingAliasStage::AwaitingReconcile) => AliasWriteGate::AwaitingRefresh, + Some((PendingAliasStage::Submitted, generation)) => AliasWriteGate::AwaitingResult(generation), + Some((PendingAliasStage::AwaitingReconcile, generation)) => AliasWriteGate::AwaitingRefresh(generation), None => AliasWriteGate::Idle, }; + // Keep the local generation source ahead of any adopted pending write so + // the next new mutation can't collide with it. + if let Some((_, generation)) = alias_stage { + self.next_alias_generation = self.next_alias_generation.max(generation); + } self.render_alias_section(cx); // Avatar fallback text (first char of name) @@ -1750,6 +1910,7 @@ impl RoomSettingsModal { &mut self, cx: &mut Cx, room_id: &RoomId, + reason: RoomSettingsFetchReason, language: AppLanguage, canonical_alias: Option, alt_aliases: Vec, @@ -1758,14 +1919,15 @@ impl RoomSettingsModal { if !self.is_current_room(room_id) { return; } - // P1-1: route the gate decision BEFORE touching state. A refresh arriving - // while a mutation's result is still pending (AwaitingResult) is a stray - // fetch from a prior op — reject it so it can't clobber optimistic state - // or the rollback snapshot. Otherwise accept and advance the gate. - if !self.alias_gate.should_accept_refresh() { + // Route the gate decision BEFORE touching state, matched by generation + + // purpose (P1). `Ignore` → a stale open-fetch or mismatched reconcile: + // drop it (never clobber optimistic state / snapshot). `ApplyAndRelease` + // → this write's own reconcile: apply + release. `Apply` → an open-fetch + // while idle: load without releasing anything. + let disposition = self.alias_gate.on_fetch(reason); + if disposition == FetchDisposition::Ignore { return; } - self.alias_gate.on_refresh(); // Store authoritative state; a fresh fetch supersedes optimism. self.language = language; self.current_canonical = canonical_alias; @@ -1779,13 +1941,15 @@ impl RoomSettingsModal { /// produce data (no client / room unavailable). Unlike `apply_alias_settings` /// this does NOT overwrite state — it keeps the current (optimistic) aliases /// and just re-enables the controls, so the gate can never strand disabled. - /// Guarded like a refresh: a stray release during `AwaitingResult` is ignored. - pub fn release_alias_lock(&mut self, cx: &mut Cx, room_id: &RoomId) { - if !self.is_current_room(room_id) || !self.alias_gate.should_accept_refresh() { + /// Releases ONLY on the matching reconcile (generation + purpose); an + /// open-fetch or mismatched reconcile is a no-op. + pub fn release_alias_lock(&mut self, cx: &mut Cx, room_id: &RoomId, reason: RoomSettingsFetchReason) { + if !self.is_current_room(room_id) { return; } - self.alias_gate.on_refresh(); - self.render_alias_section(cx); + if self.alias_gate.on_fetch_unavailable(reason) { + self.render_alias_section(cx); + } } /// Render the whole alias section (labels, per-row list, gating) from the @@ -1903,7 +2067,8 @@ impl RoomSettingsModal { ); self.snapshot_alias_state(); self.current_alts = new_alts.clone(); - self.alias_gate.on_submit(); + let generation = self.take_alias_generation(); + self.alias_gate.on_submit(generation); self.render_alias_section(cx); self.view.text_input(cx, ids!(add_address_input)).set_text(cx, ""); @@ -1912,9 +2077,16 @@ impl RoomSettingsModal { alias: valid_alias, canonical: self.current_canonical.clone(), alt_aliases: new_alts, + generation, }); } + /// Allocate the next monotonic write generation for a new mutation. + fn take_alias_generation(&mut self) -> u64 { + self.next_alias_generation = self.next_alias_generation.wrapping_add(1); + self.next_alias_generation + } + /// Promote `alias` to canonical: reconcile, optimistically update, emit. fn set_canonical_alias(&mut self, cx: &mut Cx, alias: OwnedRoomAliasId) { use crate::shared::popup_list::{PopupKind, enqueue_popup_notification}; @@ -1932,12 +2104,14 @@ impl RoomSettingsModal { self.snapshot_alias_state(); self.current_canonical = state.canonical.clone(); self.current_alts = state.alt_aliases.clone(); - self.alias_gate.on_submit(); + let generation = self.take_alias_generation(); + self.alias_gate.on_submit(generation); self.render_alias_section(cx); cx.action(RoomSettingsAction::SetCanonicalAlias { room_id, canonical: state.canonical, alt_aliases: state.alt_aliases, + generation, }); } Err(CanonicalReconcileError::NotPublished) => { @@ -1975,13 +2149,15 @@ impl RoomSettingsModal { self.snapshot_alias_state(); self.current_canonical = state.canonical.clone(); self.current_alts = state.alt_aliases.clone(); - self.alias_gate.on_submit(); + let generation = self.take_alias_generation(); + self.alias_gate.on_submit(generation); self.render_alias_section(cx); cx.action(RoomSettingsAction::RemoveAlias { room_id, alias, canonical: state.canonical, alt_aliases: state.alt_aliases, + generation, }); } } @@ -2042,7 +2218,7 @@ impl RoomSettingsModalRef { room_name: &str, room_topic: &str, canonical_alias: Option<&str>, - alias_stage: Option, + alias_stage: Option<(PendingAliasStage, u64)>, ) { let Some(mut inner) = self.borrow_mut() else { return }; inner.show(cx, room_id, room_name, room_topic, canonical_alias, alias_stage); @@ -2067,19 +2243,20 @@ impl RoomSettingsModalRef { &self, cx: &mut Cx, room_id: &RoomId, + reason: RoomSettingsFetchReason, language: AppLanguage, canonical_alias: Option, alt_aliases: Vec, can_manage: bool, ) { let Some(mut inner) = self.borrow_mut() else { return }; - inner.apply_alias_settings(cx, room_id, language, canonical_alias, alt_aliases, can_manage); + inner.apply_alias_settings(cx, room_id, reason, language, canonical_alias, alt_aliases, can_manage); } /// Release a stranded alias gate when its reconcile fetch was unavailable. - pub fn release_alias_lock(&self, cx: &mut Cx, room_id: &RoomId) { + pub fn release_alias_lock(&self, cx: &mut Cx, room_id: &RoomId, reason: RoomSettingsFetchReason) { let Some(mut inner) = self.borrow_mut() else { return }; - inner.release_alias_lock(cx, room_id); + inner.release_alias_lock(cx, room_id, reason); } /// Update the avatar widget after a successful upload. diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index 9d4746fef..6b5f7d3ee 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -701,6 +701,9 @@ pub type OnLinkPreviewFetchedFn = fn( #[derive(Clone, Debug)] pub struct RoomSettingsFetchedAction { pub room_id: OwnedRoomId, + /// Why this fetch was issued — echoed from the request so a stale or + /// unrelated fetch can never be mistaken for a specific write's reconcile. + pub reason: crate::home::room_settings_modal::RoomSettingsFetchReason, pub topic: Option, pub is_public: bool, /// The room's canonical (main) alias, if any. @@ -719,6 +722,9 @@ pub struct RoomSettingsFetchedAction { #[derive(Clone, Debug)] pub struct RoomSettingsFetchUnavailableAction { pub room_id: OwnedRoomId, + /// Why the fetch was issued (echoed from the request) — release only fires + /// on the matching write reconcile, never an open-fetch. + pub reason: crate::home::room_settings_modal::RoomSettingsFetchReason, } /// Posted after a room avatar is successfully uploaded and set. @@ -1687,9 +1693,13 @@ pub enum MatrixRequest { update_sender: Option>, }, /// Fetch room-specific settings: topic and whether the room is public. - /// Response arrives as a [`RoomSettingsFetchedAction`]. + /// Response arrives as a [`RoomSettingsFetchedAction`] (or a + /// [`RoomSettingsFetchUnavailableAction`] if the room can't be read). Both + /// echo `reason` so the alias write gate/registry only accept the reconcile + /// that matches the write in flight. FetchRoomSettings { room_id: OwnedRoomId, + reason: crate::home::room_settings_modal::RoomSettingsFetchReason, }, /// Publish a new alias into the room directory, mapping it to this room /// (`PUT /directory/room/{alias}`), then — only on success — advertise it @@ -4405,12 +4415,13 @@ async fn matrix_worker_task( }); } - MatrixRequest::FetchRoomSettings { room_id } => { + MatrixRequest::FetchRoomSettings { room_id, reason } => { // No client → still signal "unavailable" so a waiting alias write // releases its gate/registry instead of stranding (never silently - // drop this fetch — the alias reconcile depends on a reply). + // drop this fetch — the alias reconcile depends on a reply). The + // `reason` is echoed so only the matching write reconcile acts. let Some(client) = get_client() else { - Cx::post_action(RoomSettingsFetchUnavailableAction { room_id }); + Cx::post_action(RoomSettingsFetchUnavailableAction { room_id, reason }); continue; }; let _fetch_room_settings_task = Handle::current().spawn(async move { @@ -4429,6 +4440,7 @@ async fn matrix_worker_task( }; Cx::post_action(RoomSettingsFetchedAction { room_id, + reason, topic, is_public, canonical_alias, @@ -4437,7 +4449,7 @@ async fn matrix_worker_task( }); } else { // Room not currently available — release any waiting gate. - Cx::post_action(RoomSettingsFetchUnavailableAction { room_id }); + Cx::post_action(RoomSettingsFetchUnavailableAction { room_id, reason }); } }); } From bbeaf5b0beb8d92e7dd48709195b81e6218b20ac Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Sun, 26 Jul 2026 14:00:57 +0800 Subject: [PATCH 08/11] fix(room-aliases): make settings-fetch freshness sound (epoch-tag Opens + server-truth reconciles) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex found two freshness holes in the generation-tagged fetches (msg_0237), both letting a stale fetch clobber a completed write. P1-1 — Open responses need freshness too. `(Idle, Open) → Apply` re-opened the door: after a write's reconcile released to Idle, an older Open (snapshot taken pre-write) returning last was applied and repainted pre-write state, letting the user submit a second write from it. Every fetch now carries a monotonic epoch (new `OpenFreshness`): an Open applies only if it is still the newest, and BOTH a new write AND any accepted apply push `min_acceptable` past every epoch issued so far — so an older Open can never become acceptable again, even after the gate returns to Idle. Same guard covers the Unavailable-release path (the write's submit already invalidates earlier Opens). P1-2 — reconcile must carry SERVER truth, not local cache. In the pinned matrix-sdk `send_state_event` does not update local RoomInfo, so `room.canonical_alias()/alt_aliases()` can still return the pre-write value; the handler also captured alias data before its power-level await. A reconcile now reads `m.room.canonical_alias` directly from the server (ruma `get_state_event_for_key` via `client.send`) at post time; if that fresh read is unavailable it releases via `RoomSettingsFetchUnavailableAction` instead of applying stale cache data (`reconcile_fetch_outcome`). Opens keep the cache read (epoch-guarded, no extra round-trip). Adds codex-named regressions: write-then-old-Open rejected in BOTH the Fetched and Unavailable variants; reconcile applies server truth and releases-without- applying when the fresh read is unavailable; plus OpenFreshness unit tests. cargo test --lib: 575 passed / 0 failed. agent-spec lint: 100%. Co-Authored-By: Claude Fable 5 --- src/app.rs | 7 +- src/home/room_settings_modal.rs | 228 ++++++++++++++++++++++++++++---- src/sliding_sync.rs | 74 ++++++++++- 3 files changed, 281 insertions(+), 28 deletions(-) diff --git a/src/app.rs b/src/app.rs index c96bbd67a..f4c79a5d0 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1934,14 +1934,17 @@ impl MatchEvent for App { // an alias write still settling. let alias_stage = self.pending_alias_writes.stage(&room_id); log!("RoomSettingsAction::Open for {} (name: {})", room_id, room_name); - self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) + // `show_settings` returns the freshness epoch to tag this + // open-fetch with, so a slow earlier open can't repaint over a + // newer one or over a post-write reconcile (P1-1). + let open_epoch = self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) .show_settings(cx, room_id.clone(), &room_name, "", alias_str, alias_stage); self.ui.modal(cx, ids!(room_settings_modal)).open(cx); // Open-fetch: not tied to any write, so it can never consume a // write reconcile (purpose = Open). submit_async_request(MatrixRequest::FetchRoomSettings { room_id, - reason: RoomSettingsFetchReason::Open, + reason: RoomSettingsFetchReason::Open(open_epoch), }); continue; } diff --git a/src/home/room_settings_modal.rs b/src/home/room_settings_modal.rs index c523d925e..114cc2dae 100644 --- a/src/home/room_settings_modal.rs +++ b/src/home/room_settings_modal.rs @@ -202,14 +202,67 @@ pub fn next_step_after_directory_write(directory_ok: bool) -> SequencedAliasStep /// flagged: settings fetches previously carried only `room_id`). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum RoomSettingsFetchReason { - /// Opened the modal (or another non-write refresh). Not tied to any alias - /// write, so it must NEVER consume a write's reconcile gate/registry entry. - Open, + /// Opened the modal (or another non-write refresh), carrying a monotonic + /// epoch. Not tied to any alias write, so it must NEVER consume a write's + /// reconcile. Its data is applied only when it is the *newest* Open and no + /// write has been submitted since it was issued (see [`OpenFreshness`]) — so + /// a slow pre-write Open can't repaint stale state after a write reconciles. + Open(u64), /// The authoritative reconcile for the alias write of this generation. Only a /// matching generation may release that write's gate / clear its registry. AliasReconcile(u64), } +/// Freshness guard for `Open` settings fetches (P1-1). An `Open` snapshots +/// server state at request time, so a slow one issued *before* a write can +/// return *after* that write reconciles and repaint pre-write state — letting +/// the user submit a second write from stale data and clobber the first. +/// +/// Every request takes a monotonic epoch; an `Open` is applied only if its epoch +/// is still `>= min_acceptable`. Both a new write AND any accepted apply push +/// `min_acceptable` past every epoch issued so far, so an older `Open` can never +/// become acceptable again — even after the gate returns to `Idle`. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub struct OpenFreshness { + next_epoch: u64, + min_acceptable: u64, +} + +impl OpenFreshness { + /// Allocate the epoch for a new `Open` fetch. This `Open` (and any newer one) + /// is acceptable; any earlier outstanding `Open` is now stale. + pub fn take_open(&mut self) -> u64 { + self.next_epoch = self.next_epoch.wrapping_add(1); + self.min_acceptable = self.next_epoch; + self.next_epoch + } + + /// Allocate the generation for a new write. A write permanently invalidates + /// every `Open` issued so far (only a *future* Open can be accepted). + pub fn take_write(&mut self) -> u64 { + self.next_epoch = self.next_epoch.wrapping_add(1); + self.min_acceptable = self.next_epoch.wrapping_add(1); + self.next_epoch + } + + /// An authoritative apply landed (an accepted Open or reconcile). Invalidate + /// every `Open` issued so far so none can repaint over the just-applied state. + pub fn on_apply(&mut self) { + self.min_acceptable = self.next_epoch.wrapping_add(1); + } + + /// Whether an `Open` of `epoch` is still fresh enough to apply. + pub fn accepts_open(&self, epoch: u64) -> bool { + epoch >= self.min_acceptable + } + + /// Ensure the epoch source is at least `value` (keeps it monotonic when the + /// modal adopts a pending write's generation on reopen). + pub fn observe(&mut self, value: u64) { + self.next_epoch = self.next_epoch.max(value); + } +} + /// Serializes alias mutations for the modal's room: at most one may be in /// flight at a time. Overlapping mutations are the cross-operation race that /// can resurrect an unbound alias — each write snapshots the *full* @@ -289,7 +342,9 @@ impl AliasWriteGate { /// `AwaitingRefresh(g)` with the same generation; otherwise `Ignore`. pub fn disposition(self, reason: RoomSettingsFetchReason) -> FetchDisposition { match (self, reason) { - (AliasWriteGate::Idle, RoomSettingsFetchReason::Open) => FetchDisposition::Apply, + // Open only *gates* on Idle here; its epoch freshness is enforced by + // the caller via `OpenFreshness` (P1-1). + (AliasWriteGate::Idle, RoomSettingsFetchReason::Open(_)) => FetchDisposition::Apply, (AliasWriteGate::AwaitingRefresh(g), RoomSettingsFetchReason::AliasReconcile(r)) if g == r => { @@ -431,6 +486,37 @@ pub fn remove_alias_treat_as_success(directory_not_found: bool) -> bool { directory_not_found } +/// What a reconcile fetch should post, given its **server-fresh** read of +/// `m.room.canonical_alias` (P1-2). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ReconcileFetchOutcome { + /// A server-fresh read succeeded (`None` inner = no canonical alias set) — + /// post it as the authoritative fetched settings. + Fetched { + canonical: Option, + alt_aliases: Vec, + }, + /// The server-fresh read was unavailable — release the gate WITHOUT applying + /// any (possibly stale) data. + Unavailable, +} + +/// Decide what a reconcile fetch posts from the result of its server-fresh +/// canonical-alias read. In the pinned matrix-sdk, `send_state_event` does not +/// update the local `RoomInfo`, so the cache-backed `room.canonical_alias()` can +/// still return the *pre-write* value after a successful write; a reconcile must +/// therefore read the server directly and, if that read fails, release via +/// `Unavailable` rather than apply stale cache data. `server_read == None` means +/// the fresh read could not be obtained. +pub fn reconcile_fetch_outcome( + server_read: Option<(Option, Vec)>, +) -> ReconcileFetchOutcome { + match server_read { + Some((canonical, alt_aliases)) => ReconcileFetchOutcome::Fetched { canonical, alt_aliases }, + None => ReconcileFetchOutcome::Unavailable, + } +} + #[cfg(test)] mod alias_logic_tests { use super::*; @@ -615,15 +701,15 @@ mod alias_logic_tests { // An open-fetch loads state only when idle; during any in-flight write it // is stale and must be ignored (never clobber, never release). assert_eq!( - AliasWriteGate::Idle.disposition(RoomSettingsFetchReason::Open), + AliasWriteGate::Idle.disposition(RoomSettingsFetchReason::Open(1)), FetchDisposition::Apply, ); assert_eq!( - AliasWriteGate::AwaitingResult(1).disposition(RoomSettingsFetchReason::Open), + AliasWriteGate::AwaitingResult(1).disposition(RoomSettingsFetchReason::Open(1)), FetchDisposition::Ignore, ); assert_eq!( - AliasWriteGate::AwaitingRefresh(1).disposition(RoomSettingsFetchReason::Open), + AliasWriteGate::AwaitingRefresh(1).disposition(RoomSettingsFetchReason::Open(1)), FetchDisposition::Ignore, ); } @@ -648,7 +734,7 @@ mod alias_logic_tests { #[test] fn test_alias_gate_unavailable_releases_only_matching_reconcile() { let mut gate = AliasWriteGate::AwaitingRefresh(4); - assert!(!gate.on_fetch_unavailable(RoomSettingsFetchReason::Open)); // open-fetch never releases + assert!(!gate.on_fetch_unavailable(RoomSettingsFetchReason::Open(1))); // open-fetch never releases assert!(!gate.on_fetch_unavailable(RoomSettingsFetchReason::AliasReconcile(5))); // wrong gen assert_eq!(gate, AliasWriteGate::AwaitingRefresh(4)); // still held assert!(gate.on_fetch_unavailable(RoomSettingsFetchReason::AliasReconcile(4))); // matching → release @@ -666,7 +752,7 @@ mod alias_logic_tests { gate.on_submit(1); // W1 gate.on_result(true); // AwaitingRefresh(1) assert_eq!( - gate.on_fetch(RoomSettingsFetchReason::Open), + gate.on_fetch(RoomSettingsFetchReason::Open(1)), FetchDisposition::Ignore, ); assert_eq!(gate, AliasWriteGate::AwaitingRefresh(1)); // still held, not clobbered @@ -789,6 +875,78 @@ mod alias_logic_tests { assert!(remove_alias_treat_as_success(true)); // already gone → success (de-advertise) assert!(!remove_alias_treat_as_success(false)); // other error → real fail } + + // ── P1-1: Open-fetch freshness (OpenFreshness) ── + + #[test] + fn test_open_freshness_newest_open_wins() { + // An older Open is stale the moment a newer Open is issued. + let mut f = OpenFreshness::default(); + let e1 = f.take_open(); + let e2 = f.take_open(); + assert!(!f.accepts_open(e1)); // superseded + assert!(f.accepts_open(e2)); // newest + } + + #[test] + fn test_regression_write_then_old_fetched_open_rejected() { + // codex-named (a), Fetched variant: F1 issued (pre-write), W1 submitted, + // W1's reconcile applies; the OLD F1 returning last must be rejected so it + // can't repaint pre-write state (which would let W2 clobber W1). + let mut f = OpenFreshness::default(); + let e_f1 = f.take_open(); // slow open, snapshots S0 + let _g_w1 = f.take_write(); // write — invalidates all earlier opens + f.on_apply(); // W1's reconcile applies authoritative S1 + assert!(!f.accepts_open(e_f1)); // old F1 can never repaint S0 + } + + #[test] + fn test_regression_write_then_old_open_rejected_via_unavailable_release() { + // codex-named (a), Unavailable variant: the write's reconcile came back + // Unavailable (released without applying), but the OLD F1 must STILL be + // rejected — the write's submit already invalidated it, independent of + // whether the reconcile applied or released. + let mut f = OpenFreshness::default(); + let e_f1 = f.take_open(); + let _g_w1 = f.take_write(); // submit invalidates earlier opens (no on_apply needed) + assert!(!f.accepts_open(e_f1)); + } + + #[test] + fn test_open_freshness_apply_invalidates_outstanding_opens() { + // After an accepted apply, an Open issued *before* it is stale even though + // no new write happened (a duplicate/older open must not repaint). + let mut f = OpenFreshness::default(); + let e_old = f.take_open(); + let _e_new = f.take_open(); // a newer open we accept and apply + f.on_apply(); + assert!(!f.accepts_open(e_old)); + } + + // ── P1-2: reconcile must carry server truth, not stale cache ── + + #[test] + fn test_reconcile_outcome_applies_server_read() { + let out = reconcile_fetch_outcome(Some(( + Some(alias("#main:example.org")), + vec![alias("#alt:example.org")], + ))); + assert_eq!( + out, + ReconcileFetchOutcome::Fetched { + canonical: Some(alias("#main:example.org")), + alt_aliases: vec![alias("#alt:example.org")], + }, + ); + } + + #[test] + fn test_reconcile_outcome_releases_without_applying_when_read_unavailable() { + // "send succeeded but local RoomInfo not yet synced": the reconcile does a + // server-fresh read; if that read is unavailable it must NOT apply stale + // cache data — it releases via Unavailable instead. + assert_eq!(reconcile_fetch_outcome(None), ReconcileFetchOutcome::Unavailable); + } } // ───────────────────────────────────────────────────────────────────────────── @@ -1624,10 +1782,12 @@ pub struct RoomSettingsModal { /// Serializes alias mutations: at most one write in flight per room. Gates /// the edit controls from submit until the operation fully settles. #[rust] alias_gate: AliasWriteGate, - /// Monotonic source of write generations. Each mutation takes the next value - /// so its reconcile fetch can be matched by generation (never confused with - /// an open-fetch or a stale reconcile). Persists across close/reopen. - #[rust] next_alias_generation: u64, + /// Monotonic source of request epochs (write generations AND open-fetch + /// epochs) plus the `Open`-freshness threshold. Each mutation takes the next + /// value so its reconcile is matched by generation; each open takes one so a + /// stale pre-write open can't repaint after a write reconciles (P1-1). + /// Persists across close/reopen. + #[rust] open_freshness: OpenFreshness, /// The alias rows to render, in display order (canonical first). Drives the /// alias `PortalList` in `draw_walk`; rebuilt by `render_alias_section`. #[rust] alias_entries: Vec, @@ -1789,6 +1949,8 @@ impl RoomSettingsModal { /// this room has an alias write still settling (submitted in a prior modal /// session), so the section opens locked in the matching gate state until /// that op's reconcile fetch lands. + /// Returns the epoch to tag this room's open-fetch with, so a stale earlier + /// open (from a prior show) can't repaint over it (P1-1). pub fn show( &mut self, cx: &mut Cx, @@ -1797,7 +1959,7 @@ impl RoomSettingsModal { room_topic: &str, canonical_alias: Option<&str>, alias_stage: Option<(PendingAliasStage, u64)>, - ) { + ) -> u64 { let room_id_text = room_id.as_str().to_string(); self.room_id = Some(room_id); self.original_name = room_name.to_string(); @@ -1840,11 +2002,14 @@ impl RoomSettingsModal { Some((PendingAliasStage::AwaitingReconcile, generation)) => AliasWriteGate::AwaitingRefresh(generation), None => AliasWriteGate::Idle, }; - // Keep the local generation source ahead of any adopted pending write so - // the next new mutation can't collide with it. + // Keep the epoch source ahead of any adopted pending write so the next + // new mutation / open can't collide with it. if let Some((_, generation)) = alias_stage { - self.next_alias_generation = self.next_alias_generation.max(generation); + self.open_freshness.observe(generation); } + // Allocate this open's epoch AFTER adopting the pending generation, so it + // is strictly newer; it becomes the only acceptable open (P1-1). + let open_epoch = self.open_freshness.take_open(); self.render_alias_section(cx); // Avatar fallback text (first char of name) @@ -1857,6 +2022,7 @@ impl RoomSettingsModal { self.view.label(cx, ids!(name_error_label)).set_text(cx, ""); self.view.redraw(cx); + open_epoch } /// Update the avatar widget with freshly uploaded image bytes. @@ -1928,6 +2094,18 @@ impl RoomSettingsModal { if disposition == FetchDisposition::Ignore { return; } + // P1-1: an accepted Open must still be the *freshest*. Reject a slow + // pre-write open that would otherwise repaint stale state after a write + // reconciled (and let the user submit a second write from it). The gate + // was not mutated for an Open, so returning here leaves it Idle. + if let RoomSettingsFetchReason::Open(epoch) = reason { + if !self.open_freshness.accepts_open(epoch) { + return; + } + } + // Accepted authoritative apply — invalidate every earlier open so none + // can repaint over the state we're about to store (P1-1). + self.open_freshness.on_apply(); // Store authoritative state; a fresh fetch supersedes optimism. self.language = language; self.current_canonical = canonical_alias; @@ -2081,10 +2259,11 @@ impl RoomSettingsModal { }); } - /// Allocate the next monotonic write generation for a new mutation. + /// Allocate the next monotonic write generation for a new mutation. This also + /// invalidates every `Open` fetch issued so far (P1-1), so a slow pre-write + /// open can't repaint stale state after this write reconciles. fn take_alias_generation(&mut self) -> u64 { - self.next_alias_generation = self.next_alias_generation.wrapping_add(1); - self.next_alias_generation + self.open_freshness.take_write() } /// Promote `alias` to canonical: reconcile, optimistically update, emit. @@ -2210,7 +2389,8 @@ impl RoomSettingsModal { } impl RoomSettingsModalRef { - /// Populate the modal with room data and prepare for display. + /// Populate the modal with room data and prepare for display. Returns the + /// epoch to tag this room's open-fetch with (P1-1); `0` if the ref is empty. pub fn show_settings( &self, cx: &mut Cx, @@ -2219,9 +2399,9 @@ impl RoomSettingsModalRef { room_topic: &str, canonical_alias: Option<&str>, alias_stage: Option<(PendingAliasStage, u64)>, - ) { - let Some(mut inner) = self.borrow_mut() else { return }; - inner.show(cx, room_id, room_name, room_topic, canonical_alias, alias_stage); + ) -> u64 { + let Some(mut inner) = self.borrow_mut() else { return 0 }; + inner.show(cx, room_id, room_name, room_topic, canonical_alias, alias_stage) } /// Apply asynchronously-fetched settings (topic, is_public). Dropped if the diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index 6b5f7d3ee..222b5fc1b 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -776,6 +776,52 @@ async fn alias_maps_to_room(client: &Client, alias: &RoomAliasId, room_id: &Room matches!(client.send(request).await, Ok(resp) if resp.room_id.as_str() == room_id.as_str()) } +/// Read `m.room.canonical_alias` DIRECTLY FROM THE SERVER (P1-2). +/// +/// A reconcile fetch must NOT use `room.canonical_alias()` / `room.alt_aliases()`: +/// in the pinned matrix-sdk `send_state_event` does not update the local +/// `RoomInfo`, so those accessors can still return the pre-write value after a +/// successful write until a later sync. This issues a direct `GET +/// /state/m.room.canonical_alias` via `client.send`, which reflects the write. +/// +/// Returns `Some((canonical, alt_aliases))` on a successful read (including a +/// 404 → no canonical alias set → `(None, [])`), or `None` if the fresh read +/// could not be obtained (caller then releases the gate without applying stale +/// data). See [`reconcile_fetch_outcome`](crate::home::room_settings_modal::reconcile_fetch_outcome). +async fn fetch_canonical_alias_from_server( + client: &Client, + room_id: &RoomId, +) -> Option<(Option, Vec)> { + use matrix_sdk::ruma::api::client::state::get_state_event_for_key; + use matrix_sdk::ruma::events::room::canonical_alias::RoomCanonicalAliasEventContent; + // Default format returns just the event content as raw JSON. + let request = get_state_event_for_key::v3::Request::new( + room_id.to_owned(), + StateEventType::RoomCanonicalAlias, + String::new(), + ); + match client.send(request).await { + Ok(resp) => match serde_json::from_str::( + resp.event_or_content.get(), + ) { + Ok(content) => Some((content.alias, content.alt_aliases)), + Err(e) => { + error!("Failed to parse server canonical_alias for {room_id}: {e:?}"); + None + } + }, + Err(e) => { + if alias_error_status(&e) == Some(404) { + // No canonical_alias state event → a valid, fresh "empty" read. + Some((None, Vec::new())) + } else { + error!("Failed to read server canonical_alias for {room_id}: {e:?}"); + None + } + } + } +} + /// Send the room's `m.room.canonical_alias` state event (`alias` + `alt_aliases`). /// /// Shared by the sequenced publish/remove flows (as their second, gated step) @@ -4425,11 +4471,12 @@ async fn matrix_worker_task( continue; }; let _fetch_room_settings_task = Handle::current().spawn(async move { + use crate::home::room_settings_modal::{ + reconcile_fetch_outcome, ReconcileFetchOutcome, RoomSettingsFetchReason, + }; if let Some(room) = client.get_room(&room_id) { let topic = room.topic(); let is_public = room.is_public().unwrap_or(false); - let canonical_alias = room.canonical_alias(); - let alt_aliases = room.alt_aliases(); // Alias edit controls are gated on the power level to send // `m.room.canonical_alias` (see `UserPowerLevels`). let can_manage_aliases = match client.user_id() { @@ -4438,6 +4485,29 @@ async fn matrix_worker_task( .is_some_and(|pl| pl.can_set_canonical_alias()), None => false, }; + // Capture alias data AFTER the power-level await, at post + // time (P1-2). A reconcile reads SERVER truth (the local + // cache lags `send_state_event`); if that fresh read fails + // it releases via Unavailable rather than apply stale data. + // An open uses the cache (epoch-guarded, no extra round-trip). + let (canonical_alias, alt_aliases) = match reason { + RoomSettingsFetchReason::AliasReconcile(_) => { + match reconcile_fetch_outcome( + fetch_canonical_alias_from_server(&client, &room_id).await, + ) { + ReconcileFetchOutcome::Fetched { canonical, alt_aliases } => { + (canonical, alt_aliases) + } + ReconcileFetchOutcome::Unavailable => { + Cx::post_action(RoomSettingsFetchUnavailableAction { room_id, reason }); + return; + } + } + } + RoomSettingsFetchReason::Open(_) => { + (room.canonical_alias(), room.alt_aliases()) + } + }; Cx::post_action(RoomSettingsFetchedAction { room_id, reason, From f2fa2f784277724003ed22878703005259c012ed Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Sun, 26 Jul 2026 14:48:15 +0800 Subject: [PATCH 09/11] fix(room-aliases): make matching-Unavailable a terminal freshness barrier + sound reopen recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex found one uncovered interleaving in the freshness model (msg_0246): a reopen DURING a pending write issues Open(e2) with e2 > min_acceptable. If the write's reconcile hits a transient failure (matching Unavailable), release_alias_lock() only set the gate Idle — it did NOT advance OpenFreshness. Open(e2) then applied a possibly pre-write cache snapshot (+ can_manage), letting a second write clobber the first (which had succeeded server-side). The opposite order left the modal wedged read-only. Fixes (codex's required closure): 1. Terminal barrier: a matching-Unavailable reconcile now invalidates every Open issued during that pending generation via OpenFreshness::on_apply (exactly as a successful reconcile does), so a late reopen Open can never apply. 2. Safe recovery: release_alias_lock returns a fresh post-barrier epoch; app.rs issues a new Recovery(epoch) fetch that reads SERVER truth (not the cache) so the reopened modal repopulates state + permission via a fetch postdating the invalidation — never a pre-write snapshot, and never wedged read-only. A new `Recovery` fetch reason behaves like Open in the gate (applies when idle, epoch-guarded) but reads the server like a reconcile; a failed Recovery does not spawn another (bounded). Plain Opens keep the fast cache read — a stale reopen Open is rejected by the barrier, so it never applies. Adds codex-named pure interleaving tests, both orders: matching-Unavailable then post-write Open (rejected); post-write Open then matching-Unavailable (no stale apply AND the post-barrier recovery repopulates, not wedged). cargo test --lib: 577 passed / 0 failed. agent-spec lint: 100%. Co-Authored-By: Claude Fable 5 --- src/app.rs | 13 +++- src/home/room_settings_modal.rs | 121 ++++++++++++++++++++++++++------ src/sliding_sync.rs | 13 ++-- 3 files changed, 121 insertions(+), 26 deletions(-) diff --git a/src/app.rs b/src/app.rs index f4c79a5d0..f85a27443 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2085,8 +2085,19 @@ impl MatchEvent for App { if let RoomSettingsFetchReason::AliasReconcile(generation) = unavailable.reason { self.pending_alias_writes.on_reconciled(&unavailable.room_id, generation); } - self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) + // A matching-reconcile Unavailable is a terminal freshness barrier + // (round 7): it invalidates the reopen's own Open, so the modal + // hands back a fresh post-barrier Open epoch to repopulate via a + // (server-fresh) fetch that postdates the invalidation — never a + // pre-write cache snapshot, and never wedged read-only. + let recovery = self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) .release_alias_lock(cx, &unavailable.room_id, unavailable.reason); + if let Some(recovery_epoch) = recovery { + submit_async_request(MatrixRequest::FetchRoomSettings { + room_id: unavailable.room_id.clone(), + reason: RoomSettingsFetchReason::Recovery(recovery_epoch), + }); + } continue; } diff --git a/src/home/room_settings_modal.rs b/src/home/room_settings_modal.rs index 114cc2dae..86a059825 100644 --- a/src/home/room_settings_modal.rs +++ b/src/home/room_settings_modal.rs @@ -204,15 +204,34 @@ pub fn next_step_after_directory_write(directory_ok: bool) -> SequencedAliasStep pub enum RoomSettingsFetchReason { /// Opened the modal (or another non-write refresh), carrying a monotonic /// epoch. Not tied to any alias write, so it must NEVER consume a write's - /// reconcile. Its data is applied only when it is the *newest* Open and no - /// write has been submitted since it was issued (see [`OpenFreshness`]) — so - /// a slow pre-write Open can't repaint stale state after a write reconciles. + /// reconcile. Reads the (fast) local cache; its data is applied only when it + /// is the *newest* open-class fetch and no write has been submitted since it + /// was issued (see [`OpenFreshness`]) — so a slow pre-write Open can't + /// repaint stale state after a write reconciles. Open(u64), + /// A post-barrier *recovery* re-fetch (round 7), issued after a matching + /// reconcile came back Unavailable so the modal isn't wedged read-only. + /// Behaves like [`Self::Open`] in the gate (applies when idle, epoch-guarded) + /// but reads SERVER truth (not the cache), so it can never repopulate the + /// modal with a pre-write snapshot. + Recovery(u64), /// The authoritative reconcile for the alias write of this generation. Only a /// matching generation may release that write's gate / clear its registry. AliasReconcile(u64), } +impl RoomSettingsFetchReason { + /// The freshness epoch for an "open-class" fetch (`Open` / `Recovery`), which + /// apply when idle and are gated by [`OpenFreshness`]. `None` for a reconcile + /// (matched by generation instead). + pub fn open_epoch(self) -> Option { + match self { + RoomSettingsFetchReason::Open(e) | RoomSettingsFetchReason::Recovery(e) => Some(e), + RoomSettingsFetchReason::AliasReconcile(_) => None, + } + } +} + /// Freshness guard for `Open` settings fetches (P1-1). An `Open` snapshots /// server state at request time, so a slow one issued *before* a write can /// return *after* that write reconciles and repaint pre-write state — letting @@ -342,9 +361,10 @@ impl AliasWriteGate { /// `AwaitingRefresh(g)` with the same generation; otherwise `Ignore`. pub fn disposition(self, reason: RoomSettingsFetchReason) -> FetchDisposition { match (self, reason) { - // Open only *gates* on Idle here; its epoch freshness is enforced by - // the caller via `OpenFreshness` (P1-1). - (AliasWriteGate::Idle, RoomSettingsFetchReason::Open(_)) => FetchDisposition::Apply, + // Open-class fetches (Open / Recovery) only *gate* on Idle here; their + // epoch freshness is enforced by the caller via `OpenFreshness` (P1-1). + (AliasWriteGate::Idle, RoomSettingsFetchReason::Open(_)) + | (AliasWriteGate::Idle, RoomSettingsFetchReason::Recovery(_)) => FetchDisposition::Apply, (AliasWriteGate::AwaitingRefresh(g), RoomSettingsFetchReason::AliasReconcile(r)) if g == r => { @@ -947,6 +967,41 @@ mod alias_logic_tests { // cache data — it releases via Unavailable instead. assert_eq!(reconcile_fetch_outcome(None), ReconcileFetchOutcome::Unavailable); } + + // ── round 7: matching-Unavailable is a terminal freshness barrier ── + // The failing interleaving: a reopen DURING a pending write issues Open(e2) + // (take_open AFTER take_write); the write's reconcile then comes back + // *matching-Unavailable*. Unlike round 6's test (Open issued BEFORE the + // write), here e2 > the write generation, so only a terminal barrier on the + // Unavailable path can invalidate it. + + #[test] + fn test_regression_unavailable_barrier_then_reopen_open_rejected() { + // Order (a): matching Unavailable THEN the post-write reopen Open. The + // barrier (on_apply, fired on the matching Unavailable) must reject e2 so + // it can never apply its possibly-pre-write snapshot. + let mut f = OpenFreshness::default(); + let _g_w1 = f.take_write(); // W1 submitted (pending) + let e2 = f.take_open(); // reopen-during-pending Open (e2 > W1 generation) + f.on_apply(); // matching-Unavailable terminal barrier + assert!(!f.accepts_open(e2)); // rejected — no stale apply + } + + #[test] + fn test_regression_reopen_open_then_unavailable_recovers_without_stale_or_wedge() { + // Order (b): the reopen Open(e2) arrives first (ignored while the gate is + // held — modeled by NOT applying it), THEN the matching Unavailable fires + // the barrier + a recovery Open(e3). e2 must be stale (no stale apply); + // e3 (issued post-barrier, after show() reset can_manage=false) must be + // acceptable so the modal repopulates and is NOT wedged read-only. + let mut f = OpenFreshness::default(); + let _g_w1 = f.take_write(); + let e2 = f.take_open(); // reopen Open — arrives first, gate-ignored (not applied) + f.on_apply(); // matching-Unavailable barrier + let e3 = f.take_open(); // post-barrier recovery Open + assert!(!f.accepts_open(e2)); // e2 can never apply → no stale apply + assert!(f.accepts_open(e3)); // e3 repopulates → not wedged read-only + } } // ───────────────────────────────────────────────────────────────────────────── @@ -2094,11 +2149,12 @@ impl RoomSettingsModal { if disposition == FetchDisposition::Ignore { return; } - // P1-1: an accepted Open must still be the *freshest*. Reject a slow - // pre-write open that would otherwise repaint stale state after a write - // reconciled (and let the user submit a second write from it). The gate - // was not mutated for an Open, so returning here leaves it Idle. - if let RoomSettingsFetchReason::Open(epoch) = reason { + // P1-1: an accepted open-class fetch (Open / Recovery) must still be the + // *freshest*. Reject a slow pre-write open that would otherwise repaint + // stale state after a write reconciled (and let the user submit a second + // write from it). The gate was not mutated for these, so returning here + // leaves it Idle. + if let Some(epoch) = reason.open_epoch() { if !self.open_freshness.accepts_open(epoch) { return; } @@ -2117,16 +2173,35 @@ impl RoomSettingsModal { /// Release a waiting alias write's gate when its reconcile fetch could not /// produce data (no client / room unavailable). Unlike `apply_alias_settings` - /// this does NOT overwrite state — it keeps the current (optimistic) aliases - /// and just re-enables the controls, so the gate can never strand disabled. - /// Releases ONLY on the matching reconcile (generation + purpose); an - /// open-fetch or mismatched reconcile is a no-op. - pub fn release_alias_lock(&mut self, cx: &mut Cx, room_id: &RoomId, reason: RoomSettingsFetchReason) { + /// this does NOT overwrite state. Releases ONLY on the matching reconcile + /// (generation + purpose); an open-fetch or mismatched reconcile is a no-op. + /// + /// A matching-Unavailable is a *terminal freshness barrier* (P1, round 7): + /// it invalidates every `Open` issued during that pending generation (via + /// `OpenFreshness::on_apply`), exactly as a successful reconcile does — so a + /// reopen-during-the-write `Open` that returns after this release can't apply + /// its (possibly pre-write) snapshot. Because that barrier also blocks the + /// reopen's own `Open`, the modal would be left read-only; so this returns + /// `Some(epoch)` for a fresh post-barrier recovery `Open` the caller must + /// issue to repopulate the modal via a fetch that postdates the invalidation. + pub fn release_alias_lock( + &mut self, + cx: &mut Cx, + room_id: &RoomId, + reason: RoomSettingsFetchReason, + ) -> Option { if !self.is_current_room(room_id) { - return; + return None; } if self.alias_gate.on_fetch_unavailable(reason) { + // Terminal barrier: invalidate all Opens from the pending generation. + self.open_freshness.on_apply(); self.render_alias_section(cx); + // Recovery: a fresh post-barrier Open so the modal isn't wedged + // read-only (the reopen's own Open was just invalidated). + Some(self.open_freshness.take_open()) + } else { + None } } @@ -2434,9 +2509,15 @@ impl RoomSettingsModalRef { } /// Release a stranded alias gate when its reconcile fetch was unavailable. - pub fn release_alias_lock(&self, cx: &mut Cx, room_id: &RoomId, reason: RoomSettingsFetchReason) { - let Some(mut inner) = self.borrow_mut() else { return }; - inner.release_alias_lock(cx, room_id, reason); + /// Returns `Some(epoch)` for a fresh recovery `Open` the caller must issue. + pub fn release_alias_lock( + &self, + cx: &mut Cx, + room_id: &RoomId, + reason: RoomSettingsFetchReason, + ) -> Option { + let mut inner = self.borrow_mut()?; + inner.release_alias_lock(cx, room_id, reason) } /// Update the avatar widget after a successful upload. diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index 222b5fc1b..c852b4706 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -4486,12 +4486,15 @@ async fn matrix_worker_task( None => false, }; // Capture alias data AFTER the power-level await, at post - // time (P1-2). A reconcile reads SERVER truth (the local - // cache lags `send_state_event`); if that fresh read fails - // it releases via Unavailable rather than apply stale data. - // An open uses the cache (epoch-guarded, no extra round-trip). + // time (P1-2). A reconcile and a post-barrier Recovery read + // SERVER truth (the local cache lags `send_state_event`); if + // that fresh read fails they release via Unavailable rather + // than apply stale data. A plain Open uses the cache + // (fast, no round-trip) — a stale reopen Open is rejected by + // the OpenFreshness barrier, so it never applies (round 7). let (canonical_alias, alt_aliases) = match reason { - RoomSettingsFetchReason::AliasReconcile(_) => { + RoomSettingsFetchReason::AliasReconcile(_) + | RoomSettingsFetchReason::Recovery(_) => { match reconcile_fetch_outcome( fetch_canonical_alias_from_server(&client, &room_id).await, ) { From 1d117aa58b17a3ba527411d3d70c3468b9da9a41 Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Sun, 26 Jul 2026 16:53:02 +0800 Subject: [PATCH 10/11] =?UTF-8?q?fix(room-aliases):=20single=20source=20of?= =?UTF-8?q?=20truth=20=E2=80=94=20every=20alias=20fetch=20reads=20server,?= =?UTF-8?q?=20cache=20read=20deleted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's root-cause finding (msg_0255): epoch ordering ranked freshness across TWO sources (local cache vs server) with different consistency, so a later cache-Open could legitimately supersede server truth by epoch and apply a stale snapshot + can_manage — write-enablement (a W2 from the stale full-state snapshot de-advertises a still-directory-mapped alias). Fix (coordinator directive — kill the cache read entirely): EVERY alias fetch (Open, the post-barrier recovery Open, and AliasReconcile) now reads `m.room.canonical_alias` from the SERVER via `fetch_canonical_alias_from_server` + `reconcile_fetch_outcome` (404 → fresh empty; error → Unavailable, no apply). The Open arm's `room.canonical_alias()/alt_aliases()` cache reads are gone. With one source, epochs order same-source reads only (sound), and the entire cross-source provenance-ranking class dies by construction — a fetch payload can never be a stale cache value. The `Recovery` fetch reason collapsed into a plain server-truth `Open` (removed). The barrier + one-shot recovery behavior is unchanged: a matching-Unavailable still invalidates the pending generation's Opens and issues one fresh recovery Open; a failed Open releases nothing (gate untouched) and does not respawn, so there is no wedge and no loop. Modal opens are user-initiated and rare, so the one extra GET per open is negligible. Adds codex-named source-aware tests (a) later-Open-after-recovery-pending and (b) Open-after-recovery-applied-before-sync, proving the applied payload is the server read and a modeled stale cache value can never appear. cargo test --lib: 579 passed / 0 failed. agent-spec lint: 100%. Co-Authored-By: Claude Fable 5 --- src/app.rs | 6 +- src/home/room_settings_modal.rs | 134 +++++++++++++++++++++----------- src/sliding_sync.rs | 45 +++++------ 3 files changed, 111 insertions(+), 74 deletions(-) diff --git a/src/app.rs b/src/app.rs index f85a27443..cfcea4273 100644 --- a/src/app.rs +++ b/src/app.rs @@ -2088,14 +2088,14 @@ impl MatchEvent for App { // A matching-reconcile Unavailable is a terminal freshness barrier // (round 7): it invalidates the reopen's own Open, so the modal // hands back a fresh post-barrier Open epoch to repopulate via a - // (server-fresh) fetch that postdates the invalidation — never a - // pre-write cache snapshot, and never wedged read-only. + // (server-truth) fetch that postdates the invalidation — never + // wedged read-only. A failed recovery Open does not spawn another. let recovery = self.ui.room_settings_modal(cx, ids!(room_settings_modal_inner)) .release_alias_lock(cx, &unavailable.room_id, unavailable.reason); if let Some(recovery_epoch) = recovery { submit_async_request(MatrixRequest::FetchRoomSettings { room_id: unavailable.room_id.clone(), - reason: RoomSettingsFetchReason::Recovery(recovery_epoch), + reason: RoomSettingsFetchReason::Open(recovery_epoch), }); } continue; diff --git a/src/home/room_settings_modal.rs b/src/home/room_settings_modal.rs index 86a059825..5a88e42c0 100644 --- a/src/home/room_settings_modal.rs +++ b/src/home/room_settings_modal.rs @@ -202,36 +202,22 @@ pub fn next_step_after_directory_write(directory_ok: bool) -> SequencedAliasStep /// flagged: settings fetches previously carried only `room_id`). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum RoomSettingsFetchReason { - /// Opened the modal (or another non-write refresh), carrying a monotonic - /// epoch. Not tied to any alias write, so it must NEVER consume a write's - /// reconcile. Reads the (fast) local cache; its data is applied only when it - /// is the *newest* open-class fetch and no write has been submitted since it - /// was issued (see [`OpenFreshness`]) — so a slow pre-write Open can't - /// repaint stale state after a write reconciles. + /// A non-write refresh (modal open, or the post-barrier recovery re-fetch), + /// carrying a monotonic epoch. Not tied to any alias write, so it must NEVER + /// consume a write's reconcile. Its data is applied only when it is the + /// *newest* Open and no write has been submitted since it was issued (see + /// [`OpenFreshness`]). + /// + /// Like every alias fetch it reads `m.room.canonical_alias` from the SERVER + /// (round 8: single source of truth) — the local cache is never consulted, so + /// there is no cross-source freshness to rank and a fetch payload can never be + /// a stale cache snapshot. Open(u64), - /// A post-barrier *recovery* re-fetch (round 7), issued after a matching - /// reconcile came back Unavailable so the modal isn't wedged read-only. - /// Behaves like [`Self::Open`] in the gate (applies when idle, epoch-guarded) - /// but reads SERVER truth (not the cache), so it can never repopulate the - /// modal with a pre-write snapshot. - Recovery(u64), /// The authoritative reconcile for the alias write of this generation. Only a /// matching generation may release that write's gate / clear its registry. AliasReconcile(u64), } -impl RoomSettingsFetchReason { - /// The freshness epoch for an "open-class" fetch (`Open` / `Recovery`), which - /// apply when idle and are gated by [`OpenFreshness`]. `None` for a reconcile - /// (matched by generation instead). - pub fn open_epoch(self) -> Option { - match self { - RoomSettingsFetchReason::Open(e) | RoomSettingsFetchReason::Recovery(e) => Some(e), - RoomSettingsFetchReason::AliasReconcile(_) => None, - } - } -} - /// Freshness guard for `Open` settings fetches (P1-1). An `Open` snapshots /// server state at request time, so a slow one issued *before* a write can /// return *after* that write reconciles and repaint pre-write state — letting @@ -361,10 +347,9 @@ impl AliasWriteGate { /// `AwaitingRefresh(g)` with the same generation; otherwise `Ignore`. pub fn disposition(self, reason: RoomSettingsFetchReason) -> FetchDisposition { match (self, reason) { - // Open-class fetches (Open / Recovery) only *gate* on Idle here; their - // epoch freshness is enforced by the caller via `OpenFreshness` (P1-1). - (AliasWriteGate::Idle, RoomSettingsFetchReason::Open(_)) - | (AliasWriteGate::Idle, RoomSettingsFetchReason::Recovery(_)) => FetchDisposition::Apply, + // Open only *gates* on Idle here; its epoch freshness is enforced by + // the caller via `OpenFreshness` (P1-1). + (AliasWriteGate::Idle, RoomSettingsFetchReason::Open(_)) => FetchDisposition::Apply, (AliasWriteGate::AwaitingRefresh(g), RoomSettingsFetchReason::AliasReconcile(r)) if g == r => { @@ -521,12 +506,12 @@ pub enum ReconcileFetchOutcome { Unavailable, } -/// Decide what a reconcile fetch posts from the result of its server-fresh +/// Decide what an alias fetch posts from the result of its server-fresh /// canonical-alias read. In the pinned matrix-sdk, `send_state_event` does not /// update the local `RoomInfo`, so the cache-backed `room.canonical_alias()` can -/// still return the *pre-write* value after a successful write; a reconcile must -/// therefore read the server directly and, if that read fails, release via -/// `Unavailable` rather than apply stale cache data. `server_read == None` means +/// still return the *pre-write* value after a successful write; every alias +/// fetch therefore reads the server directly and, if that read fails, releases +/// via `Unavailable` rather than apply stale data. `server_read == None` means /// the fresh read could not be obtained. pub fn reconcile_fetch_outcome( server_read: Option<(Option, Vec)>, @@ -537,6 +522,19 @@ pub fn reconcile_fetch_outcome( } } +/// The payload an alias fetch applies, under the SINGLE-SOURCE model (round 8): +/// it is derived ONLY from the server read. The `_local_cache` parameter is +/// present solely to make the guarantee testable — the applied payload is +/// independent of the cache, so a stale cache value can never appear in a fetch +/// (which is what makes epoch ordering sound: it ranks same-source reads only). +pub fn alias_fetch_payload( + _local_cache: (Option, Vec), + server_read: Option<(Option, Vec)>, +) -> ReconcileFetchOutcome { + // The cache is intentionally ignored: every fetch reads server truth. + reconcile_fetch_outcome(server_read) +} + #[cfg(test)] mod alias_logic_tests { use super::*; @@ -1002,6 +1000,52 @@ mod alias_logic_tests { assert!(!f.accepts_open(e2)); // e2 can never apply → no stale apply assert!(f.accepts_open(e3)); // e3 repopulates → not wedged read-only } + + // ── round 8: single source of truth — no fetch payload can be stale cache ── + // Every alias fetch (Open, recovery Open, reconcile) reads the SERVER; the + // cache is never consulted. So epochs only ever order same-source (server) + // reads, and a stale cache snapshot can never reach a fetch payload — which + // is what closes the cross-source provenance-ranking hole codex flagged. + + #[test] + fn test_single_source_a_later_open_applies_server_not_cache() { + // (a) A recovery Open is pending and a later Open(e4) is issued. Both read + // the server. Concrete repro: W1 published #a. The local cache still shows + // NO #a (S0); the server shows #a (S1). The applied payload must be the + // SERVER read — the cache S0 (which would drop #a from a W2 alt-list and + // de-advertise the still-directory-mapped #a) can never appear. + let s1_server = Some((None, vec![alias("#a:example.org"), alias("#b:example.org")])); + let s0_stale_cache = (None, vec![alias("#b:example.org")]); // missing #a + let arbitrary_other_cache = (Some(alias("#a:example.org")), vec![alias("#b:example.org")]); + // The payload is independent of the cache: any cache → same server payload. + assert_eq!( + alias_fetch_payload(s0_stale_cache.clone(), s1_server.clone()), + alias_fetch_payload(arbitrary_other_cache, s1_server.clone()), + ); + assert_eq!( + alias_fetch_payload(s0_stale_cache, s1_server), + ReconcileFetchOutcome::Fetched { + canonical: None, + alt_aliases: vec![alias("#a:example.org"), alias("#b:example.org")], + }, + ); + } + + #[test] + fn test_single_source_b_open_after_recovery_applied_before_sync() { + // (b) The recovery already applied S1, then a later Open(e4) is issued + // BEFORE the local cache syncs (cache still S0). The later Open also reads + // the server, so it re-applies S1; the pre-write cache S0 is unreachable. + let s1_server = Some((Some(alias("#a:example.org")), vec![alias("#b:example.org")])); + let s0_stale_cache = (None, vec![]); // sync hasn't caught up + assert_eq!( + alias_fetch_payload(s0_stale_cache, s1_server), + ReconcileFetchOutcome::Fetched { + canonical: Some(alias("#a:example.org")), + alt_aliases: vec![alias("#b:example.org")], + }, + ); + } } // ───────────────────────────────────────────────────────────────────────────── @@ -2149,12 +2193,11 @@ impl RoomSettingsModal { if disposition == FetchDisposition::Ignore { return; } - // P1-1: an accepted open-class fetch (Open / Recovery) must still be the - // *freshest*. Reject a slow pre-write open that would otherwise repaint - // stale state after a write reconciled (and let the user submit a second - // write from it). The gate was not mutated for these, so returning here - // leaves it Idle. - if let Some(epoch) = reason.open_epoch() { + // P1-1: an accepted Open must still be the *freshest*. Reject a slow + // pre-write open that would otherwise repaint older state after a write + // reconciled (and let the user submit a second write from it). The gate + // was not mutated for an Open, so returning here leaves it Idle. + if let RoomSettingsFetchReason::Open(epoch) = reason { if !self.open_freshness.accepts_open(epoch) { return; } @@ -2176,14 +2219,15 @@ impl RoomSettingsModal { /// this does NOT overwrite state. Releases ONLY on the matching reconcile /// (generation + purpose); an open-fetch or mismatched reconcile is a no-op. /// - /// A matching-Unavailable is a *terminal freshness barrier* (P1, round 7): - /// it invalidates every `Open` issued during that pending generation (via + /// A matching-Unavailable is a *terminal freshness barrier* (round 7): it + /// invalidates every `Open` issued during that pending generation (via /// `OpenFreshness::on_apply`), exactly as a successful reconcile does — so a - /// reopen-during-the-write `Open` that returns after this release can't apply - /// its (possibly pre-write) snapshot. Because that barrier also blocks the - /// reopen's own `Open`, the modal would be left read-only; so this returns - /// `Some(epoch)` for a fresh post-barrier recovery `Open` the caller must - /// issue to repopulate the modal via a fetch that postdates the invalidation. + /// reopen-during-the-write `Open` that returns after this release can't + /// supersede it by epoch. Because that barrier also blocks the reopen's own + /// `Open`, the modal would be left read-only; so this returns `Some(epoch)` + /// for a fresh post-barrier recovery `Open` the caller must issue to + /// repopulate the modal via a (server-truth) fetch that postdates the + /// invalidation. A failed recovery `Open` does not spawn another (bounded). pub fn release_alias_lock( &mut self, cx: &mut Cx, diff --git a/src/sliding_sync.rs b/src/sliding_sync.rs index c852b4706..5559d10e4 100644 --- a/src/sliding_sync.rs +++ b/src/sliding_sync.rs @@ -4471,9 +4471,7 @@ async fn matrix_worker_task( continue; }; let _fetch_room_settings_task = Handle::current().spawn(async move { - use crate::home::room_settings_modal::{ - reconcile_fetch_outcome, ReconcileFetchOutcome, RoomSettingsFetchReason, - }; + use crate::home::room_settings_modal::{reconcile_fetch_outcome, ReconcileFetchOutcome}; if let Some(room) = client.get_room(&room_id) { let topic = room.topic(); let is_public = room.is_public().unwrap_or(false); @@ -4485,30 +4483,25 @@ async fn matrix_worker_task( .is_some_and(|pl| pl.can_set_canonical_alias()), None => false, }; - // Capture alias data AFTER the power-level await, at post - // time (P1-2). A reconcile and a post-barrier Recovery read - // SERVER truth (the local cache lags `send_state_event`); if - // that fresh read fails they release via Unavailable rather - // than apply stale data. A plain Open uses the cache - // (fast, no round-trip) — a stale reopen Open is rejected by - // the OpenFreshness barrier, so it never applies (round 7). - let (canonical_alias, alt_aliases) = match reason { - RoomSettingsFetchReason::AliasReconcile(_) - | RoomSettingsFetchReason::Recovery(_) => { - match reconcile_fetch_outcome( - fetch_canonical_alias_from_server(&client, &room_id).await, - ) { - ReconcileFetchOutcome::Fetched { canonical, alt_aliases } => { - (canonical, alt_aliases) - } - ReconcileFetchOutcome::Unavailable => { - Cx::post_action(RoomSettingsFetchUnavailableAction { room_id, reason }); - return; - } - } + // SINGLE SOURCE OF TRUTH (round 8): EVERY alias fetch — + // Open, recovery Open, and reconcile alike — reads + // `m.room.canonical_alias` from the SERVER, captured AFTER + // the power-level await (post time). The local + // `room.canonical_alias()/alt_aliases()` cache lags + // `send_state_event`, so ranking a cache read against a + // server read by epoch could apply a stale snapshot; with + // one source there is no cross-source provenance to rank and + // a payload can never be a stale cache value. A failed fresh + // read releases via Unavailable rather than apply anything. + let (canonical_alias, alt_aliases) = match reconcile_fetch_outcome( + fetch_canonical_alias_from_server(&client, &room_id).await, + ) { + ReconcileFetchOutcome::Fetched { canonical, alt_aliases } => { + (canonical, alt_aliases) } - RoomSettingsFetchReason::Open(_) => { - (room.canonical_alias(), room.alt_aliases()) + ReconcileFetchOutcome::Unavailable => { + Cx::post_action(RoomSettingsFetchUnavailableAction { room_id, reason }); + return; } }; Cx::post_action(RoomSettingsFetchedAction { From 6d2d1d2f9a8ecaee20a57d1b420d209154d606e5 Mon Sep 17 00:00:00 2001 From: AlexZhang Date: Sun, 26 Jul 2026 17:16:36 +0800 Subject: [PATCH 11/11] fix(room-aliases): stay read-only (Recovering) on matching-Unavailable until recovery Open applies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex found the last recovery-path gap (msg_0275): release_alias_lock set the gate Idle and re-rendered IMMEDIATELY on a matching reconcile-Unavailable, with can_manage_aliases still true — so edit controls came back while the recovery Open's server GET was still pending. A write's send can err ambiguously (the server may have applied S1; error != not-applied); the rollback shows S0, so the user could submit W2 from S0 (whose take_write even invalidates the pending recovery epoch) and overwrite S1, losing W1 — violating the gate invariant that the next mutation builds on reconciled state. Fix: a matching-Unavailable no longer unlocks. It enters a new explicit read-only state, AliasWriteGate::Recovering(epoch), where can_submit() is false so mutations are refused and the controls are hidden (edit_enabled = can_manage && can_submit). Only the recovery Open(epoch), when ACCEPTED, applies server truth AND releases the gate to Idle — restoring permission from that apply (which recomputes can_manage from power levels). The barrier (OpenFreshness::on_apply) still fires. A failed recovery Open does not release from the unknown state; the gate stays Recovering (read-only) until an explicit reopen, which re-derives from the (cleared) registry to Idle and issues its own fresh Open — "reopen replaces recovery" (documented in show()). The epoch freshness check now runs before any gate mutation, so a stale open can never release the gate. Adds codex-named tests: (a) Unavailable → recovery pending → second write rejected (can_submit false in Recovering), released only after the recovery Open applies; (b) bounded recovery failure → stays Recovering (read-only), reopen recovers. cargo test --lib: 581 passed / 0 failed. agent-spec lint: 100%. Co-Authored-By: Claude Fable 5 --- src/home/room_settings_modal.rs | 186 ++++++++++++++++++++++---------- 1 file changed, 132 insertions(+), 54 deletions(-) diff --git a/src/home/room_settings_modal.rs b/src/home/room_settings_modal.rs index 5a88e42c0..b9073c375 100644 --- a/src/home/room_settings_modal.rs +++ b/src/home/room_settings_modal.rs @@ -288,6 +288,11 @@ pub enum AliasWriteGate { AwaitingResult(u64), /// The write (this generation) reached the server; awaiting its reconcile. AwaitingRefresh(u64), + /// The write's reconcile came back Unavailable (round 9): the write outcome + /// is unknown, so edit controls stay non-interactive (read-only) until the + /// recovery `Open(epoch)` APPLIES server truth. Never releases from an + /// unknown state — a failed recovery stays here until an explicit reopen. + Recovering(u64), } /// What the modal should do with an incoming settings fetch, decided purely from @@ -355,13 +360,19 @@ impl AliasWriteGate { { FetchDisposition::ApplyAndRelease } + // The recovery Open (matched by epoch) is what leaves `Recovering`: + // it applies server truth AND releases the gate to Idle (round 9), so + // controls only come back once authoritative state has been applied. + (AliasWriteGate::Recovering(e), RoomSettingsFetchReason::Open(r)) if e == r => { + FetchDisposition::ApplyAndRelease + } _ => FetchDisposition::Ignore, } } /// Apply the disposition of a fetch, releasing the gate iff it is the - /// matching reconcile. Returns the disposition so the caller can decide - /// whether to overwrite state. + /// matching reconcile or the matching recovery Open. Returns the disposition + /// so the caller can decide whether to overwrite state. pub fn on_fetch(&mut self, reason: RoomSettingsFetchReason) -> FetchDisposition { let disposition = self.disposition(reason); if disposition == FetchDisposition::ApplyAndRelease { @@ -370,19 +381,21 @@ impl AliasWriteGate { disposition } - /// A settings fetch could not produce data (no client / room gone). Release - /// the gate ONLY if it is the matching reconcile for a write awaiting one - /// (so the controls never strand disabled). Returns whether it released. - pub fn on_fetch_unavailable(&mut self, reason: RoomSettingsFetchReason) -> bool { - if let (AliasWriteGate::AwaitingRefresh(g), RoomSettingsFetchReason::AliasReconcile(r)) = - (*self, reason) - { - if g == r { - *self = AliasWriteGate::Idle; - return true; - } - } - false + /// Whether `reason` is the matching reconcile for the write this gate is + /// awaiting — used to decide whether an Unavailable enters `Recovering`. + pub fn matches_reconcile(self, reason: RoomSettingsFetchReason) -> bool { + matches!( + (self, reason), + (AliasWriteGate::AwaitingRefresh(g), RoomSettingsFetchReason::AliasReconcile(r)) if g == r + ) + } + + /// The matching reconcile came back Unavailable: enter `Recovering(epoch)` + /// instead of releasing to `Idle`, so edit controls stay non-interactive + /// (`can_submit() == false`) until the recovery `Open(epoch)` applies server + /// truth. Callers must first confirm [`Self::matches_reconcile`]. + pub fn enter_recovering(&mut self, recovery_epoch: u64) { + *self = AliasWriteGate::Recovering(recovery_epoch); } } @@ -750,13 +763,17 @@ mod alias_logic_tests { } #[test] - fn test_alias_gate_unavailable_releases_only_matching_reconcile() { - let mut gate = AliasWriteGate::AwaitingRefresh(4); - assert!(!gate.on_fetch_unavailable(RoomSettingsFetchReason::Open(1))); // open-fetch never releases - assert!(!gate.on_fetch_unavailable(RoomSettingsFetchReason::AliasReconcile(5))); // wrong gen - assert_eq!(gate, AliasWriteGate::AwaitingRefresh(4)); // still held - assert!(gate.on_fetch_unavailable(RoomSettingsFetchReason::AliasReconcile(4))); // matching → release - assert_eq!(gate, AliasWriteGate::Idle); + fn test_alias_gate_matches_reconcile_only_matching() { + let gate = AliasWriteGate::AwaitingRefresh(4); + assert!(!gate.matches_reconcile(RoomSettingsFetchReason::Open(1))); // open never matches + assert!(!gate.matches_reconcile(RoomSettingsFetchReason::AliasReconcile(5))); // wrong gen + assert!(gate.matches_reconcile(RoomSettingsFetchReason::AliasReconcile(4))); // matching + // A matching Unavailable enters Recovering (NOT Idle) — controls stay + // non-interactive until the recovery Open applies (round 9). + let mut g = gate; + g.enter_recovering(9); + assert_eq!(g, AliasWriteGate::Recovering(9)); + assert!(!g.can_submit()); } // ── codex-named regression tests (generation/purpose correlation) ── @@ -1046,6 +1063,58 @@ mod alias_logic_tests { }, ); } + + // ── round 9: don't unlock on matching-Unavailable — stay Recovering ── + // A write's send can err ambiguously (server may have applied it), so on a + // matching reconcile-Unavailable the modal must NOT re-enable edits from that + // unknown state; it stays read-only (Recovering) until the recovery Open + // applies server truth, and restores permission only from that apply. + + #[test] + fn test_recovering_a_blocks_second_write_until_recovery_applies() { + // (a) Unavailable → recovery pending → a second write is rejected. + let mut gate = AliasWriteGate::default(); + gate.on_submit(1); // W1 + gate.on_result(true); // AwaitingRefresh(1) + assert!(gate.matches_reconcile(RoomSettingsFetchReason::AliasReconcile(1))); + gate.enter_recovering(3); // matching Unavailable → Recovering(recovery epoch) + assert_eq!(gate, AliasWriteGate::Recovering(3)); + assert!(!gate.can_submit()); // W2 rejected while recovering + assert!(!gate.on_submit(4)); // an attempted submit is refused… + assert_eq!(gate, AliasWriteGate::Recovering(3)); // …gate unchanged + // Only the recovery Open(3) applying server truth releases the gate: + assert_eq!( + gate.on_fetch(RoomSettingsFetchReason::Open(3)), + FetchDisposition::ApplyAndRelease, + ); + assert_eq!(gate, AliasWriteGate::Idle); + assert!(gate.can_submit()); // interactive ONLY after the apply + } + + #[test] + fn test_recovering_b_bounded_failure_stays_read_only_until_reopen() { + // (b) The recovery Open also fails: the gate must NOT release from an + // unknown state; it stays read-only until an explicit reopen recovers it. + let mut gate = AliasWriteGate::Recovering(3); + // A failed recovery Open is not a reconcile → release_alias_lock no-ops: + assert!(!gate.matches_reconcile(RoomSettingsFetchReason::Open(3))); + // A stray/older open can't unlock it either: + assert_eq!( + gate.on_fetch(RoomSettingsFetchReason::Open(2)), + FetchDisposition::Ignore, + ); + assert_eq!(gate, AliasWriteGate::Recovering(3)); // still read-only + assert!(!gate.can_submit()); + // Reopen re-derives from the (cleared) registry → Idle (show() mapping), + // and issues its own fresh Open — recovering the modal. + let reopened_gate = match None::<(PendingAliasStage, u64)> { + Some((PendingAliasStage::Submitted, g)) => AliasWriteGate::AwaitingResult(g), + Some((PendingAliasStage::AwaitingReconcile, g)) => AliasWriteGate::AwaitingRefresh(g), + None => AliasWriteGate::Idle, + }; + assert_eq!(reopened_gate, AliasWriteGate::Idle); + assert!(reopened_gate.can_submit()); // reopen recovers + } } // ───────────────────────────────────────────────────────────────────────────── @@ -2096,6 +2165,12 @@ impl RoomSettingsModal { // own reconcile (matching generation) unlocks it. `Submitted` → // AwaitingResult (reject an unrelated open-fetch until the result // returns); `AwaitingReconcile` → AwaitingRefresh. No pending write → Idle. + // + // Round 9 invariant "reopen replaces recovery": a `Recovering` gate is + // never a registry stage (the registry entry is cleared when the reconcile + // Unavailable enters Recovering), so reopening a mid-recovery room maps to + // `Idle` here and issues its own fresh open-fetch — abandoning the stale + // recovery Open (which is rejected by epoch if it still lands). self.alias_gate = match alias_stage { Some((PendingAliasStage::Submitted, generation)) => AliasWriteGate::AwaitingResult(generation), Some((PendingAliasStage::AwaitingReconcile, generation)) => AliasWriteGate::AwaitingRefresh(generation), @@ -2184,24 +2259,29 @@ impl RoomSettingsModal { if !self.is_current_room(room_id) { return; } - // Route the gate decision BEFORE touching state, matched by generation + - // purpose (P1). `Ignore` → a stale open-fetch or mismatched reconcile: - // drop it (never clobber optimistic state / snapshot). `ApplyAndRelease` - // → this write's own reconcile: apply + release. `Apply` → an open-fetch - // while idle: load without releasing anything. - let disposition = self.alias_gate.on_fetch(reason); + // Decide (purely, no gate mutation yet) whether to apply. `Ignore` → a + // stale open-fetch or mismatched reconcile: drop it (never clobber + // optimistic state / snapshot). `ApplyAndRelease` → this write's own + // reconcile OR the recovery Open leaving `Recovering`: apply + release. + // `Apply` → an open-fetch while idle: load without releasing. + let disposition = self.alias_gate.disposition(reason); if disposition == FetchDisposition::Ignore { return; } // P1-1: an accepted Open must still be the *freshest*. Reject a slow // pre-write open that would otherwise repaint older state after a write - // reconciled (and let the user submit a second write from it). The gate - // was not mutated for an Open, so returning here leaves it Idle. + // reconciled (and let the user submit a second write from it). Checked + // BEFORE mutating the gate, so a stale open can never release it. if let RoomSettingsFetchReason::Open(epoch) = reason { if !self.open_freshness.accepts_open(epoch) { return; } } + // Accepted: release the gate to Idle if this was the matching reconcile + // or the recovery Open. (`Apply` — an idle open-load — leaves it Idle.) + if disposition == FetchDisposition::ApplyAndRelease { + self.alias_gate = AliasWriteGate::Idle; + } // Accepted authoritative apply — invalidate every earlier open so none // can repaint over the state we're about to store (P1-1). self.open_freshness.on_apply(); @@ -2214,39 +2294,37 @@ impl RoomSettingsModal { self.render_alias_section(cx); } - /// Release a waiting alias write's gate when its reconcile fetch could not - /// produce data (no client / room unavailable). Unlike `apply_alias_settings` - /// this does NOT overwrite state. Releases ONLY on the matching reconcile - /// (generation + purpose); an open-fetch or mismatched reconcile is a no-op. + /// Handle a reconcile fetch that could not produce data (no client / room + /// unavailable). Acts ONLY on the matching reconcile (generation + purpose); + /// an open-fetch or mismatched reconcile is a no-op. /// - /// A matching-Unavailable is a *terminal freshness barrier* (round 7): it - /// invalidates every `Open` issued during that pending generation (via - /// `OpenFreshness::on_apply`), exactly as a successful reconcile does — so a - /// reopen-during-the-write `Open` that returns after this release can't - /// supersede it by epoch. Because that barrier also blocks the reopen's own - /// `Open`, the modal would be left read-only; so this returns `Some(epoch)` - /// for a fresh post-barrier recovery `Open` the caller must issue to - /// repopulate the modal via a (server-truth) fetch that postdates the - /// invalidation. A failed recovery `Open` does not spawn another (bounded). + /// The write's outcome is now unknown (the send may have applied server-side; + /// error ≠ not-applied), so the modal must NOT unlock from that unknown state + /// (round 9). Instead it enters `Recovering(epoch)`: edit controls stay + /// non-interactive (`can_submit() == false`, controls hidden) until the + /// recovery `Open(epoch)` is ACCEPTED and applies server truth — which also + /// re-derives permission from power levels. It is still a terminal freshness + /// barrier (round 7): `OpenFreshness::on_apply` invalidates every `Open` from + /// the pending generation. Returns `Some(epoch)` for the one recovery `Open` + /// the caller must issue. If that recovery `Open` also fails, the gate stays + /// `Recovering` (read-only) until an explicit reopen — never unlocked. pub fn release_alias_lock( &mut self, cx: &mut Cx, room_id: &RoomId, reason: RoomSettingsFetchReason, ) -> Option { - if !self.is_current_room(room_id) { + if !self.is_current_room(room_id) || !self.alias_gate.matches_reconcile(reason) { return None; } - if self.alias_gate.on_fetch_unavailable(reason) { - // Terminal barrier: invalidate all Opens from the pending generation. - self.open_freshness.on_apply(); - self.render_alias_section(cx); - // Recovery: a fresh post-barrier Open so the modal isn't wedged - // read-only (the reopen's own Open was just invalidated). - Some(self.open_freshness.take_open()) - } else { - None - } + // Terminal barrier: invalidate all Opens from the pending generation. + self.open_freshness.on_apply(); + // Allocate the recovery Open epoch and hold the modal read-only in + // `Recovering` until that Open applies server truth. + let recovery_epoch = self.open_freshness.take_open(); + self.alias_gate.enter_recovering(recovery_epoch); + self.render_alias_section(cx); + Some(recovery_epoch) } /// Render the whole alias section (labels, per-row list, gating) from the