diff --git a/.eslintrc.yaml b/.eslintrc.yaml index 21038810cb8..540aa2e506a 100644 --- a/.eslintrc.yaml +++ b/.eslintrc.yaml @@ -75,12 +75,13 @@ rules: # of Airbnb's settings where we don't want a full repeal. # - # Tricky code. + # Tricky-looking code. These are all sometimes perfectly legitimate. no-bitwise: off no-confusing-arrow: off no-continue: off no-plusplus: off no-nested-ternary: off + no-control-regex: off # We prefer `let foo = undefined;` over `let foo;`, not vice versa. # diff --git a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt index 95d67d2964a..7cde10d8f44 100644 --- a/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt +++ b/android/app/src/main/java/com/zulipmobile/notifications/FcmMessage.kt @@ -55,7 +55,7 @@ sealed class Recipient { } /** A stream message. */ - data class Stream(val stream: String, val topic: String) : Recipient() + data class Stream(val streamName: String, val topic: String) : Recipient() } /** @@ -116,7 +116,7 @@ data class MessageFcmMessage( when (recipient) { is Recipient.Stream -> { putString("recipient_type", "stream") - putString("stream", recipient.stream) + putString("stream_name", recipient.streamName) putString("topic", recipient.topic) } is Recipient.GroupPm -> { diff --git a/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt b/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt index 8edb55c2584..7b46520d911 100644 --- a/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt +++ b/android/app/src/main/java/com/zulipmobile/notifications/NotificationUiManager.kt @@ -259,7 +259,7 @@ private fun extractConversationKey(fcmMessage: MessageFcmMessage): String { // So long as this does use the stream name, we use `\u0000` as the delimiter because // it's the one character not allowed in Zulip stream names. // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.) - is Recipient.Stream -> "stream:${fcmMessage.recipient.stream}\u0000${fcmMessage.recipient.topic}" + is Recipient.Stream -> "stream:${fcmMessage.recipient.streamName}\u0000${fcmMessage.recipient.topic}" is Recipient.GroupPm -> "groupPM:${fcmMessage.recipient.pmUsers.toString()}" is Recipient.Pm -> "private:${fcmMessage.sender.id}" } @@ -321,7 +321,7 @@ private fun updateNotification( // group-PM threads (pending #5116) get titled with the latest sender, rather than // the first. messagingStyle.setConversationTitle(when (fcmMessage.recipient) { - is Recipient.Stream -> "#${fcmMessage.recipient.stream} > ${fcmMessage.recipient.topic}" + is Recipient.Stream -> "#${fcmMessage.recipient.streamName} > ${fcmMessage.recipient.topic}" // TODO(#5116): use proper title for GroupPM, for which we will need // to have a way to get names of PM users here. is Recipient.GroupPm -> context.resources.getQuantityString( diff --git a/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt b/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt index a875843ef84..3efd95471d9 100644 --- a/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt +++ b/android/app/src/test/java/com/zulipmobile/notifications/FcmMessageTest.kt @@ -127,7 +127,7 @@ class MessageFcmMessageTest : FcmMessageTestBase() { ), zulipMessageId = 12345, recipient = Recipient.Stream( - stream = Example.stream["stream"]!!, + streamName = Example.stream["stream"]!!, topic = Example.stream["topic"]!! ), content = Example.stream["content"]!!, diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 4417800d95d..1c01f95ccc1 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -47,12 +47,7 @@ import rootReducer from '../../boot/reducers'; import { authOfAccount } from '../../account/accountMisc'; import { HOME_NARROW } from '../../utils/narrow'; import type { BackgroundData } from '../../webview/MessageList'; -import { - getSettings, - getStreamsById, - getStreamsByName, - getSubscriptionsById, -} from '../../selectors'; +import { getSettings, getStreamsById, getSubscriptionsById } from '../../selectors'; /* ======================================================================== * Utilities @@ -269,6 +264,10 @@ export const stream: Stream = makeStream({ name: 'a stream', description: 'An example stream.', }); +export const otherStream: Stream = makeStream({ + name: 'another stream', + description: 'Another example stream.', +}); /** A subscription, by default to eg.stream. */ export const makeSubscription = ( @@ -292,6 +291,9 @@ export const makeSubscription = ( /** A subscription to eg.stream. */ export const subscription: Subscription = makeSubscription(); +/** A subscription to eg.otherStream. */ +export const otherSubscription: Subscription = makeSubscription({ stream: otherStream }); + /* ======================================================================== * Messages */ @@ -572,8 +574,8 @@ export const plusReduxState: GlobalState & PerAccountState = reduxState({ realm: { ...baseReduxState.realm, user_id: selfUser.user_id, email: selfUser.email }, // TODO add crossRealmBot users: [selfUser, otherUser, thirdUser], - streams: [stream], - subscriptions: [subscription], + streams: [stream, otherStream], + subscriptions: [subscription, otherSubscription], }); /** @@ -845,7 +847,6 @@ export const backgroundData: BackgroundData = deepFreeze({ mutedUsers: Immutable.Map(), ownUser: selfUser, streams: getStreamsById(baseReduxState), - streamsByName: getStreamsByName(baseReduxState), subscriptions: getSubscriptionsById(baseReduxState), unread: baseReduxState.unread, theme: 'default', diff --git a/src/autocomplete/StreamAutocomplete.js b/src/autocomplete/StreamAutocomplete.js index 46035682826..066a4a99509 100644 --- a/src/autocomplete/StreamAutocomplete.js +++ b/src/autocomplete/StreamAutocomplete.js @@ -6,7 +6,7 @@ import { FlatList } from 'react-native'; import { useSelector } from '../react-redux'; import { Popup } from '../common'; -import { getSubscribedStreams } from '../subscriptions/subscriptionSelectors'; +import { getSubscriptions } from '../directSelectors'; import StreamItem from '../streams/StreamItem'; type Props = $ReadOnly<{| @@ -16,7 +16,7 @@ type Props = $ReadOnly<{| export default function StreamAutocomplete(props: Props): Node { const { filter, onAutocomplete } = props; - const subscriptions = useSelector(getSubscribedStreams); + const subscriptions = useSelector(getSubscriptions); const handleStreamItemAutocomplete = useCallback( (streamId: number, streamName: string): void => { diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index 23eb933f40e..11ad875efb6 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -138,7 +138,7 @@ export default function ChatScreen(props: Props): Node { const content = editMessage.content !== message ? message : undefined; const subject = caseNarrowDefault( destinationNarrow, - { topic: (stream, topic) => (topic !== editMessage.topic ? topic : undefined) }, + { topic: (streamId, topic) => (topic !== editMessage.topic ? topic : undefined) }, () => undefined, ); diff --git a/src/chat/MarkAsReadButton.js b/src/chat/MarkAsReadButton.js index c0b124669d0..f2df6965a5d 100644 --- a/src/chat/MarkAsReadButton.js +++ b/src/chat/MarkAsReadButton.js @@ -8,14 +8,14 @@ import { createStyleSheet } from '../styles'; import { useSelector } from '../react-redux'; import { ZulipButton } from '../common'; import * as api from '../api'; -import { getAuth, getStreams, getOwnUserId } from '../selectors'; +import { getAuth, getOwnUserId } from '../selectors'; import { getUnread, getUnreadIdsForPmNarrow } from '../unread/unreadModel'; import { isHomeNarrow, isStreamNarrow, isTopicNarrow, isPmNarrow, - streamNameOfNarrow, + streamIdOfNarrow, topicOfNarrow, } from '../utils/narrow'; @@ -35,7 +35,6 @@ type Props = $ReadOnly<{| export default function MarkAsReadButton(props: Props): Node { const { narrow } = props; const auth = useSelector(getAuth); - const streams = useSelector(getStreams); const unread = useSelector(getUnread); const ownUserId = useSelector(getOwnUserId); @@ -44,20 +43,12 @@ export default function MarkAsReadButton(props: Props): Node { }, [auth]); const markStreamAsRead = useCallback(() => { - const streamName = streamNameOfNarrow(narrow); - const stream = streams.find(s => s.name === streamName); - if (stream) { - api.markStreamAsRead(auth, stream.stream_id); - } - }, [auth, narrow, streams]); + api.markStreamAsRead(auth, streamIdOfNarrow(narrow)); + }, [auth, narrow]); const markTopicAsRead = useCallback(() => { - const streamName = streamNameOfNarrow(narrow); - const stream = streams.find(s => s.name === streamName); - if (stream) { - api.markTopicAsRead(auth, stream.stream_id, topicOfNarrow(narrow)); - } - }, [auth, narrow, streams]); + api.markTopicAsRead(auth, streamIdOfNarrow(narrow), topicOfNarrow(narrow)); + }, [auth, narrow]); const markPmAsRead = useCallback(() => { // The message IDs come from our unread-messages data, which is diff --git a/src/chat/__tests__/fetchingReducer-test.js b/src/chat/__tests__/fetchingReducer-test.js index 85294e84f73..2cff2ba1145 100644 --- a/src/chat/__tests__/fetchingReducer-test.js +++ b/src/chat/__tests__/fetchingReducer-test.js @@ -41,16 +41,17 @@ describe('fetchingReducer', () => { [HOME_NARROW_STR]: { older: false, newer: false }, }); + const narrow = streamNarrow(eg.stream.stream_id); const action = deepFreeze({ type: MESSAGE_FETCH_START, - narrow: streamNarrow('some stream'), + narrow, numBefore: 10, numAfter: 0, }); const expectedState = { [HOME_NARROW_STR]: { older: false, newer: false }, - [keyFromNarrow(streamNarrow('some stream'))]: { older: true, newer: false }, + [keyFromNarrow(narrow)]: { older: true, newer: false }, }; const newState = fetchingReducer(initialState, action); diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index 5462b972db1..822ebcfc938 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -27,9 +27,9 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('narrowsReducer', () => { const privateNarrowStr = keyFromNarrow(pm1to1NarrowFromUser(eg.otherUser)); const groupNarrowStr = keyFromNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])); - const streamNarrowStr = keyFromNarrow(streamNarrow(eg.stream.name)); + const streamNarrowStr = keyFromNarrow(streamNarrow(eg.stream.stream_id)); const egTopic = eg.streamMessage().subject; - const topicNarrowStr = keyFromNarrow(topicNarrow(eg.stream.name, egTopic)); + const topicNarrowStr = keyFromNarrow(topicNarrow(eg.stream.stream_id, egTopic)); describe('EVENT_NEW_MESSAGE', () => { test('if not caught up in narrow, do not add message in home narrow', () => { diff --git a/src/chat/__tests__/narrowsSelectors-test.js b/src/chat/__tests__/narrowsSelectors-test.js index ce7664dc2a3..690acef7c9d 100644 --- a/src/chat/__tests__/narrowsSelectors-test.js +++ b/src/chat/__tests__/narrowsSelectors-test.js @@ -146,7 +146,7 @@ describe('getShownMessagesForNarrow', () => { }); describe('stream narrow', () => { - const narrow = streamNarrow(stream.name); + const narrow = streamNarrow(stream.stream_id); const makeState = extra => makeStateGeneral(message, narrow, extra); const shown = state => shownGeneral(state, narrow); @@ -171,7 +171,7 @@ describe('getShownMessagesForNarrow', () => { }); describe('topic narrow', () => { - const narrow = topicNarrow(stream.name, message.subject); + const narrow = topicNarrow(stream.stream_id, message.subject); const makeState = extra => makeStateGeneral(message, narrow, extra); const shown = state => shownGeneral(state, narrow); @@ -306,26 +306,28 @@ describe('getStreamInNarrow', () => { }); test('return subscription if stream in narrow is subscribed', () => { - const narrow = streamNarrow(stream1.name); + const narrow = streamNarrow(stream1.stream_id); expect(getStreamInNarrow(state, narrow)).toEqual(sub1); }); test('return stream if stream in narrow is not subscribed', () => { - const narrow = streamNarrow(stream3.name); + const narrow = streamNarrow(stream3.stream_id); expect(getStreamInNarrow(state, narrow)).toEqual({ ...stream3, in_home_view: true }); }); test('return NULL_SUBSCRIPTION if stream in narrow is not valid', () => { - const narrow = streamNarrow(stream4.name); + const narrow = streamNarrow(stream4.stream_id); expect(getStreamInNarrow(state, narrow)).toEqual(NULL_SUBSCRIPTION); }); test('return NULL_SUBSCRIPTION is narrow is not topic or stream', () => { expect(getStreamInNarrow(state, pm1to1NarrowFromUser(eg.otherUser))).toEqual(NULL_SUBSCRIPTION); - expect(getStreamInNarrow(state, topicNarrow(stream4.name, 'topic'))).toEqual(NULL_SUBSCRIPTION); + expect(getStreamInNarrow(state, topicNarrow(stream4.stream_id, 'topic'))).toEqual( + NULL_SUBSCRIPTION, + ); }); }); @@ -345,7 +347,7 @@ describe('isNarrowValid', () => { const state = eg.reduxState({ streams: [stream], }); - const narrow = streamNarrow(stream.name); + const narrow = streamNarrow(stream.stream_id); const result = isNarrowValid(state, narrow); @@ -356,7 +358,7 @@ describe('isNarrowValid', () => { const state = eg.reduxState({ streams: [], }); - const narrow = streamNarrow('nonexisting'); + const narrow = streamNarrow(eg.stream.stream_id); const result = isNarrowValid(state, narrow); @@ -369,7 +371,7 @@ describe('isNarrowValid', () => { const state = eg.reduxState({ streams: [stream], }); - const narrow = topicNarrow(stream.name, 'topic does not matter'); + const narrow = topicNarrow(stream.stream_id, 'topic does not matter'); const result = isNarrowValid(state, narrow); diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index e20baf422b0..8aa78a6dd62 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -17,7 +17,6 @@ import { getSubscriptions, getMessages, getMute, - getStreams, getOutbox, getFlags, } from '../directSelectors'; @@ -28,13 +27,14 @@ import { isMessageInNarrow, caseNarrowDefault, keyFromNarrow, - streamNameOfNarrow, caseNarrow, + streamIdOfNarrow, } from '../utils/narrow'; import { isTopicMuted } from '../mute/muteModel'; import { streamNameOfStreamMessage } from '../utils/recipient'; import { NULL_ARRAY, NULL_SUBSCRIPTION } from '../nullObjects'; import * as logging from '../utils/logging'; +import { getStreamsById, getSubscriptionsById } from '../selectors'; export const outboxMessagesForNarrow: Selector<$ReadOnlyArray, Narrow> = createSelector( (state, narrow) => narrow, @@ -196,20 +196,20 @@ export const getLastMessageId = (state: PerAccountState, narrow: Narrow): number // TODO: clean up what this returns; possibly to just `Stream` export const getStreamInNarrow: Selector = createSelector( (state, narrow) => narrow, - state => getSubscriptions(state), - state => getStreams(state), + state => getSubscriptionsById(state), + state => getStreamsById(state), (narrow, subscriptions, streams) => { if (!isStreamOrTopicNarrow(narrow)) { return NULL_SUBSCRIPTION; } - const streamName = streamNameOfNarrow(narrow); + const streamId = streamIdOfNarrow(narrow); - const subscription = subscriptions.find(x => x.name === streamName); + const subscription = subscriptions.get(streamId); if (subscription) { return subscription; } - const stream = streams.find(x => x.name === streamName); + const stream = streams.get(streamId); if (stream) { return { ...stream, @@ -223,14 +223,14 @@ export const getStreamInNarrow: Selector = createSelector( (state, narrow) => narrow, - state => getStreams(state), + state => getStreamsById(state), state => getAllUsersById(state), (narrow, streams, allUsersById) => caseNarrowDefault( narrow, { - stream: streamName => streams.find(s => s.name === streamName) !== undefined, - topic: streamName => streams.find(s => s.name === streamName) !== undefined, + stream: streamId => streams.get(streamId) !== undefined, + topic: streamId => streams.get(streamId) !== undefined, pm: ids => ids.every(id => allUsersById.get(id) !== undefined), }, () => true, diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index 64b021143ae..40160457be3 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -35,7 +35,7 @@ import { isStreamNarrow, isStreamOrTopicNarrow, isTopicNarrow, - streamNameOfNarrow, + streamIdOfNarrow, topicNarrow, topicOfNarrow, } from '../utils/narrow'; @@ -48,6 +48,7 @@ import { getAuth, getIsAdmin, getStreamInNarrow, + getStreamsById, getVideoChatProvider, getRealm, } from '../selectors'; @@ -71,6 +72,7 @@ type SelectorProps = {| videoChatProvider: VideoChatProvider | null, mandatoryTopics: boolean, stream: Subscription | {| ...Stream, in_home_view: boolean |}, + streamsById: Map, |}; type OuterProps = $ReadOnly<{| @@ -401,9 +403,9 @@ class ComposeBoxInner extends PureComponent { getDestinationNarrow = (): Narrow => { const { narrow, isEditing } = this.props; if (isStreamNarrow(narrow) || (isTopicNarrow(narrow) && isEditing)) { - const streamName = streamNameOfNarrow(narrow); - const topic = this.state.topic.trim(); - return topicNarrow(streamName, topic || apiConstants.NO_TOPIC_TOPIC); + const streamId = streamIdOfNarrow(narrow); + const topic = this.state.topic.trim() || apiConstants.NO_TOPIC_TOPIC; + return topicNarrow(streamId, topic); } invariant(isConversationNarrow(narrow), 'destination narrow must be conversation'); return narrow; @@ -542,6 +544,7 @@ class ComposeBoxInner extends PureComponent { isAnnouncementOnly, isSubscribed, stream, + streamsById, videoChatProvider, _, } = this.props; @@ -555,7 +558,7 @@ class ComposeBoxInner extends PureComponent { return ; } - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, allUsersById); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, allUsersById, streamsById); const style = { paddingBottom: insets.bottom, backgroundColor: 'hsla(0, 0%, 50%, 0.1)', @@ -676,6 +679,7 @@ const ComposeBox: ComponentType = compose( isAnnouncementOnly: getIsActiveStreamAnnouncementOnly(state, props.narrow), isSubscribed: getIsActiveStreamSubscribed(state, props.narrow), stream: getStreamInNarrow(state, props.narrow), + streamsById: getStreamsById(state), videoChatProvider: getVideoChatProvider(state), mandatoryTopics: getRealm(state).mandatoryTopics, })), diff --git a/src/compose/__tests__/getComposeInputPlaceholder-test.js b/src/compose/__tests__/getComposeInputPlaceholder-test.js index a661fa377f6..61351dc8ad6 100644 --- a/src/compose/__tests__/getComposeInputPlaceholder-test.js +++ b/src/compose/__tests__/getComposeInputPlaceholder-test.js @@ -9,14 +9,16 @@ import { pmNarrowFromUsersUnsafe, } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; +import { getStreamsById } from '../../selectors'; describe('getComposeInputPlaceholder', () => { const usersById = new Map([eg.selfUser, eg.otherUser, eg.thirdUser].map(u => [u.user_id, u])); const ownUserId = eg.selfUser.user_id; + const streamsById = getStreamsById(eg.plusReduxState); test('returns "Message @ThisPerson" object for person narrow', () => { const narrow = deepFreeze(pm1to1NarrowFromUser(eg.otherUser)); - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); expect(placeholder).toEqual({ text: 'Message {recipient}', values: { recipient: `@${eg.otherUser.full_name}` }, @@ -25,25 +27,28 @@ describe('getComposeInputPlaceholder', () => { test('returns "Jot down something" object for self narrow', () => { const narrow = deepFreeze(pm1to1NarrowFromUser(eg.selfUser)); - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); expect(placeholder).toEqual({ text: 'Jot down something' }); }); test('returns "Message #streamName" for stream narrow', () => { - const narrow = deepFreeze(streamNarrow('Denmark')); - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); - expect(placeholder).toEqual({ text: 'Message {recipient}', values: { recipient: '#Denmark' } }); + const narrow = deepFreeze(streamNarrow(eg.stream.stream_id)); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); + expect(placeholder).toEqual({ + text: 'Message {recipient}', + values: { recipient: `#${eg.stream.name}` }, + }); }); test('returns properly for topic narrow', () => { - const narrow = deepFreeze(topicNarrow('Denmark', 'Copenhagen')); - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); + const narrow = deepFreeze(topicNarrow(eg.stream.stream_id, 'Copenhagen')); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); expect(placeholder).toEqual({ text: 'Reply' }); }); test('returns "Message group" object for group narrow', () => { const narrow = deepFreeze(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])); - const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById); + const placeholder = getComposeInputPlaceholder(narrow, ownUserId, usersById, streamsById); expect(placeholder).toEqual({ text: 'Message group' }); }); }); diff --git a/src/compose/getComposeInputPlaceholder.js b/src/compose/getComposeInputPlaceholder.js index 8c9cd9915a9..2211cab8af8 100644 --- a/src/compose/getComposeInputPlaceholder.js +++ b/src/compose/getComposeInputPlaceholder.js @@ -1,11 +1,12 @@ /* @flow strict-local */ -import type { Narrow, UserId, UserOrBot, LocalizableText } from '../types'; +import type { Narrow, Stream, UserId, UserOrBot, LocalizableText } from '../types'; import { caseNarrowDefault } from '../utils/narrow'; export default ( narrow: Narrow, ownUserId: UserId, allUsersById: Map, + streamsById: Map, ): LocalizableText => caseNarrowDefault( narrow, @@ -30,10 +31,13 @@ export default ( values: { recipient: `@${user.full_name}` }, }; }, - stream: name => ({ - text: 'Message {recipient}', - values: { recipient: `#${name}` }, - }), + stream: streamId => { + const stream = streamsById.get(streamId); + if (!stream) { + return { text: 'Type a message' }; + } + return { text: 'Message {recipient}', values: { recipient: `#${stream.name}` } }; + }, topic: () => ({ text: 'Reply' }), }, () => ({ text: 'Type a message' }), diff --git a/src/drafts/__tests__/draftsReducer-test.js b/src/drafts/__tests__/draftsReducer-test.js index 46d88e412b3..d3a70d993d6 100644 --- a/src/drafts/__tests__/draftsReducer-test.js +++ b/src/drafts/__tests__/draftsReducer-test.js @@ -1,12 +1,14 @@ +// @flow strict-local import deepFreeze from 'deep-freeze'; import { NULL_OBJECT } from '../../nullObjects'; import draftsReducer from '../draftsReducer'; import { DRAFT_UPDATE } from '../../actionConstants'; import { topicNarrow, keyFromNarrow } from '../../utils/narrow'; +import * as eg from '../../__tests__/lib/exampleData'; describe('draftsReducer', () => { - const testNarrow = topicNarrow('denmark', 'denmark2'); + const testNarrow = topicNarrow(eg.stream.stream_id, 'denmark2'); const testNarrowStr = keyFromNarrow(testNarrow); describe('DRAFT_UPDATE', () => { @@ -70,7 +72,7 @@ describe('draftsReducer', () => { const action = { type: DRAFT_UPDATE, content: ' ', - narrow: topicNarrow('denmark', 'denmark2'), + narrow: testNarrow, }; const expectedState = {}; diff --git a/src/drafts/__tests__/draftsSelectors-test.js b/src/drafts/__tests__/draftsSelectors-test.js index ec07fd65f21..efe7d2bff12 100644 --- a/src/drafts/__tests__/draftsSelectors-test.js +++ b/src/drafts/__tests__/draftsSelectors-test.js @@ -6,7 +6,7 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('getDraftForNarrow', () => { test('return content of draft if exists', () => { - const narrow = topicNarrow(eg.stream.name, 'topic'); + const narrow = topicNarrow(eg.stream.stream_id, 'topic'); const state = eg.reduxState({ drafts: { [keyFromNarrow(narrow)]: 'content', @@ -19,14 +19,14 @@ describe('getDraftForNarrow', () => { }); test('return empty string if not exists', () => { - const narrow = topicNarrow(eg.stream.name, 'topic'); + const narrow = topicNarrow(eg.stream.stream_id, 'topic'); const state = eg.reduxState({ drafts: { [keyFromNarrow(narrow)]: 'content', }, }); - const draft = getDraftForNarrow(state, topicNarrow(eg.stream.name, 'topic1')); + const draft = getDraftForNarrow(state, topicNarrow(eg.stream.stream_id, 'topic1')); expect(draft).toEqual(''); }); diff --git a/src/message/__tests__/fetchActions-test.js b/src/message/__tests__/fetchActions-test.js index 521266f0215..7a5d3f8a52b 100644 --- a/src/message/__tests__/fetchActions-test.js +++ b/src/message/__tests__/fetchActions-test.js @@ -26,7 +26,7 @@ import * as logging from '../../utils/logging'; const mockStore = configureStore([thunk]); -const narrow = streamNarrow(eg.stream.name); +const narrow = streamNarrow(eg.stream.stream_id); const streamNarrowStr = keyFromNarrow(narrow); global.FormData = class FormData {}; diff --git a/src/message/__tests__/messageActions-test.js b/src/message/__tests__/messageActions-test.js index 625e54f85b5..1b9abe778f5 100644 --- a/src/message/__tests__/messageActions-test.js +++ b/src/message/__tests__/messageActions-test.js @@ -13,7 +13,7 @@ import type { Action } from '../../actionTypes'; const mockStore = configureStore([thunk]); -const streamNarrowObj = streamNarrow(eg.stream.name); +const streamNarrowObj = streamNarrow(eg.stream.stream_id); describe('messageActions', () => { describe('doNarrow', () => { diff --git a/src/message/__tests__/scrollStrategy-test.js b/src/message/__tests__/scrollStrategy-test.js index 0313ad8d6b1..cac58da74af 100644 --- a/src/message/__tests__/scrollStrategy-test.js +++ b/src/message/__tests__/scrollStrategy-test.js @@ -4,8 +4,9 @@ import { getScrollStrategy } from '../scrollStrategy'; import * as eg from '../../__tests__/lib/exampleData'; -const someNarrow = streamNarrow(eg.stream.name); -const anotherNarrow = streamNarrow(eg.makeStream().name); +const someNarrow = streamNarrow(eg.stream.stream_id); +const anotherStream = eg.makeStream(); +const anotherNarrow = streamNarrow(anotherStream.stream_id); describe('getScrollStrategy', () => { test('initial load positions at anchor (first unread)', () => { diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index d4238b98017..857e818ec4c 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -24,6 +24,7 @@ import { getFetchingForNarrow, getIsAdmin, getIdentity, + getStreamsById, } from '../selectors'; import config from '../config'; import { @@ -121,7 +122,11 @@ export const fetchMessages = (fetchArgs: {| await tryFetch(() => api.getMessages(getAuth(getState()), { ...fetchArgs, - narrow: apiNarrowOfNarrow(fetchArgs.narrow, getAllUsersById(getState())), + narrow: apiNarrowOfNarrow( + fetchArgs.narrow, + getAllUsersById(getState()), + getStreamsById(getState()), + ), useFirstUnread: fetchArgs.anchor === FIRST_UNREAD_ANCHOR, // TODO: don't use this; see #4203 }), ); @@ -147,14 +152,14 @@ export const fetchMessages = (fetchArgs: {| // Describe the narrow without sending sensitive data to Sentry. narrow: caseNarrow(fetchArgs.narrow, { - stream: name => 'stream', - topic: (streamName, topic) => 'topic', + stream: () => 'stream', + topic: () => 'topic', pm: ids => (ids.length > 1 ? 'pm (group)' : 'pm (1:1)'), home: () => 'all', starred: () => 'starred', mentioned: () => 'mentioned', allPrivate: () => 'all-pm', - search: query => 'search', + search: () => 'search', }), anchor: fetchArgs.anchor, @@ -336,7 +341,11 @@ export const fetchMessagesInNarrow = ( const fetchPrivateMessages = () => async (dispatch, getState) => { const auth = getAuth(getState()); const { messages, found_newest, found_oldest } = await api.getMessages(auth, { - narrow: apiNarrowOfNarrow(ALL_PRIVATE_NARROW, getAllUsersById(getState())), + narrow: apiNarrowOfNarrow( + ALL_PRIVATE_NARROW, + getAllUsersById(getState()), + getStreamsById(getState()), + ), anchor: LAST_MESSAGE_ANCHOR, numBefore: 100, numAfter: 0, diff --git a/src/nav/ChatNavBar.js b/src/nav/ChatNavBar.js index 78bbf6f74d4..6585cf61327 100644 --- a/src/nav/ChatNavBar.js +++ b/src/nav/ChatNavBar.js @@ -14,7 +14,7 @@ import Title from '../title/Title'; import NavBarBackButton from './NavBarBackButton'; import { getStreamColorForNarrow } from '../subscriptions/subscriptionSelectors'; import { foregroundColorFromBackground } from '../utils/color'; -import { caseNarrowDefault, streamNameOfNarrow, streamNarrow } from '../utils/narrow'; +import { caseNarrowDefault, streamIdOfNarrow, streamNarrow } from '../utils/narrow'; import { navigateToStream, navigateToAccountDetails, @@ -23,22 +23,23 @@ import { doNarrow, } from '../actions'; import * as NavigationService from './NavigationService'; -import { getStreams } from '../selectors'; +import { isNarrowValid as getIsNarrowValid } from '../selectors'; import NavButton from './NavButton'; function ExtraNavButtonStream(props): Node { const { color, narrow } = props; - const streams = useSelector(getStreams); + + const isNarrowValid = useSelector(state => getIsNarrowValid(state, narrow)); + if (!isNarrowValid) { + return null; + } + return ( { - const streamName = streamNameOfNarrow(narrow); - const stream = streams.find(x => x.name === streamName); - if (stream) { - NavigationService.dispatch(navigateToTopicList(stream.stream_id)); - } + NavigationService.dispatch(navigateToTopicList(streamIdOfNarrow(narrow))); }} /> ); @@ -47,32 +48,28 @@ function ExtraNavButtonStream(props): Node { function ExtraNavButtonTopic(props): Node { const { narrow, color } = props; const dispatch = useDispatch(); - const streams = useSelector(getStreams); const handlePress = useCallback(() => { - const streamName = streamNameOfNarrow(narrow); - const stream = streams.find(x => x.name === streamName); - if (stream) { - dispatch(doNarrow(streamNarrow(stream.name))); - } - }, [dispatch, narrow, streams]); + dispatch(doNarrow(streamNarrow(streamIdOfNarrow(narrow)))); + }, [dispatch, narrow]); return ; } function InfoNavButtonStream(props): Node { const { color, narrow } = props; - const streams = useSelector(getStreams); + + const isNarrowValid = useSelector(state => getIsNarrowValid(state, narrow)); + if (!isNarrowValid) { + return null; + } + return ( { - const streamName = streamNameOfNarrow(narrow); - const stream = streams.find(x => x.name === streamName); - if (stream) { - NavigationService.dispatch(navigateToStream(stream.stream_id)); - } + NavigationService.dispatch(navigateToStream(streamIdOfNarrow(narrow))); }} /> ); @@ -115,13 +112,13 @@ const ActionItems: ComponentType<{| +color: string, +narrow: Narrow |}> = props caseNarrowDefault( props.narrow, { - stream: streamName => ( + stream: () => ( <> ), - topic: (streamName, topic) => ( + topic: () => ( <> diff --git a/src/notification/__tests__/notification-test.js b/src/notification/__tests__/notification-test.js index 0b20be04d6a..6897f22c76e 100644 --- a/src/notification/__tests__/notification-test.js +++ b/src/notification/__tests__/notification-test.js @@ -20,19 +20,21 @@ describe('getNarrowFromNotificationData', () => { test('unknown notification data returns null', () => { // $FlowFixMe[incompatible-type]: actually validate APNs messages const notification: Notification = {}; - const narrow = getNarrowFromNotificationData(notification, new Map(), ownUserId); + const narrow = getNarrowFromNotificationData(notification, new Map(), new Map(), ownUserId); expect(narrow).toBe(null); }); test('recognizes stream notifications and returns topic narrow', () => { + const stream = eg.makeStream({ name: 'some stream' }); + const streamsByName = new Map([[stream.name, stream]]); const notification = { realm_uri, recipient_type: 'stream', - stream: 'some stream', + stream_name: 'some stream', topic: 'some topic', }; - const narrow = getNarrowFromNotificationData(notification, new Map(), ownUserId); - expect(narrow).toEqual(topicNarrow('some stream', 'some topic')); + const narrow = getNarrowFromNotificationData(notification, new Map(), streamsByName, ownUserId); + expect(narrow).toEqual(topicNarrow(stream.stream_id, 'some topic')); }); test('on notification for a private message returns a PM narrow', () => { @@ -43,7 +45,12 @@ describe('getNarrowFromNotificationData', () => { recipient_type: 'private', sender_email: eg.otherUser.email, }; - const narrow = getNarrowFromNotificationData(notification, allUsersByEmail, ownUserId); + const narrow = getNarrowFromNotificationData( + notification, + allUsersByEmail, + new Map(), + ownUserId, + ); expect(narrow).toEqual(pm1to1NarrowFromUser(eg.otherUser)); }); @@ -59,7 +66,12 @@ describe('getNarrowFromNotificationData', () => { const expectedNarrow = pmNarrowFromUsersUnsafe(users.slice(1)); - const narrow = getNarrowFromNotificationData(notification, allUsersByEmail, ownUserId); + const narrow = getNarrowFromNotificationData( + notification, + allUsersByEmail, + new Map(), + ownUserId, + ); expect(narrow).toEqual(expectedNarrow); }); @@ -78,9 +90,14 @@ describe('extract iOS notification data', () => { for (const [type, data] of objectEntries(barebones)) { test(`${type} notification`, () => { + const expected = (() => { + const { stream: stream_name = undefined, ...rest } = data; + return stream_name !== undefined ? { ...rest, stream_name } : data; + })(); + // barebones 1.9.0-style message is accepted const msg = data; - expect(verify(msg)).toEqual(msg); + expect(verify(msg)).toEqual(expected); // new(-ish) optional user_id is accepted and copied // TODO: Rewrite so modern-style payloads are the baseline, e.g., @@ -88,15 +105,15 @@ describe('extract iOS notification data', () => { // individual tests for supporting older-style payloads, and mark // those for future deletion, like with `TODO(1.9.0)`. const msg1 = { ...msg, user_id }; - expect(verify(msg1)).toEqual(msg1); + expect(verify(msg1)).toEqual({ ...expected, user_id }); // unused fields are not copied const msg2 = { ...msg, realm_id: 8675309 }; - expect(verify(msg2)).toEqual(msg); + expect(verify(msg2)).toEqual(expected); // unknown fields are ignored and not copied const msg2a = { ...msg, unknown_data: ['unknown_data'] }; - expect(verify(msg2a)).toEqual(msg); + expect(verify(msg2a)).toEqual(expected); }); } }); diff --git a/src/notification/extract.js b/src/notification/extract.js index e109a19b1b6..3fa3dd0209d 100644 --- a/src/notification/extract.js +++ b/src/notification/extract.js @@ -125,14 +125,14 @@ export const fromAPNsImpl = (rawData: ?JSONableDict): Notification | void => { }; if (recipient_type === 'stream') { - const { stream, topic } = zulip; - if (typeof stream !== 'string' || typeof topic !== 'string') { + const { stream: stream_name, topic } = zulip; + if (typeof stream_name !== 'string' || typeof topic !== 'string') { throw err('invalid'); } return { ...identity, recipient_type: 'stream', - stream, + stream_name, topic, }; } else { diff --git a/src/notification/index.js b/src/notification/index.js index 8fde87d68cc..3fce46b6b94 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -4,7 +4,16 @@ import PushNotificationIOS from '@react-native-community/push-notification-ios'; import type { PushNotificationEventName } from '@react-native-community/push-notification-ios'; import type { Notification } from './types'; -import type { Auth, Dispatch, GlobalDispatch, Account, Narrow, UserId, UserOrBot } from '../types'; +import type { + Auth, + Dispatch, + GlobalDispatch, + Account, + Narrow, + Stream, + UserId, + UserOrBot, +} from '../types'; import { topicNarrow, pm1to1NarrowFromUser, pmNarrowFromRecipients } from '../utils/narrow'; import type { JSONable, JSONableDict } from '../utils/jsonable'; import * as api from '../api'; @@ -119,6 +128,7 @@ export const getAccountFromNotificationData = ( export const getNarrowFromNotificationData = ( data: Notification, allUsersByEmail: Map, + streamsByName: Map, ownUserId: UserId, ): Narrow | null => { if (!data.recipient_type) { @@ -130,8 +140,28 @@ export const getNarrowFromNotificationData = ( return null; } + // TODO: If the notification is in an unknown stream, or a 1:1 PM from an + // unknown user, give a better error. (This can happen for the stream + // case if we were removed from the stream after the notification's + // message was sent. It can also happen if the user or stream was just + // created, and we haven't yet learned about it in the event queue; see + // e068771d7, which fixed this issue for group PMs.) + // + // This version just silently ignores the notification. + // + // A nicer version would navigate to ChatScreen for that unknown + // conversation, which would show InvalidNarrow (with its sensible error + // message) and whatever the notification did tell us about the + // stream/user: in particular, the stream name. + // + // But because Narrow objects don't carry stream names, doing that will + // require some alternate plumbing to pass the stream name through. For + // now, we skip dealing with that; this should be an uncommon case, so + // we settle for not crashing. + if (data.recipient_type === 'stream') { - return topicNarrow(data.stream, data.topic); + const stream = streamsByName.get(data.stream_name); + return (stream && topicNarrow(stream.stream_id, data.topic)) ?? null; } if (data.pm_users === undefined) { diff --git a/src/notification/notificationActions.js b/src/notification/notificationActions.js index ac92b2258f5..aeb2168649e 100644 --- a/src/notification/notificationActions.js +++ b/src/notification/notificationActions.js @@ -19,7 +19,7 @@ import { getAccountFromNotificationData, } from '.'; import type { Notification } from './types'; -import { getAuth } from '../selectors'; +import { getAuth, getStreamsByName } from '../selectors'; import { getGlobalSession, getAccounts } from '../directSelectors'; import { GOT_PUSH_TOKEN, ACK_PUSH_TOKEN, UNACK_PUSH_TOKEN } from '../actionConstants'; import { identityOfAccount, authOfAccount } from '../account/accountMisc'; @@ -76,6 +76,7 @@ export const narrowToNotification = (data: ?Notification): GlobalThunkAction, + streamsById: Map, allUsersById: Map, ownUser: UserOrBot, ): DataFromNarrow => @@ -144,8 +144,8 @@ const outboxPropertiesForNarrow = ( subject: '', }), - topic: (streamName, topic) => { - const stream = streamsByName.get(streamName); + topic: (streamId, topic) => { + const stream = streamsById.get(streamId); invariant(stream, 'narrow must be known stream'); return { type: 'stream', @@ -185,7 +185,7 @@ export const addToOutbox = ( isSent: false, ...outboxPropertiesForNarrow( destinationNarrow, - getStreamsByName(state), + getStreamsById(state), getAllUsersById(state), ownUser, ), diff --git a/src/sharing/ShareToStream.js b/src/sharing/ShareToStream.js index a8c2f7bca57..2ec0ce047c2 100644 --- a/src/sharing/ShareToStream.js +++ b/src/sharing/ShareToStream.js @@ -74,14 +74,14 @@ class ShareToStreamInner extends React.Component { }; focusTopic = () => { - const { streamName, streamId } = this.state; + const { streamId } = this.state; const { dispatch } = this.props; if (streamId !== null) { // We have an actual stream selected. Fetch its recent topic names. // TODO: why do we do this? `TopicAutocomplete` does the same thing, // with an effect that reruns when the stream changes. - const narrow = streamNarrow(streamName); + const narrow = streamNarrow(streamId); dispatch(fetchTopicsForStream(narrow)); } // If what's entered in the stream-name field isn't an actual stream, @@ -147,7 +147,7 @@ class ShareToStreamInner extends React.Component { render() { const { sharedData } = this.props.route.params; const { streamName, streamId, topic, isStreamFocused, isTopicFocused } = this.state; - const narrow = streamId !== null ? streamNarrow(streamName) : null; + const narrow = streamId !== null ? streamNarrow(streamId) : null; const sendTo = { streamName, /* $FlowFixMe[incompatible-cast]: ShareWrapper will only look at this diff --git a/src/sharing/ShareWrapper.js b/src/sharing/ShareWrapper.js index e5e84953f35..5a32721bcf8 100644 --- a/src/sharing/ShareWrapper.js +++ b/src/sharing/ShareWrapper.js @@ -210,8 +210,8 @@ class ShareWrapperInner extends React.Component { } case 'stream': { - const { streamName } = sendTo; - const narrow = streamNarrow(streamName); + const { streamId } = sendTo; + const narrow = streamNarrow(streamId); NavigationService.dispatch(replaceWithChat(narrow)); break; } diff --git a/src/storage/__tests__/migrations-test.js b/src/storage/__tests__/migrations-test.js index 2c516de1dae..a1ceeed6dc8 100644 --- a/src/storage/__tests__/migrations-test.js +++ b/src/storage/__tests__/migrations-test.js @@ -59,8 +59,8 @@ describe('migrations', () => { ], }; - // What `base` becomes after all migrations. - const endBase = { + // What `base` becomes after migrations up through 37. + const base37 = { migrations: { version: 37 }, accounts: [ { @@ -84,14 +84,20 @@ describe('migrations', () => { }, }; + // What `base` becomes after all migrations. + const endBase = { + ...base37, + migrations: { version: 39 }, + }; + for (const [desc, before, after] of [ // Test the behavior with no migration state, which doesn't apply any // of the specific migrations. - ['empty state -> just store version', {}, { migrations: { version: 37 } }], + ['empty state -> just store version', {}, { migrations: endBase.migrations }], [ 'no migration state -> just store version, leave everything else', { nonsense: [1, 2, 3] }, - { migrations: { version: 37 }, nonsense: [1, 2, 3] }, + { migrations: endBase.migrations, nonsense: [1, 2, 3] }, ], // Test the whole sequence all together. This covers many of the @@ -144,9 +150,12 @@ describe('migrations', () => { migrations: { version: 21 }, drafts: { 'pm:d:12:other@example.com': 'text', 'topic:s:general\x00stuff': 'other text' }, }, - { ...endBase, drafts: { 'pm:12': 'text', 'topic:s:general\x00stuff': 'other text' } }, - // NB the original version of this migration was buggy; it resulted in: - // { ...endBase, drafts: { 'topic:s:general\x00stuff': 'other text' } }, // WRONG + // This migration itself leaves the `topic:` draft in place. But it + // gets dropped later by migration 38. + { ...endBase, drafts: { 'pm:12': 'text' } }, + // NB the original version of this migration was buggy; it would drop + // the PM drafts entirely, so this test case would end up as: + // { ...endBase, drafts: {} }, // WRONG // Should have written tests for it the first time. :-) ], [ @@ -204,6 +213,34 @@ describe('migrations', () => { { ...base15, settings: { ...base15.settings, doNotMarkMessagesAsRead: true } }, { ...endBase, settings: { ...endBase.settings, doNotMarkMessagesAsRead: true } }, ], + [ + 'check 38', + { + ...base37, + drafts: { + 'topic:s:general\x00stuff': 'text', + 'stream:s:general': 'more text', + 'pm:12': 'pm text', + }, + }, + { ...endBase, drafts: { 'pm:12': 'pm text' } }, + ], + [ + 'check 39', + { + ...base37, + migrations: { version: 38 }, + drafts: { + 'topic:d:8:general\x00stuff': 'text', + 'stream:d:8:general': 'more text', + 'pm:12': 'pm text', + }, + }, + { + ...endBase, + drafts: { 'topic:8:stuff': 'text', 'stream:8': 'more text', 'pm:12': 'pm text' }, + }, + ], ]) { /* eslint-disable no-loop-func */ test(desc, async () => { diff --git a/src/storage/migrations.js b/src/storage/migrations.js index 3ad1f4dbda9..3eaa9afb58f 100644 --- a/src/storage/migrations.js +++ b/src/storage/migrations.js @@ -350,6 +350,31 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} = }, }), + // Change format of keys representing stream and topic narrows, adding IDs. + '38': state => ({ + ...state, + drafts: objectFromEntries( + // Just drop drafts for stream and topic narrows, for the same reasons + // as for PM narrows in migration 21 above. + Object.keys(state.drafts) + .filter(key => !key.startsWith('stream:') && !key.startsWith('topic:')) + .map(key => [key, state.drafts[key]]), + ), + }), + + // Change format of keys representing stream/topic narrows, dropping names. + '39': state => ({ + ...state, + drafts: objectFromEntries( + Object.keys(state.drafts).map(key => [ + key + .replace(/^stream:d:(\d+):.*/s, 'stream:$1') + .replace(/^topic:d:(\d+):.*?\x00(.*)/s, 'topic:$1:$2'), + state.drafts[key], + ]), + ), + }), + // TIP: When adding a migration, consider just using `dropCache`. }; diff --git a/src/streams/SubscriptionsCard.js b/src/streams/SubscriptionsCard.js index af652523c70..0754849721b 100644 --- a/src/streams/SubscriptionsCard.js +++ b/src/streams/SubscriptionsCard.js @@ -12,7 +12,7 @@ import StreamList from './StreamList'; import { LoadingBanner } from '../common'; import { streamNarrow } from '../utils/narrow'; import { getUnreadByStream } from '../selectors'; -import { getSubscribedStreams } from '../subscriptions/subscriptionSelectors'; +import { getSubscriptions } from '../directSelectors'; import { doNarrow } from '../actions'; const styles = createStyleSheet({ @@ -29,13 +29,11 @@ type Props = $ReadOnly<{| export default function SubscriptionsCard(props: Props): Node { const dispatch = useDispatch(); - const subscriptions = useSelector(getSubscribedStreams); + const subscriptions = useSelector(getSubscriptions); const unreadByStream = useSelector(getUnreadByStream); const handleNarrow = useCallback( - (streamId: number, streamName: string) => { - dispatch(doNarrow(streamNarrow(streamName))); - }, + (streamId: number) => dispatch(doNarrow(streamNarrow(streamId))), [dispatch], ); diff --git a/src/streams/TopicItem.js b/src/streams/TopicItem.js index e1856b223c4..a7fe7b73b6d 100644 --- a/src/streams/TopicItem.js +++ b/src/streams/TopicItem.js @@ -17,7 +17,6 @@ import { getFlags, getSubscriptionsById, getStreamsById, - getStreamsByName, getOwnUser, } from '../selectors'; import { getUnread } from '../unread/unreadModel'; @@ -66,7 +65,6 @@ export default function TopicItem(props: Props): Node { auth: getAuth(state), mute: getMute(state), streams: getStreamsById(state), - streamsByName: getStreamsByName(state), subscriptions: getSubscriptionsById(state), unread: getUnread(state), ownUser: getOwnUser(state), diff --git a/src/subscriptions/StreamListCard.js b/src/subscriptions/StreamListCard.js index 5a55ab79243..1667de57ee4 100644 --- a/src/subscriptions/StreamListCard.js +++ b/src/subscriptions/StreamListCard.js @@ -55,9 +55,7 @@ export default function StreamListCard(props: Props): Node { ); const handleNarrow = useCallback( - (streamId: number, streamName: string) => { - dispatch(doNarrow(streamNarrow(streamName))); - }, + (streamId: number) => dispatch(doNarrow(streamNarrow(streamId))), [dispatch], ); diff --git a/src/subscriptions/__tests__/subscriptionSelectors-test.js b/src/subscriptions/__tests__/subscriptionSelectors-test.js index abdd4f73194..d65af205558 100644 --- a/src/subscriptions/__tests__/subscriptionSelectors-test.js +++ b/src/subscriptions/__tests__/subscriptionSelectors-test.js @@ -1,10 +1,9 @@ -import deepFreeze from 'deep-freeze'; +// @flow strict-local import { getStreamsById, getSubscriptionsById, getIsActiveStreamSubscribed, - getSubscribedStreams, getStreamColorForNarrow, } from '../subscriptionSelectors'; import { @@ -17,113 +16,64 @@ import { import * as eg from '../../__tests__/lib/exampleData'; describe('getStreamsById', () => { - test('returns empty object for an empty input', () => { - const state = deepFreeze({ - streams: [], - }); + test('returns empty map for an empty input', () => { + const state = eg.reduxState({ streams: [] }); expect(getStreamsById(state)).toEqual(new Map()); }); - test('returns an object with stream id as keys', () => { - const state = deepFreeze({ - streams: [{ stream_id: 1 }, { stream_id: 2 }], - }); - - // prettier-ignore - const expectedState = new Map([[1, { stream_id: 1 }], [2, { stream_id: 2 }]]); - - const streamsById = getStreamsById(state); - - expect(streamsById).toEqual(expectedState); + test('returns a map with stream id as keys', () => { + const state = eg.reduxState({ streams: [eg.stream, eg.otherStream] }); + expect(getStreamsById(state)).toEqual( + new Map([ + [eg.stream.stream_id, eg.stream], + [eg.otherStream.stream_id, eg.otherStream], + ]), + ); }); }); describe('getSubscriptionsById', () => { - test('returns empty object for an empty input', () => { - const state = deepFreeze({ - subscriptions: [], - }); + test('returns empty map for an empty input', () => { + const state = eg.reduxState({ subscriptions: [] }); expect(getSubscriptionsById(state)).toEqual(new Map()); }); - test('returns an object with stream id as keys', () => { - const state = deepFreeze({ - subscriptions: [{ stream_id: 1 }, { stream_id: 2 }], - }); - - // prettier-ignore - const expectedState = new Map([[1, { stream_id: 1 }], [2, { stream_id: 2 }]]); - - const subscriptionsById = getSubscriptionsById(state); - - expect(subscriptionsById).toEqual(expectedState); + test('returns a map with stream id as keys', () => { + const state = eg.reduxState({ subscriptions: [eg.subscription, eg.otherSubscription] }); + expect(getSubscriptionsById(state)).toEqual( + new Map([ + [eg.subscription.stream_id, eg.subscription], + [eg.otherSubscription.stream_id, eg.otherSubscription], + ]), + ); }); }); describe('getIsActiveStreamSubscribed', () => { - test('return true for narrows other than stream and topic', () => { - const state = deepFreeze({}); + const state = eg.reduxStatePlus({ subscriptions: [eg.subscription] }); + test('return true for narrows other than stream and topic', () => { expect(getIsActiveStreamSubscribed(state, HOME_NARROW)).toBe(true); }); test('return true if current narrowed stream is subscribed', () => { - const state = deepFreeze({ - subscriptions: [{ name: 'announce' }], - }); - - expect(getIsActiveStreamSubscribed(state, streamNarrow('announce'))).toBe(true); + const narrow = streamNarrow(eg.stream.stream_id); + expect(getIsActiveStreamSubscribed(state, narrow)).toBe(true); }); test('return false if current narrowed stream is not subscribed', () => { - const state = deepFreeze({ - subscriptions: [{ name: 'announce' }], - }); - - expect(getIsActiveStreamSubscribed(state, streamNarrow('all'))).toBe(false); + const narrow = streamNarrow(eg.otherStream.stream_id); + expect(getIsActiveStreamSubscribed(state, narrow)).toBe(false); }); test('return true if stream of current narrowed topic is subscribed', () => { - const state = deepFreeze({ - subscriptions: [{ name: 'announce' }], - }); - - expect(getIsActiveStreamSubscribed(state, topicNarrow('announce', 'news'))).toBe(true); + const narrow = topicNarrow(eg.stream.stream_id, 'news'); + expect(getIsActiveStreamSubscribed(state, narrow)).toBe(true); }); test('return false if stream of current narrowed topic is not subscribed', () => { - const state = deepFreeze({ - subscriptions: [{ name: 'announce' }], - }); - - expect(getIsActiveStreamSubscribed(state, topicNarrow('all', 'news'))).toBe(false); - }); -}); - -describe('getSubscribedStreams', () => { - test('get all subscribed streams', () => { - const state = deepFreeze({ - streams: [ - { stream_id: 1, name: 'all', description: 'stream for all' }, - { stream_id: 2, name: 'new announce', description: 'stream for announce' }, - { stream_id: 3, name: 'Denmark', description: 'Denmark is awesome' }, - { stream_id: 4, name: 'general', description: 'stream for general' }, - ], - subscriptions: [ - { stream_id: 1, name: 'all', color: '#001' }, - { stream_id: 2, name: 'announce', color: '#002' }, - { stream_id: 4, name: 'general', color: '#003' }, - ], - }); - - const expectedResult = [ - { stream_id: 1, name: 'all', color: '#001', description: 'stream for all' }, - { stream_id: 2, name: 'new announce', color: '#002', description: 'stream for announce' }, - { stream_id: 4, name: 'general', color: '#003', description: 'stream for general' }, - ]; - - const actualResult = getSubscribedStreams(state); - expect(actualResult).toEqual(expectedResult); + const narrow = topicNarrow(eg.otherStream.stream_id, 'news'); + expect(getIsActiveStreamSubscribed(state, narrow)).toBe(false); }); }); @@ -134,18 +84,19 @@ describe('getStreamColorForNarrow', () => { }); test('return stream color for stream and topic narrow', () => { - expect(getStreamColorForNarrow(state, streamNarrow(eg.stream.name))).toEqual(exampleColor); + const narrow = streamNarrow(eg.stream.stream_id); + expect(getStreamColorForNarrow(state, narrow)).toEqual(exampleColor); }); test('return null stream color for invalid stream or unknown subscriptions', () => { const unknownStream = eg.makeStream(); - expect(getStreamColorForNarrow(state, streamNarrow(unknownStream.name))).toEqual('gray'); + const narrow = streamNarrow(unknownStream.stream_id); + expect(getStreamColorForNarrow(state, narrow)).toEqual('gray'); }); test('return undefined for non topic/stream narrow', () => { expect(getStreamColorForNarrow(state, pm1to1NarrowFromUser(eg.otherUser))).toEqual(undefined); - expect( - getStreamColorForNarrow(state, pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])), - ).toEqual(undefined); + const narrow = pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]); + expect(getStreamColorForNarrow(state, narrow)).toEqual(undefined); }); }); diff --git a/src/subscriptions/subscriptionSelectors.js b/src/subscriptions/subscriptionSelectors.js index 2cebeaf49a1..f3440fca848 100644 --- a/src/subscriptions/subscriptionSelectors.js +++ b/src/subscriptions/subscriptionSelectors.js @@ -2,7 +2,7 @@ import { createSelector } from 'reselect'; import type { PerAccountState, Narrow, Selector, Stream, Subscription } from '../types'; -import { isStreamOrTopicNarrow, streamNameOfNarrow } from '../utils/narrow'; +import { isStreamOrTopicNarrow, streamIdOfNarrow } from '../utils/narrow'; import { getSubscriptions, getStreams } from '../directSelectors'; /** @@ -32,34 +32,17 @@ export const getSubscriptionsById: Selector> = createS new Map(subscriptions.map(subscription => [subscription.stream_id, subscription])), ); -export const getSubscriptionsByName: Selector> = createSelector( - getSubscriptions, - subscriptions => new Map(subscriptions.map(subscription => [subscription.name, subscription])), -); - export const getIsActiveStreamSubscribed: Selector = createSelector( (state, narrow) => narrow, - state => getSubscriptions(state), + state => getSubscriptionsById(state), (narrow, subscriptions) => { if (!isStreamOrTopicNarrow(narrow)) { return true; } - const streamName = streamNameOfNarrow(narrow); - - return subscriptions.find(sub => streamName === sub.name) !== undefined; + return subscriptions.get(streamIdOfNarrow(narrow)) !== undefined; }, ); -export const getSubscribedStreams: Selector<$ReadOnlyArray> = createSelector( - getStreams, - getSubscriptions, - (allStreams, allSubscriptions) => - allSubscriptions.map(subscription => ({ - ...subscription, - ...allStreams.find(stream => stream.stream_id === subscription.stream_id), - })), -); - /** * The stream with the given ID; throws if none. * @@ -80,14 +63,12 @@ export const getStreamForId = (state: PerAccountState, streamId: number): Stream export const getIsActiveStreamAnnouncementOnly: Selector = createSelector( (state, narrow) => narrow, - state => getStreams(state), + state => getStreamsById(state), (narrow, streams) => { if (!isStreamOrTopicNarrow(narrow)) { return false; } - const streamName = streamNameOfNarrow(narrow); - - const stream = streams.find(stream_ => streamName === stream_.name); + const stream = streams.get(streamIdOfNarrow(narrow)); return stream ? stream.is_announcement_only : false; }, ); @@ -102,7 +83,7 @@ export const getStreamColorForNarrow = (state: PerAccountState, narrow: Narrow): return undefined; } - const subscriptionsByName = getSubscriptionsByName(state); - const streamName = streamNameOfNarrow(narrow); - return subscriptionsByName.get(streamName)?.color ?? 'gray'; + const subscriptionsById = getSubscriptionsById(state); + const streamId = streamIdOfNarrow(narrow); + return subscriptionsById.get(streamId)?.color ?? 'gray'; }; diff --git a/src/topics/TopicListScreen.js b/src/topics/TopicListScreen.js index e3b3ce4fda7..9a6f5d834a0 100644 --- a/src/topics/TopicListScreen.js +++ b/src/topics/TopicListScreen.js @@ -26,8 +26,8 @@ export default function TopicListScreen(props: Props): Node { const [filter, setFilter] = useState(''); const handlePress = useCallback( - (streamId: number, streamName: string, topic: string) => { - dispatch(doNarrow(topicNarrow(streamName, topic))); + (streamId: number, _ignored_streamName: string, topic: string) => { + dispatch(doNarrow(topicNarrow(streamId, topic))); }, [dispatch], ); diff --git a/src/topics/__tests__/topicsSelectors-test.js b/src/topics/__tests__/topicsSelectors-test.js index 8dbd721137f..fc4edb8f4c5 100644 --- a/src/topics/__tests__/topicsSelectors-test.js +++ b/src/topics/__tests__/topicsSelectors-test.js @@ -23,7 +23,7 @@ describe('getTopicsForNarrow', () => { }, }); - const topics = getTopicsForNarrow(state, streamNarrow(stream.name)); + const topics = getTopicsForNarrow(state, streamNarrow(stream.stream_id)); expect(topics).toEqual(['hi', 'wow']); }); diff --git a/src/topics/topicActions.js b/src/topics/topicActions.js index b91259673b7..af8b700841c 100644 --- a/src/topics/topicActions.js +++ b/src/topics/topicActions.js @@ -2,8 +2,8 @@ import type { Narrow, Topic, PerAccountAction, ThunkAction, Outbox } from '../types'; import * as api from '../api'; import { INIT_TOPICS } from '../actionConstants'; -import { isStreamNarrow, streamNameOfNarrow } from '../utils/narrow'; -import { getAuth, getStreams } from '../selectors'; +import { isStreamNarrow, streamIdOfNarrow } from '../utils/narrow'; +import { getAuth } from '../selectors'; import { deleteOutboxMessage } from '../actions'; import { getOutbox } from '../directSelectors'; @@ -26,20 +26,10 @@ export const fetchTopicsForStream = (narrow: Narrow): ThunkAction> dispatch, getState, ) => { - const state = getState(); - if (!isStreamNarrow(narrow)) { return; } - const streamName = streamNameOfNarrow(narrow); - - const streams = getStreams(state); - // TODO (#4333): Look for the stream by its ID, not its name. - const stream = streams.find(sub => streamName === sub.name); - if (!stream) { - return; - } - dispatch(fetchTopics(stream.stream_id)); + dispatch(fetchTopics(streamIdOfNarrow(narrow))); }; export const deleteMessagesForTopic = ( diff --git a/src/topics/topicSelectors.js b/src/topics/topicSelectors.js index fe11baafcad..506d93d6f2d 100644 --- a/src/topics/topicSelectors.js +++ b/src/topics/topicSelectors.js @@ -1,35 +1,27 @@ /* @flow strict-local */ import { createSelector } from 'reselect'; -import type { Narrow, Selector, StreamsState, TopicExtended, TopicsState } from '../types'; -import { getMute, getStreams, getTopics } from '../directSelectors'; +import type { Narrow, Selector, TopicExtended, TopicsState } from '../types'; +import { getMute, getTopics } from '../directSelectors'; import { getUnread, getUnreadCountForTopic } from '../unread/unreadModel'; import { getStreamsById } from '../subscriptions/subscriptionSelectors'; import { NULL_ARRAY } from '../nullObjects'; -import { isStreamNarrow, streamNameOfNarrow } from '../utils/narrow'; +import { isStreamNarrow, streamIdOfNarrow } from '../utils/narrow'; import { isTopicMuted } from '../mute/muteModel'; export const getTopicsForNarrow: Selector<$ReadOnlyArray, Narrow> = createSelector( (state, narrow) => narrow, state => getTopics(state), - state => getStreams(state), - (narrow: Narrow, topics: TopicsState, streams: StreamsState) => { + (narrow: Narrow, topics: TopicsState) => { if (!isStreamNarrow(narrow)) { return NULL_ARRAY; } - const streamName = streamNameOfNarrow(narrow); + const streamId = streamIdOfNarrow(narrow); - // TODO (#4333): Look for the stream by its ID, not its name. One - // expected consequence of the current code is that - // `TopicAutocomplete` would stop showing any topics, if someone - // changed the stream name while you were looking at - // `TopicAutocomplete`. - const stream = streams.find(x => x.name === streamName); - if (!stream || !topics[stream.stream_id]) { + if (!topics[streamId]) { return NULL_ARRAY; } - - return topics[stream.stream_id].map(x => x.name); + return topics[streamId].map(x => x.name); }, ); diff --git a/src/unread/UnreadCards.js b/src/unread/UnreadCards.js index d532b8f102d..b1a470d187c 100644 --- a/src/unread/UnreadCards.js +++ b/src/unread/UnreadCards.js @@ -54,8 +54,8 @@ export default function UnreadCards(props: Props): Node { isPrivate={section.isPrivate} backgroundColor={section.color} unreadCount={section.unread} - onPress={(streamId: number, streamName: string) => { - setTimeout(() => dispatch(doNarrow(streamNarrow(streamName)))); + onPress={(streamId: number) => { + setTimeout(() => dispatch(doNarrow(streamNarrow(streamId)))); }} /> ) @@ -71,8 +71,8 @@ export default function UnreadCards(props: Props): Node { isMuted={section.isMuted || item.isMuted} isSelected={false} unreadCount={item.unread} - onPress={(streamId: number, streamName: string, topic: string) => { - setTimeout(() => dispatch(doNarrow(topicNarrow(streamName, topic)))); + onPress={(streamId: number, _ignored_streamName: string, topic: string) => { + setTimeout(() => dispatch(doNarrow(topicNarrow(streamId, topic)))); }} /> ) diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index 4995fa4fd40..6a482df707a 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -3,9 +3,9 @@ import { createSelector } from 'reselect'; import type { Narrow, Selector, UnreadStreamItem } from '../types'; import { caseInsensitiveCompareFunc } from '../utils/misc'; -import { getMute, getStreams } from '../directSelectors'; +import { getMute } from '../directSelectors'; import { getOwnUserId } from '../users/userSelectors'; -import { getSubscriptionsById } from '../subscriptions/subscriptionSelectors'; +import { getSubscriptionsById, getStreamsById } from '../subscriptions/subscriptionSelectors'; import { isTopicMuted } from '../mute/muteModel'; import { caseNarrow } from '../utils/narrow'; import { NULL_SUBSCRIPTION } from '../nullObjects'; @@ -222,7 +222,7 @@ export const getUnreadByHuddlesMentionsAndPMs: Selector = createSelector */ export const getUnreadCountForNarrow: Selector = createSelector( (state, narrow) => narrow, - state => getStreams(state), + state => getStreamsById(state), state => getOwnUserId(state), state => getUnreadTotal(state), state => getUnread(state), @@ -231,8 +231,8 @@ export const getUnreadCountForNarrow: Selector = createSelector( caseNarrow(narrow, { home: () => unreadTotal, - stream: name => { - const stream = streams.find(s => s.name === name); + stream: streamId => { + const stream = streams.get(streamId); if (!stream) { return 0; } @@ -241,20 +241,14 @@ export const getUnreadCountForNarrow: Selector = createSelector( unread.streams .get(stream.stream_id) ?.entrySeq() - .filterNot(([topic, _]) => isTopicMuted(name, topic, mute)) + .filterNot(([topic, _]) => isTopicMuted(stream.name, topic, mute)) .map(([_, msgIds]) => msgIds.size) .reduce((s, x) => s + x, 0) ?? 0 ); }, - topic: (streamName, topic) => { - const stream = streams.find(s => s.name === streamName); - if (!stream) { - return 0; - } - return getUnreadCountForTopic(unread, stream.stream_id, topic); - }, + topic: (streamId, topic) => getUnreadCountForTopic(unread, streamId, topic), pm: _ => getUnreadIdsForPmNarrow(unread, narrow, ownUserId).length, diff --git a/src/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js index 7fe73eccce2..638a3d6ef67 100644 --- a/src/utils/__tests__/internalLinks-test.js +++ b/src/utils/__tests__/internalLinks-test.js @@ -275,7 +275,7 @@ describe('getNarrowFromLink', () => { /* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectStream"] }] */ const expectStream = (operand, streams, expectedStream: null | Stream) => { expect(get(`#narrow/stream/${operand}`, streams)).toEqual( - expectedStream === null ? null : streamNarrow(expectedStream.name), + expectedStream === null ? null : streamNarrow(expectedStream.stream_id), ); }; @@ -320,7 +320,7 @@ describe('getNarrowFromLink', () => { const expectNameMatch = (operand, streamName) => { const stream = eg.makeStream({ name: streamName }); expect(get(`https://example.com/#narrow/stream/${operand}`, [stream])).toEqual( - streamNarrow(stream.name), + streamNarrow(stream.stream_id), ); }; expectNameMatch('jest', 'jest'); @@ -334,10 +334,10 @@ describe('getNarrowFromLink', () => { test('on old stream link, without realm info', () => { expect(get(`/#narrow/stream/${eg.stream.name}`, [eg.stream])).toEqual( - streamNarrow(eg.stream.name), + streamNarrow(eg.stream.stream_id), ); expect(get(`#narrow/stream/${eg.stream.name}`, [eg.stream])).toEqual( - streamNarrow(eg.stream.name), + streamNarrow(eg.stream.stream_id), ); }); }); @@ -346,7 +346,9 @@ describe('getNarrowFromLink', () => { test('basic', () => { const expectBasic = (operand, expectedTopic) => { const url = `#narrow/stream/${streamGeneral.stream_id}-general/topic/${operand}`; - expect(get(url, [streamGeneral])).toEqual(topicNarrow(streamGeneral.name, expectedTopic)); + expect(get(url, [streamGeneral])).toEqual( + topicNarrow(streamGeneral.stream_id, expectedTopic), + ); }; expectBasic('(no.20topic)', '(no topic)'); @@ -356,11 +358,11 @@ describe('getNarrowFromLink', () => { test('on old topic link, with dot-encoding', () => { expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/(no.20topic)`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, '(no topic)')); + ).toEqual(topicNarrow(eg.stream.stream_id, '(no topic)')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/google.2Ecom`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'google.com')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'google.com')); expect(() => get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/google.com`, [eg.stream]), @@ -368,23 +370,23 @@ describe('getNarrowFromLink', () => { expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/topic.20name`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'topic name')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'topic name')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/stream`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'stream')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'stream')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/topic`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'topic')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'topic')); }); test('on old topic link, without realm info', () => { expect(get(`/#narrow/stream/${eg.stream.name}/topic/topic`, [eg.stream])).toEqual( - topicNarrow(eg.stream.name, 'topic'), + topicNarrow(eg.stream.stream_id, 'topic'), ); expect(get(`#narrow/stream/${eg.stream.name}/topic/topic`, [eg.stream])).toEqual( - topicNarrow(eg.stream.name, 'topic'), + topicNarrow(eg.stream.stream_id, 'topic'), ); }); }); @@ -416,11 +418,11 @@ describe('getNarrowFromLink', () => { expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/test/near/1`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'test')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'test')); expect( get(`https://example.com/#narrow/stream/${eg.stream.name}/subject/test/near/1`, [eg.stream]), - ).toEqual(topicNarrow(eg.stream.name, 'test')); + ).toEqual(topicNarrow(eg.stream.stream_id, 'test')); }); }); diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 7664639f65f..2827cc4562d 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -23,9 +23,9 @@ import { STARRED_NARROW, MENTIONED_NARROW, keyFromNarrow, - streamNameOfNarrow, topicOfNarrow, caseNarrowPartial, + streamIdOfNarrow, } from '../narrow'; import type { Narrow, Message } from '../../types'; import * as eg from '../../__tests__/lib/exampleData'; @@ -61,8 +61,8 @@ describe('isPmNarrow', () => { describe('isStreamOrTopicNarrow', () => { test('check for stream or topic narrow', () => { expect(isStreamOrTopicNarrow(undefined)).toBe(false); - expect(isStreamOrTopicNarrow(streamNarrow(eg.stream.name))).toBe(true); - expect(isStreamOrTopicNarrow(topicNarrow(eg.stream.name, 'some topic'))).toBe(true); + expect(isStreamOrTopicNarrow(streamNarrow(eg.stream.stream_id))).toBe(true); + expect(isStreamOrTopicNarrow(topicNarrow(eg.stream.stream_id, 'some topic'))).toBe(true); expect(isStreamOrTopicNarrow(HOME_NARROW)).toBe(false); expect(isStreamOrTopicNarrow(pm1to1NarrowFromUser(eg.otherUser))).toBe(false); expect(isStreamOrTopicNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe( @@ -76,37 +76,37 @@ describe('specialNarrow', () => { test('only narrowing with the "is" operator is special narrow', () => { expect(isSpecialNarrow(undefined)).toBe(false); expect(isSpecialNarrow(HOME_NARROW)).toBe(false); - expect(isSpecialNarrow(streamNarrow(eg.stream.name))).toBe(false); + expect(isSpecialNarrow(streamNarrow(eg.stream.stream_id))).toBe(false); expect(isSpecialNarrow(STARRED_NARROW)).toBe(true); }); }); describe('streamNarrow', () => { test('narrows to messages from a specific stream', () => { - const narrow = streamNarrow(eg.stream.name); + const narrow = streamNarrow(eg.stream.stream_id); expect(isStreamNarrow(narrow)).toBeTrue(); - expect(streamNameOfNarrow(narrow)).toEqual(eg.stream.name); + expect(streamIdOfNarrow(narrow)).toEqual(eg.stream.stream_id); }); test('only narrow with operator of "stream" is a stream narrow', () => { expect(isStreamNarrow(undefined)).toBe(false); expect(isStreamNarrow(HOME_NARROW)).toBe(false); - expect(isStreamNarrow(streamNarrow(eg.stream.name))).toBe(true); + expect(isStreamNarrow(streamNarrow(eg.stream.stream_id))).toBe(true); }); }); describe('topicNarrow', () => { test('narrows to a specific topic within a specified stream', () => { - const narrow = topicNarrow(eg.stream.name, 'some topic'); + const narrow = topicNarrow(eg.stream.stream_id, 'some topic'); expect(isTopicNarrow(narrow)).toBeTrue(); - expect(streamNameOfNarrow(narrow)).toEqual(eg.stream.name); + expect(streamIdOfNarrow(narrow)).toEqual(eg.stream.stream_id); expect(topicOfNarrow(narrow)).toEqual('some topic'); }); test('only narrow with two items, one for stream, one for topic is a topic narrow', () => { expect(isTopicNarrow(undefined)).toBe(false); expect(isTopicNarrow(HOME_NARROW)).toBe(false); - expect(isTopicNarrow(topicNarrow(eg.stream.name, 'some topic'))).toBe(true); + expect(isTopicNarrow(topicNarrow(eg.stream.stream_id, 'some topic'))).toBe(true); }); }); @@ -127,12 +127,12 @@ describe('isMessageInNarrow', () => { ['a message', true, eg.streamMessage()], ]], - ['whole-stream narrow', streamNarrow(eg.stream.name), [ + ['whole-stream narrow', streamNarrow(eg.stream.stream_id), [ ['matching stream message', true, eg.streamMessage()], ['other-stream message', false, eg.streamMessage({ stream: otherStream })], ['PM', false, eg.pmMessage()], ]], - ['stream conversation', topicNarrow(eg.stream.name, 'cabbages'), [ + ['stream conversation', topicNarrow(eg.stream.stream_id, 'cabbages'), [ ['matching message', true, eg.streamMessage({ subject: 'cabbages' })], ['message in same stream but other topic', false, eg.streamMessage({ subject: 'kings' })], ['other-stream message', false, eg.streamMessage({ stream: otherStream })], @@ -253,8 +253,8 @@ describe('getNarrowsForMessage', () => { }, expectedNarrows: [ HOME_NARROW, - streamNarrow(eg.stream.name), - topicNarrow(eg.stream.name, 'myTopic'), + streamNarrow(eg.stream.stream_id), + topicNarrow(eg.stream.stream_id, 'myTopic'), ], }, { @@ -273,7 +273,11 @@ describe('getNarrowsForMessage', () => { ...eg.streamMessage({ stream: eg.stream }), subject: '', }, - expectedNarrows: [HOME_NARROW, streamNarrow(eg.stream.name), topicNarrow(eg.stream.name, '')], + expectedNarrows: [ + HOME_NARROW, + streamNarrow(eg.stream.stream_id), + topicNarrow(eg.stream.stream_id, ''), + ], }, { label: 'PM message with one person', @@ -323,7 +327,7 @@ describe('getNarrowForReply', () => { test('for stream message with nonempty topic, returns a topic narrow', () => { const message = eg.streamMessage(); - const expectedNarrow = topicNarrow(eg.stream.name, message.subject); + const expectedNarrow = topicNarrow(eg.stream.stream_id, message.subject); const actualNarrow = getNarrowForReply(message, eg.selfUser.user_id); @@ -333,8 +337,8 @@ describe('getNarrowForReply', () => { describe('keyFromNarrow+parseNarrow', () => { const baseNarrows = [ - ['whole stream', streamNarrow(eg.stream.name)], - ['stream conversation', topicNarrow(eg.stream.name, 'a topic')], + ['whole stream', streamNarrow(eg.stream.stream_id)], + ['stream conversation', topicNarrow(eg.stream.stream_id, 'a topic')], ['1:1 PM conversation, non-self', pm1to1NarrowFromUser(eg.otherUser)], ['self-1:1 conversation', pm1to1NarrowFromUser(eg.selfUser)], ['group-PM conversation', pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser])], @@ -348,19 +352,20 @@ describe('keyFromNarrow+parseNarrow', () => { // The only character not allowed in Zulip stream names is '\x00'. // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.) // Try approximately everything else. - /* eslint-disable no-control-regex */ const diverseCharacters = eg.diverseCharacters.replace(/\x00/g, ''); const htmlEntities = 'h & t & &lquo;ml"'; + const diverseCharactersStream = eg.makeStream({ name: diverseCharacters }); + const htmlEntitiesStream = eg.makeStream({ name: htmlEntities }); const awkwardNarrows = [ - ['whole stream about awkward characters', streamNarrow(diverseCharacters)], - ['whole stream about HTML entities', streamNarrow(htmlEntities)], + ['whole stream about awkward characters', streamNarrow(diverseCharactersStream.stream_id)], + ['whole stream about HTML entities', streamNarrow(htmlEntitiesStream.stream_id)], [ 'stream conversation about awkward characters', - topicNarrow(diverseCharacters, `regarding ${diverseCharacters}`), + topicNarrow(diverseCharactersStream.stream_id, `regarding ${diverseCharacters}`), ], [ 'stream conversation about HTML entities', - topicNarrow(htmlEntities, `regarding ${htmlEntities}`), + topicNarrow(htmlEntitiesStream.stream_id, `regarding ${htmlEntities}`), ], ['search narrow for awkward characters', SEARCH_NARROW(diverseCharacters)], ['search narrow for HTML entities', SEARCH_NARROW(htmlEntities)], diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index 71a3f1f50cd..2c07103b16f 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -119,18 +119,18 @@ export const decodeHashComponent = (string: string): string => { }; /** - * Parse the operand of a `stream` operator, returning a stream ID and name. + * Parse the operand of a `stream` operator. * * Return null if the operand doesn't match any known stream. */ -const parseStreamOperand = (operand, streamsById, streamsByName): null | [string, number] => { +const parseStreamOperand = (operand, streamsById, streamsByName): null | Stream => { // "New" (2018) format: ${stream_id}-${stream_name} . const match = /^([\d]+)(?:-.*)?$/.exec(operand); if (match) { const streamId = parseInt(match[1], 10); const stream = streamsById.get(streamId); if (stream) { - return [stream.name, streamId]; + return stream; } } @@ -139,7 +139,7 @@ const parseStreamOperand = (operand, streamsById, streamsByName): null | [string const streamName = decodeHashComponent(operand); const stream = streamsByName.get(streamName); if (stream) { - return [streamName, stream.stream_id]; + return stream; } // Not any stream we know. (Most likely this means a stream the user @@ -188,12 +188,12 @@ export const getNarrowFromLink = ( return pmNarrowFromRecipients(pmKeyRecipientsFromIds(ids, ownUserId)); } case 'topic': { - const streamNameAndId = parseStreamOperand(paths[1], streamsById, streamsByName); - return streamNameAndId && topicNarrow(streamNameAndId[0], parseTopicOperand(paths[3])); + const stream = parseStreamOperand(paths[1], streamsById, streamsByName); + return stream && topicNarrow(stream.stream_id, parseTopicOperand(paths[3])); } case 'stream': { - const streamNameAndId = parseStreamOperand(paths[1], streamsById, streamsByName); - return streamNameAndId && streamNarrow(streamNameAndId[0]); + const stream = parseStreamOperand(paths[1], streamsById, streamsByName); + return stream && streamNarrow(stream.stream_id); } case 'special': try { diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 22e405fb7b5..7c3c2d3335b 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -1,12 +1,11 @@ /* @flow strict-local */ import { makeUserId } from '../api/idTypes'; -import type { ApiNarrow, Message, Outbox, UserId, UserOrBot } from '../types'; +import type { ApiNarrow, Message, Outbox, Stream, UserId, UserOrBot } from '../types'; import { normalizeRecipientsAsUserIdsSansMe, pmKeyRecipientsFromMessage, recipientsOfPrivateMessage, - streamNameOfStreamMessage, type PmKeyRecipients, type PmKeyUsers, pmKeyRecipientsFromPmKeyUsers, @@ -34,8 +33,8 @@ import { * server, and `apiNarrowOfNarrow` for converting to it. */ export opaque type Narrow = - | {| type: 'stream', streamName: string |} - | {| type: 'topic', streamName: string, topic: string |} + | {| type: 'stream', streamId: number |} + | {| type: 'topic', streamId: number, topic: string |} | {| type: 'pm', userIds: PmKeyRecipients |} | {| type: 'search', query: string |} | {| type: 'all' | 'starred' | 'mentioned' | 'all-pm' |}; @@ -148,11 +147,11 @@ export const ALL_PRIVATE_NARROW: Narrow = specialNarrow('private'); export const ALL_PRIVATE_NARROW_STR: string = keyFromNarrow(ALL_PRIVATE_NARROW); -export const streamNarrow = (stream: string): Narrow => - Object.freeze({ type: 'stream', streamName: stream }); +export const streamNarrow = (streamId: number): Narrow => + Object.freeze({ type: 'stream', streamId }); -export const topicNarrow = (stream: string, topic: string): Narrow => - Object.freeze({ type: 'topic', streamName: stream, topic }); +export const topicNarrow = (streamId: number, topic: string): Narrow => + Object.freeze({ type: 'topic', streamId, topic }); export const SEARCH_NARROW = (query: string): Narrow => Object.freeze({ type: 'search', query }); @@ -162,8 +161,8 @@ type NarrowCases = {| starred: () => T, mentioned: () => T, allPrivate: () => T, - stream: (name: string) => T, - topic: (streamName: string, topic: string) => T, + stream: (streamId: number) => T, + topic: (streamId: number, topic: string) => T, search: (query: string) => T, |}; @@ -174,8 +173,8 @@ export function caseNarrow(narrow: Narrow, cases: NarrowCases): T { }; switch (narrow.type) { - case 'stream': return cases.stream(narrow.streamName); - case 'topic': return cases.topic(narrow.streamName, narrow.topic); + case 'stream': return cases.stream(narrow.streamId); + case 'topic': return cases.topic(narrow.streamId, narrow.topic); case 'pm': return cases.pm(narrow.userIds); case 'search': return cases.search(narrow.query); case 'all': return cases.home(); @@ -241,24 +240,25 @@ export function caseNarrowDefault( * channel (like an HTML attribute) that only allows certain codepoints, use * something like `base64Utf8Encode` to encode into the permitted subset. */ -// The arbitrary Unicode codepoints come from (a) stream names and topics, -// and (b) our use of `\x00` as a delimiter. +// The arbitrary Unicode codepoints come from topics. (These aren't +// *completely* arbitrary, but close enough that there's little to gain +// from relying on constraints on them.) export function keyFromNarrow(narrow: Narrow): string { - // The ":s" (for "string") in several of these is to keep them disjoint, - // out of an abundance of caution, from future keys that use numeric IDs. + // The ":s" (for "string") in several of these was to keep them disjoint, + // out of an abundance of caution, from later keys that use numeric IDs. // - // Similarly, ":d" ("dual") marks those with both numeric IDs and strings. + // Similarly, ":d" ("dual") marked those with both numeric IDs and strings. return caseNarrow(narrow, { // NB if you're changing any of these: be sure to do a migration. // Take a close look at migration 19 and any later related migrations. - stream: name => `stream:s:${name}`, - // '\x00' is the one character not allowed in Zulip stream names. - // (See `check_stream_name` in zulip.git:zerver/lib/streams.py.) - topic: (streamName, topic) => `topic:s:${streamName}\x00${topic}`, + // An earlier version had `stream:s:`. Another had `stream:d:`. + stream: streamId => `stream:${streamId}`, - // An earlier version had `pm:s:`. - // Another had `pm:d:`. + // An earlier version had `topic:s:`. Another had `topic:d:`. + topic: (streamId, topic) => `topic:${streamId}:${topic}`, + + // An earlier version had `pm:s:`. Another had `pm:d:`. pm: ids => `pm:${ids.join(',')}`, home: () => 'all', @@ -280,23 +280,21 @@ export const parseNarrow = (narrowStr: string): Narrow => { // invariant: tag + rest === narrowStr switch (tag) { case 'stream:': { - if (!rest.startsWith('s:')) { + if (!/^\d+$/.test(rest)) { throw makeError(); } - const name = rest.substr('s:'.length); - return streamNarrow(name); + return streamNarrow(Number.parseInt(rest, 10)); } case 'topic:': { - // The `/s` regexp flag means the `.` patterns match absolutely - // anything. By default they reject certain "newline" characters, - // which in principle could appear in stream names or topics. - // eslint-disable-next-line no-control-regex - const match = /^s:(.*?)\x00(.*)/s.exec(rest); + // The `/s` regexp flag means the `.` pattern matches absolutely + // anything. By default it rejects certain "newline" characters, + // which in principle could appear in topics. + const match = /^(\d+):(.*)/s.exec(rest); if (!match) { throw makeError(); } - return topicNarrow(match[1], match[2]); + return topicNarrow(Number.parseInt(match[1], 10), match[2]); } case 'pm:': { @@ -349,14 +347,14 @@ export const userIdsOfPmNarrow = (narrow: Narrow): PmKeyRecipients => caseNarrowPartial(narrow, { pm: ids => ids }); /** - * The stream name for a stream or topic narrow; else error. + * The stream ID for a stream or topic narrow; else error. * - * Most callers of this should probably be getting passed a stream name + * Most callers of this should probably be getting passed a stream ID * instead of a Narrow in the first place; or if they do handle other kinds * of narrows, should be using `caseNarrow`. */ -export const streamNameOfNarrow = (narrow: Narrow): string => - caseNarrowPartial(narrow, { stream: name => name, topic: streamName => streamName }); +export const streamIdOfNarrow = (narrow: Narrow): number => + caseNarrowPartial(narrow, { stream: id => id, topic: id => id }); /** * The topic for a topic narrow; else error. @@ -366,7 +364,7 @@ export const streamNameOfNarrow = (narrow: Narrow): string => * other kinds of narrows, should be using `caseNarrow`. */ export const topicOfNarrow = (narrow: Narrow): string => - caseNarrowPartial(narrow, { topic: (streamName, topic) => topic }); + caseNarrowPartial(narrow, { topic: (id, topic) => topic }); export const isPmNarrow = (narrow?: Narrow): boolean => !!narrow && caseNarrowDefault(narrow, { pm: () => true }, () => false); @@ -414,22 +412,28 @@ export const isConversationNarrow = (narrow: Narrow): boolean => export const apiNarrowOfNarrow = ( narrow: Narrow, allUsersById: Map, -): ApiNarrow => - caseNarrow(narrow, { - stream: streamName => [{ operator: 'stream', operand: streamName }], - topic: (streamName, topic) => [ - { operator: 'stream', operand: streamName }, + streamsById: Map, +): ApiNarrow => { + const get = (description, map: Map, key: K): V => { + const result = map.get(key); + if (result === undefined) { + throw new Error(`apiNarrowOfNarrow: missing ${description}`); + } + return result; + }; + + return caseNarrow(narrow, { + stream: streamId => [ + // TODO(server-2.1): just send stream ID instead + { operator: 'stream', operand: get('stream', streamsById, streamId).name }, + ], + topic: (streamId, topic) => [ + // TODO(server-2.1): just send stream ID instead + { operator: 'stream', operand: get('stream', streamsById, streamId).name }, { operator: 'topic', operand: topic }, ], pm: ids => { - const emails = []; - for (const id of ids) { - const email = allUsersById.get(id)?.email; - if (email === undefined) { - throw new Error('apiNarrowOfNarrow: missing user'); - } - emails.push(email); - } + const emails = ids.map(id => get('user', allUsersById, id).email); // TODO(server-2.1): just send IDs instead return [{ operator: 'pm-with', operand: emails.join(',') }]; }, @@ -439,6 +443,7 @@ export const apiNarrowOfNarrow = ( mentioned: () => [{ operator: 'is', operand: 'mentioned' }], allPrivate: () => [{ operator: 'is', operand: 'private' }], }); +}; /** * True just if the given message is part of the given narrow. @@ -462,11 +467,9 @@ export const isMessageInNarrow = ( ): boolean => caseNarrow(narrow, { home: () => true, - stream: name => message.type === 'stream' && name === streamNameOfStreamMessage(message), - topic: (streamName, topic) => - message.type === 'stream' - && streamName === streamNameOfStreamMessage(message) - && topic === message.subject, + stream: streamId => message.type === 'stream' && streamId === message.stream_id, + topic: (streamId, topic) => + message.type === 'stream' && streamId === message.stream_id && topic === message.subject, pm: ids => { if (message.type !== 'private') { return false; @@ -532,9 +535,8 @@ export const getNarrowsForMessage = ( result.push(ALL_PRIVATE_NARROW); result.push(pmNarrowFromRecipients(pmKeyRecipientsFromMessage(message, ownUserId))); } else { - const streamName = streamNameOfStreamMessage(message); - result.push(topicNarrow(streamName, message.subject)); - result.push(streamNarrow(streamName)); + result.push(topicNarrow(message.stream_id, message.subject)); + result.push(streamNarrow(message.stream_id)); } if (flags.includes('mentioned') || flags.includes('wildcard_mentioned')) { @@ -561,7 +563,6 @@ export const getNarrowForReply = (message: Message | Outbox, ownUserId: UserId): if (message.type === 'private') { return pmNarrowFromRecipients(pmKeyRecipientsFromMessage(message, ownUserId)); } else { - const streamName = streamNameOfStreamMessage(message); - return topicNarrow(streamName, message.subject); + return topicNarrow(message.stream_id, message.subject); } }; diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 248c23e49a6..f2d76d36e0e 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -47,7 +47,6 @@ import { getGlobalSettings, getSubscriptionsById, getStreamsById, - getStreamsByName, getRealm, } from '../selectors'; import { withGetText } from '../boot/TranslationProvider'; @@ -92,7 +91,6 @@ export type BackgroundData = $ReadOnly<{| mutedUsers: MutedUsersState, ownUser: User, streams: Map, - streamsByName: Map, subscriptions: Map, unread: UnreadState, theme: ThemeName, @@ -364,7 +362,6 @@ const MessageList: ComponentType = connect( mutedUsers: getMutedUsers(state), ownUser: getOwnUser(state), streams: getStreamsById(state), - streamsByName: getStreamsByName(state), subscriptions: getSubscriptionsById(state), unread: getUnread(state), theme: globalSettings.theme, diff --git a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android index 7d7a0e0eefc..5c4d809a112 100644 --- a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android +++ b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.android @@ -10,11 +10,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -88,11 +88,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -171,11 +171,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -214,11 +214,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 2
@@ -267,11 +267,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -310,11 +310,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6Mjp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjI=\\"> # stream 2
topic 2
@@ -363,11 +363,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -910,11 +910,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1056,11 +1056,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1099,11 +1099,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 2
@@ -1402,11 +1402,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1445,11 +1445,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6Mjp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjI=\\"> # stream 2
topic 2
@@ -1488,11 +1488,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1622,11 +1622,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -3373,7 +3373,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible pm:1,2 " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 1`] = ` "
@@ -3383,7 +3383,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3447,7 +3447,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 2`] = ` "
@@ -3457,7 +3457,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3526,7 +3526,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 3`] = ` "
@@ -3536,7 +3536,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3575,7 +3575,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3614,7 +3614,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 4`] = ` "
@@ -3624,7 +3624,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3699,7 +3699,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 5`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 5`] = ` "
@@ -3709,7 +3709,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3809,7 +3809,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3854,7 +3854,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3989,7 +3989,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 1`] = ` "
@@ -4054,7 +4054,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 2`] = ` "
@@ -4124,7 +4124,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 3`] = ` "
@@ -4200,7 +4200,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 4`] = ` "
diff --git a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios index 7d7a0e0eefc..5c4d809a112 100644 --- a/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios +++ b/src/webview/html/__tests__/__snapshots__/messageListElementHtml-test.js.snap.ios @@ -10,11 +10,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -88,11 +88,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -171,11 +171,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -214,11 +214,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 2
@@ -267,11 +267,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -310,11 +310,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6Mjp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjI=\\"> # stream 2
topic 2
@@ -363,11 +363,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -910,11 +910,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1056,11 +1056,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1099,11 +1099,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 2
@@ -1402,11 +1402,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1445,11 +1445,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6Mjp0b3BpYyAy\\">
+ data-narrow=\\"c3RyZWFtOjI=\\"> # stream 2
topic 2
@@ -1488,11 +1488,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -1622,11 +1622,11 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible HOME_N
+ data-narrow=\\"dG9waWM6MTp0b3BpYyAx\\">
+ data-narrow=\\"c3RyZWFtOjE=\\"> # stream 1
topic 1
@@ -3373,7 +3373,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible pm:1,2 " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 1`] = ` "
@@ -3383,7 +3383,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3447,7 +3447,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 2`] = ` "
@@ -3457,7 +3457,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3526,7 +3526,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 3`] = ` "
@@ -3536,7 +3536,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3575,7 +3575,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3614,7 +3614,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 4`] = ` "
@@ -3624,7 +3624,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3699,7 +3699,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:s:stream 1 5`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible stream:1 5`] = ` "
@@ -3709,7 +3709,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3809,7 +3809,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 2
@@ -3854,7 +3854,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream
topic 1
@@ -3989,7 +3989,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible stream " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 1`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 1`] = ` "
@@ -4054,7 +4054,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 2`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 2`] = ` "
@@ -4124,7 +4124,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 3`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 3`] = ` "
@@ -4200,7 +4200,7 @@ exports[`messages -> piece descriptors -> content HTML is stable/sensible topic: " `; -exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:s:stream 1topic 1 4`] = ` +exports[`messages -> piece descriptors -> content HTML is stable/sensible topic:1:topic 1 4`] = ` "
diff --git a/src/webview/html/__tests__/messageListElementHtml-test.js b/src/webview/html/__tests__/messageListElementHtml-test.js index cc25e4e38ef..0b7a07afad9 100644 --- a/src/webview/html/__tests__/messageListElementHtml-test.js +++ b/src/webview/html/__tests__/messageListElementHtml-test.js @@ -273,7 +273,7 @@ describe('messages -> piece descriptors -> content HTML is stable/sensible', () ].forEach(testCase => check(testCase)); }); - const streamNarrow1 = streamNarrow(stream1.name); + const streamNarrow1 = streamNarrow(stream1.stream_id); test(`${keyFromNarrow(streamNarrow1)}`, () => { [ { narrow: streamNarrow1, messages: streamMessages1 }, @@ -294,7 +294,7 @@ describe('messages -> piece descriptors -> content HTML is stable/sensible', () ].forEach(testCase => check(testCase)); }); - const topicNarrow1 = topicNarrow(stream1.name, topic1); + const topicNarrow1 = topicNarrow(stream1.stream_id, topic1); test(`${keyFromNarrow(topicNarrow1)}`, () => { [ { narrow: topicNarrow1, messages: streamMessages1 }, diff --git a/src/webview/html/header.js b/src/webview/html/header.js index e264af6c4c2..6c2b9e7a36b 100644 --- a/src/webview/html/header.js +++ b/src/webview/html/header.js @@ -39,7 +39,7 @@ export default ( if (message.type === 'stream') { const streamName = streamNameOfStreamMessage(message); - const topicNarrowStr = keyFromNarrow(topicNarrow(streamName, message.subject)); + const topicNarrowStr = keyFromNarrow(topicNarrow(message.stream_id, message.subject)); const topicHtml = renderSubject(message); if (headerStyle === 'topic+date') { @@ -57,7 +57,7 @@ export default ( const subscription = subscriptions.get(message.stream_id); const backgroundColor = subscription ? subscription.color : 'hsl(0, 0%, 80%)'; const textColor = foregroundColorFromBackground(backgroundColor); - const streamNarrowStr = keyFromNarrow(streamNarrow(streamName)); + const streamNarrowStr = keyFromNarrow(streamNarrow(message.stream_id)); return template`