Skip to content

Commit 3daa56c

Browse files
committed
internalLinks [nfc]: Fix bug in getMessageIdFromLink
There were two things wrong here: - `url.includes('near')` was a pretty poor heuristic to decide if the narrow link has a "near" operator. It would give false positives if "near" appeared somewhere other than the operator-operand pair "/near/{message_id}", e.g., if it appeared in the realm's base URL (if `url` is an absolute URL string), or a stream or topic name in an operand. - Once we (unsoundly) decided that there was a "near" operator, we'd try to locate it in hashSegments by searching *all* of hashSegments, including ones that represent operands. This could be thrown off by an operand with the value "near", as in `topic/near`. NFC because we always drop the result of this function on the floor, after passing it to doNarrow. But we need this to start giving the right answer, for zulip#5306, "Follow /near/ links through topic moves/renames".
1 parent 7333ba0 commit 3daa56c

File tree

1 file changed

+18
-4
lines changed

1 file changed

+18
-4
lines changed

src/utils/internalLinks.js

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ export const getNarrowFromLink = (
227227

228228
/**
229229
* From a URL and realm with `isNarrowLink(url, realm) === true`, give
230-
* message_id if the URL ends in /near/{message_id}, otherwise give zero.
230+
* message_id if the URL has /near/{message_id}, otherwise give zero.
231231
*
232232
* This performs a call to `new URL` and therefore may take a fraction of a
233233
* millisecond. Avoid using in a context where it might be called more than
@@ -237,10 +237,24 @@ export const getMessageIdFromLink = (url: string, realm: URL): number => {
237237
// isNarrowLink(…) is true, by jsdoc, so this call is OK.
238238
const hashSegments = getHashSegmentsFromNarrowLink(url, realm);
239239

240-
// TODO: Very wrong; inspect `hashSegments` for this, not all of `url`.
241-
const isMessageLink = url.includes('near');
240+
// This and nearOperandIndex can simplify when we rename/repurpose
241+
// getHashSegmentsFromNarrowLink so it gives an array of
242+
// { negated, operator, operand } objects; see TODO there.
243+
const nearOperatorIndex = hashSegments.findIndex(
244+
(str, i) =>
245+
// This is a segment where we expect an operator to be specified.
246+
i % 2 === 0
247+
// The operator is 'near' and its meaning is not negated (`str` does
248+
// not start with "-").
249+
&& str === 'near',
250+
);
251+
if (nearOperatorIndex < 0) {
252+
return 0;
253+
}
242254

243-
return isMessageLink ? parseInt(hashSegments[hashSegments.lastIndexOf('near') + 1], 10) : 0;
255+
// We expect an operand to directly follow its operator.
256+
const nearOperandIndex = nearOperatorIndex + 1;
257+
return parseInt(hashSegments[nearOperandIndex], 10);
244258
};
245259

246260
export const getStreamTopicUrl = (

0 commit comments

Comments
 (0)