Skip to content

Commit a78a4a0

Browse files
chrisbobbechrisbobbe
committed
messagesActions: Follow tapped /near/ links through message moves
Implementing the algorithm described in the issue and on CZO: zulip#5306 (comment) > When a user clicks a `/near/` link in a message, > > 1. Get the message, either from our local data structures or > (failing that) from a single-message fetch, using the API added > in zulip/zulip#21391. > 2. If the message is currently in the stream and topic encoded in > the `/near/` link, open the topic narrow for that stream/topic, > and scroll to the message*. > 3. If the message was *at some point* in the stream and topic > encoded in the `/near/` link, according to its `edit_history`, > open the topic narrow for the stream/topic that the message is > currently in, and scroll to the message*. > 4. Otherwise (i.e., the message was never in the stream/topic > encoded in the `/near/` link), open the topic narrow for the > stream/topic encoded in the link, and scroll to the message > "nearest" to the message ID*. > > *Actually opening a narrow scrolled to a specific message ID is > blocked on zulip#3604. It actually turned out quite easy to do the special handling for part 4, so that's included here. (The bit about blocking on zulip#3604 is still true.) Fixes: zulip#5306
1 parent 2d1aeca commit a78a4a0

File tree

2 files changed

+222
-2
lines changed

2 files changed

+222
-2
lines changed

‎src/message/messageSelectors.js

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,3 +78,47 @@ export const getFirstUnreadIdInNarrow: Selector<number | null, Narrow> = createS
7878
return firstUnread?.id ?? null;
7979
},
8080
);
81+
82+
/**
83+
* True just if we know the message has, or had, the given topic.
84+
*
85+
* Gives null if it doesn't have the topic now, but we can't access
86+
* the message's edit history to check in the past.
87+
*/
88+
export function hasMessageEverHadTopic(message: Message, topic: string): boolean | null {
89+
if (message.subject === topic) {
90+
return true;
91+
}
92+
93+
// See comments on Message['edit_history'] for the values it takes
94+
// and what they mean.
95+
if (message.edit_history === null) {
96+
return null;
97+
}
98+
if (message.edit_history === undefined) {
99+
return false;
100+
}
101+
return message.edit_history.findIndex(messageEdit => topic === messageEdit.prev_topic) >= 0;
102+
}
103+
104+
/**
105+
* True just if we know the message is, or was, in the given stream.
106+
*
107+
* Gives null if it's not in the stream now, but we can't access the
108+
* message's edit history to check in the past.
109+
*/
110+
export function hasMessageEverBeenInStream(message: Message, streamId: number): boolean | null {
111+
if (message.stream_id === streamId) {
112+
return true;
113+
}
114+
115+
// See comments on Message['edit_history'] for the values it takes
116+
// and what they mean.
117+
if (message.edit_history === null) {
118+
return null;
119+
}
120+
if (message.edit_history === undefined) {
121+
return false;
122+
}
123+
return message.edit_history.findIndex(messageEdit => streamId === messageEdit.prev_stream) >= 0;
124+
}

‎src/message/messagesActions.js

Lines changed: 178 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
/* @flow strict-local */
2+
import * as logging from '../utils/logging';
23
import * as NavigationService from '../nav/NavigationService';
34
import type { Narrow, ThunkAction } from '../types';
4-
import { getAuth } from '../selectors';
5+
import { getAuth, getRealm, getMessages, getZulipFeatureLevel } from '../selectors';
56
import { getNearOperandFromLink, getNarrowFromLink } from '../utils/internalLinks';
67
import { openLinkWithUserPreference } from '../utils/openLink';
78
import { navigateToChat } from '../nav/navActions';
@@ -10,6 +11,16 @@ import { getStreamsById, getStreamsByName } from '../subscriptions/subscriptionS
1011
import * as api from '../api';
1112
import { isUrlOnRealm } from '../utils/url';
1213
import { getOwnUserId } from '../users/userSelectors';
14+
import {
15+
isTopicNarrow,
16+
isStreamNarrow,
17+
topicNarrow,
18+
streamIdOfNarrow,
19+
topicOfNarrow,
20+
streamNarrow,
21+
caseNarrowDefault,
22+
} from '../utils/narrow';
23+
import { hasMessageEverBeenInStream, hasMessageEverHadTopic } from './messageSelectors';
1324

