diff --git a/specs/task-room-pane-action-ownership.spec.md b/specs/task-room-pane-action-ownership.spec.md new file mode 100644 index 000000000..7a5aa48cd --- /dev/null +++ b/specs/task-room-pane-action-ownership.spec.md @@ -0,0 +1,80 @@ +spec: task +name: "Scope room pane navigation actions to their owning controls" +inherits: project +tags: [bugfix, threads, search, actions] +--- + +## Intent + +Selecting a thread in one room screen must not start an event search in another +cached room or thread tab. Carry pane ownership through dynamic thread/search +rows and consume pane, toolbar, and header actions only in their owning screen. + +## Constraints + +- Preserve actions for other room screens and split views; do not discard all actions in inactive tabs. +- Keep Matrix requests, message actions, timeline lifecycle, and global app navigation unchanged. +- Do not change layout, visible strings, locale keys, dependencies, or formatter policy. +- Do not contact Matrix services in automated tests. + +## Decisions + +- The pure function `room_control_action(action: &Action, owners: &[WidgetUid]) -> Option<&WidgetAction>` in `src/home/room_screen/action_scope.rs` is the ownership decision used by room controls. +- Dynamic thread and search entries store their pane's WidgetUid at binding time and emit selection under that owner rather than their recycled row UID. +- Thread pane actions accept only the owning pane UID. Toolbar and RoomTopBar actions accept only their own control UID. Search actions accept the local pane, search button, and room-screen relay UIDs. +- Example and property tests verify WidgetAction ownership before building. Studio/CUA checks the rendered entry-to-pane wiring. The original blanket cast is retained only long enough to record a failing regression, then replaced by the ownership decision. +- The i18n review finds no new text or changed alignment; translation resources are unchanged. + +## Boundaries + +### Allowed Changes +- src/home/room_screen/action_scope.rs +- src/home/room_screen/mod.rs +- src/home/room_screen/threads_pane.rs +- src/home/room_screen/search.rs +- src/home/search_messages.rs +- specs/task-room-pane-action-ownership.spec.md + +### Forbidden +- Do not edit src/app.rs, src/event_preview.rs, tools/ux-harness/**, or src/sliding_sync.rs. +- Do not run cargo fmt or rustfmt. +- Do not commit or create a PR before user testing. + +## Acceptance Criteria + + + +### Rule: pane-owner — Room controls accept only an explicitly owned action + +Scenario: Other thread panes cannot consume a root selection + Test: room_control_action_keeps_thread_selection_in_its_own_pane + Given main, Claude-thread, Codex-thread and other-room panes have distinct widget identities + When the main pane emits a Codex root selection + Then only the main pane accepts that action and the other panes retain no jump target + +Scenario: Search pane and header relays stay local + Test: room_control_action_accepts_local_search_controls_only + Given search pane, search button and header relay identities belong to one room screen + When each control emits a search action + Then that screen accepts the original action and a different room screen rejects it + +Scenario: Missing ownership fails closed + Test: room_control_action_rejects_missing_or_zero_ownership + Given a non-widget action, an empty owner list, or an unbound zero widget identity + When ownership is checked + Then no action is accepted + +Scenario: Generated identities preserve action isolation + Test: prop_room_control_action_matches_explicit_owner_set + Given arbitrary source identities and owner sets + When ownership is checked + Then acceptance equals nonzero source membership in the owner set + +## Out of Scope + +- Repairing unrelated global-search routing in src/app.rs. +- Changing background pagination completion or Matrix history availability. diff --git a/src/home/room_screen/action_scope.rs b/src/home/room_screen/action_scope.rs new file mode 100644 index 000000000..56c8db52f --- /dev/null +++ b/src/home/room_screen/action_scope.rs @@ -0,0 +1,76 @@ +//! Widget-instance ownership for room controls shared by multiple cached tabs. + +use makepad_widgets::*; + +pub(super) fn room_control_action<'a>(action: &'a Action, owners: &[WidgetUid]) -> Option<&'a WidgetAction> { + let action = action.as_widget_action()?; + (action.widget_uid.0 != 0 && owners.contains(&action.widget_uid)).then_some(action) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::home::room_screen::ThreadsPaneAction; + use crate::home::search_messages::SearchMessagesAction; + use matrix_sdk::ruma::OwnedEventId; + use proptest::prelude::*; + + fn emitted_by(uid: u64, payload: impl WidgetActionTrait) -> Action { + Box::new(WidgetAction { + data: None, + action: Box::new(payload), + widget_uid: WidgetUid(uid), + group: None, + }) + } + + #[test] + fn room_control_action_keeps_thread_selection_in_its_own_pane() { + let root = OwnedEventId::try_from("$codex-root").unwrap(); + let action = emitted_by(11, ThreadsPaneAction::OpenThread(root.clone())); + // Main and two thread tabs share a Matrix room, but never a widget UID. + for (pane, uid) in [("Claude thread", 12), ("Codex thread", 13), ("other room", 21)] { + assert!(room_control_action(&action, &[WidgetUid(uid)]).is_none(), "{pane} accepted another pane's jump"); + } + let selected = room_control_action(&action, &[WidgetUid(11)]).unwrap(); + assert!(matches!(selected.cast_ref::(), ThreadsPaneAction::OpenThread(event) if event == &root)); + assert_eq!(action.as_widget_action().unwrap().widget_uid, WidgetUid(11)); + } + + #[test] + fn room_control_action_accepts_local_search_controls_only() { + let local = [WidgetUid(30), WidgetUid(31), WidgetUid(32)]; + let other = [WidgetUid(40), WidgetUid(41), WidgetUid(42)]; + for source in local { + let action = emitted_by(source.0, SearchMessagesAction::OpenRequested); + assert!(matches!(room_control_action(&action, &local).cast_ref(), SearchMessagesAction::OpenRequested)); + assert!(room_control_action(&action, &other).is_none()); + } + } + + #[test] + fn room_control_action_rejects_missing_or_zero_ownership() { + let plain: Action = Box::new(()); + assert!(room_control_action(&plain, &[WidgetUid(1)]).is_none()); + assert!(room_control_action(&emitted_by(1, ThreadsPaneAction::CloseRequested), &[]).is_none()); + assert!(room_control_action(&emitted_by(0, ThreadsPaneAction::CloseRequested), &[WidgetUid(0)]).is_none()); + } + + proptest! { + #[test] + fn prop_room_control_action_matches_explicit_owner_set( + source in prop_oneof![Just(0), any::()], + mut owners in prop::collection::vec(any::(), 0..8), + include_source in any::(), + ) { + if include_source { + owners.push(source); + } + let expected = source != 0 && owners.iter().any(|owner| *owner == source); + let action = emitted_by(source, ThreadsPaneAction::LoadMoreRequested); + let owners = owners.into_iter().map(WidgetUid).collect::>(); + prop_assert_eq!(room_control_action(&action, &owners).is_some(), expected); + } + } + +} diff --git a/src/home/room_screen/mod.rs b/src/home/room_screen/mod.rs index c59dd4da2..cbbb83a31 100644 --- a/src/home/room_screen/mod.rs +++ b/src/home/room_screen/mod.rs @@ -58,6 +58,7 @@ use rangemap::RangeSet; use super::{ContextMenuOpenGesture, event_reaction_list::ReactionData, invite_modal::is_invite_modal_open, loading_pane::LoadingPaneRef, new_message_context_menu::{MessageAbilities, MessageDetails}, room_read_receipt::{self, populate_read_receipts, MAX_VISIBLE_AVATARS_IN_READ_RECEIPT}}; +mod action_scope; mod bot_admin; mod bot_message; mod dsl; @@ -74,6 +75,7 @@ mod thread_lifecycle; mod threads_pane; mod updates; +use action_scope::room_control_action; pub use bot_admin::*; use bot_message::*; pub use message::*; @@ -645,7 +647,7 @@ impl Widget for RoomScreen { } // Mobile RoomTopBar (header + Chat/Info tabs) actions. - match action.as_widget_action().cast::() { + match room_control_action(action, &[self.room_top_bar(cx, ids!(room_top_bar)).widget_uid()]).cast::() { RoomTopBarAction::Back => { cx.widget_action(room_screen_widget_uid, StackNavigationAction::Pop); } @@ -828,12 +830,9 @@ impl Widget for RoomScreen { } } - // No `widget_uid_eq` filter here — `OpenThread` is emitted from - // a `ThreadsPaneEntry` (a list item), not from the pane itself, - // so its widget_uid is the entry's. `LoadMoreRequested` and - // `CloseRequested` come from the pane, but `cast_ref` handles - // all three regardless of emitter. - match action.as_widget_action().cast_ref::() { + // Entries emit under their pane's UID so cached room/thread + // tabs only handle selections from their own pane. + match room_control_action(action, &[threads_sliding_pane.widget_uid()]).cast_ref::() { ThreadsPaneAction::OpenThread(thread_root_event_id) => { log!("RoomScreen: OpenThread received, jumping to {}", thread_root_event_id); threads_sliding_pane.hide(cx); @@ -1001,7 +1000,7 @@ impl Widget for RoomScreen { // Floating threads button click → open the threads sliding pane. for action in actions { - if let ThreadsButtonAction::OpenRequested = action.as_widget_action().cast_ref() { + if let ThreadsButtonAction::OpenRequested = room_control_action(action, &[self.view.threads_button(cx, ids!(timeline.threads_button)).widget_uid()]).cast_ref() { self.show_threads_pane(cx); break; } @@ -1010,7 +1009,7 @@ impl Widget for RoomScreen { // Floating info button click → open the room info sliding pane // (desktop only — the button is hidden on mobile). for action in actions { - if let InfoButtonAction::OpenRequested = action.as_widget_action().cast_ref() { + if let InfoButtonAction::OpenRequested = room_control_action(action, &[self.view.widget(cx, ids!(timeline.info_button)).widget_uid()]).cast_ref() { self.show_room_info_pane(cx, scope.data.get::()); break; } diff --git a/src/home/room_screen/search.rs b/src/home/room_screen/search.rs index 3f28364d6..8e5a89885 100644 --- a/src/home/room_screen/search.rs +++ b/src/home/room_screen/search.rs @@ -168,6 +168,7 @@ impl RoomScreen { ) { let pane = self.search_messages_sliding_pane(cx, ids!(search_messages_pane)); let button = self.search_messages_button(cx, ids!(timeline.search_messages_button)); + let owners = [pane.widget_uid(), button.widget_uid(), self.widget_uid()]; let mut requested_close = false; let mut requested_open = false; @@ -176,11 +177,9 @@ impl RoomScreen { let mut jump_target: Option = None; for action in actions { - // Widget-emitted actions are wrapped in a `WidgetAction`, so we - // must unwrap via `as_widget_action()` before downcasting to the - // inner `SearchMessagesAction`. `cast_ref` falls back to the - // `None` sentinel for non-matching actions. - match action.as_widget_action().cast_ref::() { + // Result rows emit under the pane UID; the RoomTopBar search + // control relays under this room screen's UID. + match room_control_action(action, &owners).cast_ref::() { SearchMessagesAction::OpenRequested => requested_open = true, SearchMessagesAction::CloseRequested => requested_close = true, SearchMessagesAction::QueryChanged(q) => new_query = Some(q.clone()), diff --git a/src/home/room_screen/threads_pane.rs b/src/home/room_screen/threads_pane.rs index b8244a63f..162e70330 100644 --- a/src/home/room_screen/threads_pane.rs +++ b/src/home/room_screen/threads_pane.rs @@ -320,6 +320,7 @@ pub struct ThreadsPaneEntry { #[deref] view: View, #[rust] thread_root_event_id: Option, + #[rust] owner_widget_uid: Option, } impl Widget for ThreadsPaneEntry { @@ -327,7 +328,7 @@ impl Widget for ThreadsPaneEntry { // Hit-test the parent area BEFORE propagating to children, so the inner // HtmlOrPlaintext (and its TextFlow / HtmlLink children) don't steal // FingerDown/Up — mirrors the pattern in rooms_list_entry.rs. - if let Some(thread_root_event_id) = self.thread_root_event_id.clone() { + if let (Some(thread_root_event_id), Some(owner_widget_uid)) = (self.thread_root_event_id.clone(), self.owner_widget_uid) { let area = self.view.area(); match event.hits(cx, area) { Hit::FingerDown(_) => { @@ -336,7 +337,7 @@ impl Widget for ThreadsPaneEntry { Hit::FingerUp(fe) if fe.is_over && fe.is_primary_hit() && fe.was_tap() => { log!("ThreadsPaneEntry: tap detected, emitting OpenThread({})", thread_root_event_id); cx.widget_action( - self.widget_uid(), + owner_widget_uid, ThreadsPaneAction::OpenThread(thread_root_event_id), ); } @@ -353,8 +354,9 @@ impl Widget for ThreadsPaneEntry { } impl ThreadsPaneEntry { - fn set_entry(&mut self, cx: &mut Cx, entry: &ThreadsPaneEntryInfo) { + fn set_entry(&mut self, cx: &mut Cx, entry: &ThreadsPaneEntryInfo, owner_widget_uid: WidgetUid) { self.thread_root_event_id = Some(entry.thread_root_event_id.clone()); + self.owner_widget_uid = Some(owner_widget_uid); self.html_or_plaintext(cx, ids!(title)).show_html(cx, &entry.title); self.label(cx, ids!(time)).set_text(cx, &entry.time); self.label(cx, ids!(subtitle)).set_text(cx, &entry.subtitle); @@ -363,9 +365,9 @@ impl ThreadsPaneEntry { } impl ThreadsPaneEntryRef { - fn set_entry(&self, cx: &mut Cx, entry: &ThreadsPaneEntryInfo) { + fn set_entry(&self, cx: &mut Cx, entry: &ThreadsPaneEntryInfo, owner_widget_uid: WidgetUid) { let Some(mut inner) = self.borrow_mut() else { return }; - inner.set_entry(cx, entry); + inner.set_entry(cx, entry, owner_widget_uid); } } @@ -481,6 +483,7 @@ impl Widget for ThreadsSlidingPane { } fn draw_walk(&mut self, cx: &mut Cx2d, scope: &mut Scope, walk: Walk) -> DrawStep { + let pane_widget_uid = self.widget_uid(); let Some(info) = self.info.as_ref() else { self.visible = false; return self.view.draw_walk(cx, scope, walk); @@ -520,7 +523,7 @@ impl Widget for ThreadsSlidingPane { while let Some(item_id) = list.next_visible_item(cx) { let Some(entry) = info.entries.get(item_id) else { continue }; let item = list.item(cx, item_id, id!(ThreadEntry)); - item.as_threads_pane_entry().set_entry(cx, entry); + item.as_threads_pane_entry().set_entry(cx, entry, pane_widget_uid); item.draw_all(cx, &mut Scope::empty()); } } diff --git a/src/home/search_messages.rs b/src/home/search_messages.rs index 793e91426..db6ba08e4 100644 --- a/src/home/search_messages.rs +++ b/src/home/search_messages.rs @@ -452,18 +452,19 @@ pub struct SearchResultItem { /// The matrix event this row represents. `None` until the row is bound /// to a [`MessageSearchHit`] via [`SearchResultItem::set_hit`]. #[rust] event_id: Option, + #[rust] owner_widget_uid: Option, } impl Widget for SearchResultItem { fn handle_event(&mut self, cx: &mut Cx, event: &Event, scope: &mut Scope) { self.view.handle_event(cx, event, scope); - let Some(event_id) = self.event_id.as_ref() else { return }; + let (Some(event_id), Some(owner_widget_uid)) = (self.event_id.as_ref(), self.owner_widget_uid) else { return }; let area = self.view.area(); if let Hit::FingerUp(fe) = event.hits(cx, area) { if fe.is_over && fe.is_primary_hit() && fe.was_tap() { cx.widget_action( - self.widget_uid(), + owner_widget_uid, SearchMessagesAction::JumpToEvent(event_id.clone()), ); } @@ -476,8 +477,9 @@ impl Widget for SearchResultItem { } impl SearchResultItem { - pub fn set_hit(&mut self, cx: &mut Cx, hit: &MessageSearchHit) { + pub fn set_hit(&mut self, cx: &mut Cx, hit: &MessageSearchHit, owner_widget_uid: WidgetUid) { self.event_id = Some(hit.event_id.clone()); + self.owner_widget_uid = Some(owner_widget_uid); self.label(cx, ids!(sender_label)).set_text(cx, &hit.sender_display); self.label(cx, ids!(ts_label)).set_text(cx, &hit.timestamp_display); self.label(cx, ids!(body_label)).set_text(cx, &hit.body_preview); @@ -689,6 +691,7 @@ impl Widget for SearchMessagesSlidingPane { self.refresh_status_display(cx); + let pane_widget_uid = self.widget_uid(); while let Some(widget) = self.view.draw_walk(cx, scope, walk).step() { let portal_list_ref = widget.as_portal_list(); let Some(mut list) = portal_list_ref.borrow_mut() else { continue }; @@ -698,7 +701,7 @@ impl Widget for SearchMessagesSlidingPane { let Some(hit) = self.hits.get(idx) else { continue }; let item = list.item(cx, idx, id!(result_item)); if let Some(mut row) = item.as_search_result_item().borrow_mut() { - row.set_hit(cx, hit); + row.set_hit(cx, hit, pane_widget_uid); } item.draw_all(cx, &mut Scope::empty()); }