1425
/**
1526
* Navigate to the given narrow.
@@ -21,6 +32,171 @@ export const doNarrow =
2132
NavigationService.dispatch(navigateToChat(narrow));
2233
};
2334

35+
/**
36+
* Narrow to a /near/ link, possibly after reinterpreting it for a message move.
37+
*
38+
* It feels quite broken when a link is clearly meant to get you to a
39+
* specific message, but tapping it brings you to a narrow where the message
40+
* *used* to be but isn't anymore because it was moved to a new stream or
41+
* topic. This was #5306.
42+
*
43+
* This action, when it can, recognizes when that's about to happen and
44+
* instead narrows you to the message's current stream/topic.
45+
*
46+
* To do so, it obviously needs to know the message's current stream/topic.
47+
* If those can't be gotten from Redux, we ask the server. If the server
48+
* can't help us (gives an error), we can't help the user, so we won't
49+
* follow a move in that case.
50+
*
51+
* N.B.: Gives a bad experience when the request takes a long time. We
52+
* should fix that; see TODOs.
53+
*/
54+
const doNarrowNearLink =
55+
(narrow: Narrow, nearOperand: number): ThunkAction<Promise<void>> =>
56+
async (dispatch, getState) => {
57+
const state = getState();
58+
59+
const auth = getAuth(state);
60+
const messages = getMessages(state);
61+
const zulipFeatureLevel = getZulipFeatureLevel(state);
62+
const allowEditHistory = getRealm(state).allowEditHistory;
63+
64+
/**
65+
* Narrow to the /near/ link without reinterpreting it for a message move.
66+
*
67+
* Use this when the link is meant to find the specific message
68+
* identified by nearOperand, and:
69+
* - nearOperand refers to a message that wasn't moved outside the
70+
* narrow specified by the link, or
71+
* - nearOperand *might* refer to a message that was moved, but we don't
72+
* know; we've tried and failed to find out.
73+
*
74+
* Or, use this to insist on the traditional meaning of "near" before
75+
* the message-move feature: take the narrow's stream/topic/etc.
76+
* literally, and open to the message "nearest" the given ID (sent
77+
* around the same time), even if the message with that ID isn't
78+
* actually in the narrow [1].
79+
*
80+
* User docs on moving messages:
81+
* https://zulip.com/help/move-content-to-another-stream
82+
* https://zulip.com/help/move-content-to-another-topic
83+
*
84+
* [1] Tim points out, at
85+
* https://chat.zulip.org/#narrow/stream/101-design/topic/redirects.20from.20near.20links/near/1343095 :
86+
* "[…] useful for situations where you might replace an existing
87+
* search for `stream: 1/topic: 1/near: 15` with
88+
* `stream: 2/topic: 2/near: 15` in order to view what was happening
89+
* in another conversation at the same time as an existing
90+
* conversation."
91+
*/
92+
const noMove = () => {
93+
dispatch(doNarrow(narrow, nearOperand));
94+
};
95+
96+
const streamIdOperand =
97+
isStreamNarrow(narrow) || isTopicNarrow(narrow) ? streamIdOfNarrow(narrow) : null;
98+
const topicOperand = isTopicNarrow(narrow) ? topicOfNarrow(narrow) : null;
99+
100+
if (streamIdOperand === null && topicOperand === null) {
101+
// Message moves only happen by changing the stream and/or topic.
102+
noMove();
103+
return;
104+
}
105+
106+
// Grab the message and see if it was moved, so we can follow the move
107+
// if so.
108+
109+
// Try to get it from our local data to avoid a server round-trip…
110+
let message = messages.get(nearOperand);
111+
112+
// …but if we have to, go and ask the server.
113+
// TODO: Give feedback when the server round trip takes longer than
114+
// expected.
115+
// TODO: Let the user cancel the request so we don't force a doNarrow
116+
// after they've given up on tapping the link, and perhaps forgotten
117+
// about it. Like any request, this might take well over a minute to
118+
// resolve, or never resolve.
119+
// TODO: When these are fixed, remove warning in jsdoc.
120+
if (!message) {
121+
// TODO(server-5.0): Simplify.
122+
if (zulipFeatureLevel < 120) {
123+
// api.getSingleMessage won't give us the message's stream and
124+
// topic; see there. Hopefully the message wasn't moved.
125+
noMove();
126+
return;
127+
}
128+
try {
129+
message = await api.getSingleMessage(
130+
auth,
131+
{ message_id: nearOperand },
132+
zulipFeatureLevel,
133+
allowEditHistory,
134+
);
135+
} catch {
136+
// Hopefully the message, if it exists or ever existed, wasn't moved.
137+
noMove();
138+
return;
139+
}
140+
}
141+
142+
// The FL 120 condition on calling api.getSingleMessage should ensure
143+
// `message` isn't void.
144+
// TODO(server-5.0): Simplify away.
145+
if (!message) {
146+
logging.error('`message` from api.getSingleMessage unexpectedly falsy');
147+
noMove();
148+
return;
149+
}
150+
151+
if (message.type === 'private') {
152+
// A PM could never have been moved.
153+
noMove();
154+
return;
155+
}
156+
157+
if (
158+
(topicOperand === null || topicOperand === message.subject)
159+
&& (streamIdOperand === null || streamIdOperand === message.stream_id)
160+
) {
161+
// The message is still in the stream and/or topic in the link.
162+
noMove();
163+
return;
164+
}
165+
166+
if (
167+
(topicOperand !== null && hasMessageEverHadTopic(message, topicOperand) === false)
168+
|| (streamIdOperand !== null && hasMessageEverBeenInStream(message, streamIdOperand) === false)
169+
) {
170+
// The message was never in the narrow specified by the link. That'd
171+
// be an odd link to put in a message…anyway, perhaps we're meant to
172+
// use the traditional meaning of "near"; see noMove's jsdoc
173+
// for what that is.
174+
noMove();
175+
return;
176+
}
177+
// If we couldn't access the edit history in the checks above, assume
178+
// the message was moved. It's the likeliest explanation why its topic
179+
// and/or stream don't match the narrow link.
180+
181+
const { stream_id, subject } = message;
182+
183+
// Reinterpret the link's narrow with the message's current stream
184+
// and/or topic.
185+
dispatch(
186+
doNarrow(
187+
caseNarrowDefault(
188+
narrow,
189+
{
190+
stream: () => streamNarrow(stream_id),
191+
topic: () => topicNarrow(stream_id, subject),
192+
},
193+
() => narrow,
194+
),
195+
nearOperand,
196+
),
197+
);
198+
};
199+
24200
export const messageLinkPress =
25201
(href: string): ThunkAction<Promise<void>> =>
26202
async (dispatch, getState, { getGlobalSettings }) => {
@@ -43,7 +219,7 @@ export const messageLinkPress =
43219
return;
44220
}
45221

46-
dispatch(doNarrow(narrow, nearOperand));
222+
await dispatch(doNarrowNearLink(narrow, nearOperand));
47223
} else if (!isUrlOnRealm(href, auth.realm)) {
48224
openLinkWithUserPreference(href, getGlobalSettings());
49225
} else {

0 commit comments

Comments
 (0)