From aa745346190ce4cd22f0bf9e1255d89570451fba Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 17:39:50 -0800 Subject: [PATCH 01/15] narrow [nfc]: Distinguish ApiNarrow type, with an alias for now. We've been taking the data structure that's used in a couple of spots in the Zulip API as our internal representation of a narrow throughout the app. It's a pretty bad fit for that purpose, and we're going to change to a more reasonable one. Lay the groundwork by marking the distinction between the handful of places where we actually mean the API-facing narrow type, and the rest where we want our own internal narrow type. For the present this is just a type alias, because lots of places in our code still intimately depend on the details of this representation. In the upcoming series of commits, we'll fix those so that they all use explicit interfaces provided by the `narrow` module, after which we can switch to an opaque type alias so that Flow confirms that nothing outside that module still depends on the specifics of the type. That will then set us up for future changes that actually alter the type. --- src/api/messages/getMessages.js | 4 ++-- src/api/modelTypes.js | 8 +++++++- src/api/registerForEvents.js | 4 ++-- src/reduxTypes.js | 3 +-- src/types.js | 1 + src/utils/narrow.js | 21 ++++++++++++++++++++- 6 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/api/messages/getMessages.js b/src/api/messages/getMessages.js index 3e0ebc93589..7e5d114b03a 100644 --- a/src/api/messages/getMessages.js +++ b/src/api/messages/getMessages.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import type { Auth, ApiResponseSuccess } from '../transportTypes'; import type { Identity } from '../../types'; -import type { Message, Narrow } from '../apiTypes'; +import type { Message, ApiNarrow } from '../apiTypes'; import type { Reaction } from '../modelTypes'; import { apiGet } from '../apiFetch'; import { identityOfAuth } from '../../account/accountMisc'; @@ -86,7 +86,7 @@ const migrateResponse = (response, identity: Identity) => { export default async ( auth: Auth, args: {| - narrow: Narrow, + narrow: ApiNarrow, anchor: number, numBefore: number, numAfter: number, diff --git a/src/api/modelTypes.js b/src/api/modelTypes.js index 638c6195e3d..7e9076882e4 100644 --- a/src/api/modelTypes.js +++ b/src/api/modelTypes.js @@ -312,7 +312,13 @@ export type NarrowElement = $ReadOnly<{| operator: NarrowOperator, |}>; -export type Narrow = $ReadOnlyArray; +/** + * A narrow, in the form used in the Zulip API at get-messages. + * + * See also `Narrow` in the non-API app code, which describes how we + * represent narrows within the app. + */ +export type ApiNarrow = $ReadOnlyArray; // // diff --git a/src/api/registerForEvents.js b/src/api/registerForEvents.js index 4f23f32b1f7..655849892d0 100644 --- a/src/api/registerForEvents.js +++ b/src/api/registerForEvents.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import type { RawInitialData, InitialData } from './initialDataTypes'; import type { Auth } from './transportTypes'; -import type { Narrow } from './apiTypes'; +import type { ApiNarrow } from './apiTypes'; import type { CrossRealmBot, User } from './modelTypes'; import { apiPost } from './apiFetch'; import { AvatarURL } from '../utils/avatar'; @@ -13,7 +13,7 @@ type RegisterForEventsParams = {| event_types?: string[], fetch_event_types?: string[], include_subscribers?: boolean, - narrow?: Narrow, + narrow?: ApiNarrow, queue_lifespan_secs?: number, client_capabilities?: {| notification_settings_null: boolean, diff --git a/src/reduxTypes.js b/src/reduxTypes.js index 5a1a42d2255..2926af269d6 100644 --- a/src/reduxTypes.js +++ b/src/reduxTypes.js @@ -21,7 +21,6 @@ import type { CrossRealmBot, RealmEmojiById, RealmFilter, - Narrow, Stream, StreamUnreadItem, Subscription, @@ -30,7 +29,7 @@ import type { UserPresence, UserStatusMapObject, } from './api/apiTypes'; - +import type { Narrow } from './utils/narrow'; import type { SessionState } from './session/sessionReducer'; export type * from './actionTypes'; diff --git a/src/types.js b/src/types.js index 9300b42fd1b..c0cb11b2b99 100644 --- a/src/types.js +++ b/src/types.js @@ -10,6 +10,7 @@ import type { PmKeyUsers } from './utils/recipient'; export type * from './generics'; export type * from './reduxTypes'; export type * from './api/apiTypes'; +export type { Narrow } from './utils/narrow'; export { ensureUnreachable } from './generics'; diff --git a/src/utils/narrow.js b/src/utils/narrow.js index a40e1f5a019..12785d4a050 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import isEqual from 'lodash.isequal'; -import type { Narrow, Message, Outbox, User, UserOrBot } from '../types'; +import type { ApiNarrow, Message, Outbox, User, UserOrBot } from '../types'; import { normalizeRecipientsSansMe, pmKeyRecipientsFromMessage, @@ -13,6 +13,25 @@ import { /* eslint-disable no-use-before-define */ +/** + * A narrow. + * + * A narrow is the navigational property that defines what to show in the + * message list UI: it's a subset of messages which might be a conversation, + * a whole stream, a search, or a few other varieties. + * + * Much of the data we fetch is to support the message list UI, and so we + * keep much of it in data structures indexed by narrow. + * + * See also: + * * `keyFromNarrow`, for converting into a string key good for indexing + * into a data structure; + * * `caseNarrow` and its relatives, for pattern-matching or destructuring; + * * `ApiNarrow` for the form we put a narrow in when talking to the + * server. + */ +export type Narrow = ApiNarrow; + export const isSameNarrow = (narrow1: Narrow, narrow2: Narrow): boolean => Array.isArray(narrow1) && Array.isArray(narrow2) && isEqual(narrow1, narrow2); From 86b3e7d7b49b21047bcf19344ff365e8157dc13e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 19:33:42 -0800 Subject: [PATCH 02/15] narrow tests: Stop constructing narrows as array literals. We're going to change our internal representation of narrows soon to something more reasonable. The internal representation doesn't belong in tests anyway, so remove it. --- src/utils/__tests__/narrow-test.js | 43 ++---------------------------- 1 file changed, 2 insertions(+), 41 deletions(-) diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index 05f34029074..b68de70f6a2 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -35,8 +35,8 @@ describe('HOME_NARROW', () => { expect(HOME_NARROW).toEqual([]); }); - test('empty list is a home narrow', () => { - expect(isHomeNarrow([])).toBe(true); + test('is a home narrow', () => { + expect(isHomeNarrow(HOME_NARROW)).toBe(true); }); }); @@ -53,14 +53,6 @@ describe('pmNarrowFromEmail', () => { test('if operator is "pm-with" and only one email, then it is a private narrow', () => { expect(is1to1PmNarrow(HOME_NARROW)).toBe(false); expect(is1to1PmNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(true); - expect( - is1to1PmNarrow([ - { - operator: 'pm-with', - operand: eg.otherUser.email, - }, - ]), - ).toBe(true); }); }); @@ -70,22 +62,6 @@ describe('isPmNarrow', () => { expect(isPmNarrow(HOME_NARROW)).toBe(false); expect(isPmNarrow(pmNarrowFromEmail(eg.otherUser.email))).toBe(true); expect(isPmNarrow(pmNarrowFromUsersUnsafe([eg.otherUser, eg.thirdUser]))).toBe(true); - expect( - isPmNarrow([ - { - operator: 'pm-with', - operand: eg.otherUser.email, - }, - ]), - ).toBe(true); - expect( - isPmNarrow([ - { - operator: 'pm-with', - operand: [eg.otherUser.email, eg.thirdUser.email].join(','), - }, - ]), - ).toBe(true); }); }); @@ -118,8 +94,6 @@ describe('specialNarrow', () => { expect(isSpecialNarrow(HOME_NARROW)).toBe(false); expect(isSpecialNarrow(streamNarrow('some stream'))).toBe(false); expect(isSpecialNarrow(STARRED_NARROW)).toBe(true); - expect(isSpecialNarrow([{ operator: 'stream', operand: 'some stream' }])).toBe(false); - expect(isSpecialNarrow([{ operator: 'is', operand: 'starred' }])).toBe(true); }); }); @@ -137,7 +111,6 @@ describe('streamNarrow', () => { expect(isStreamNarrow(undefined)).toBe(false); expect(isStreamNarrow(HOME_NARROW)).toBe(false); expect(isStreamNarrow(streamNarrow('some stream'))).toBe(true); - expect(isStreamNarrow([{ operator: 'stream', operand: 'some stream' }])).toBe(true); }); }); @@ -153,18 +126,6 @@ describe('topicNarrow', () => { expect(isTopicNarrow(undefined)).toBe(false); expect(isTopicNarrow(HOME_NARROW)).toBe(false); expect(isTopicNarrow(topicNarrow('some stream', 'some topic'))).toBe(true); - expect( - isTopicNarrow([ - { - operator: 'stream', - operand: 'some stream', - }, - { - operator: 'topic', - operand: 'some topic', - }, - ]), - ).toBe(true); }); }); From b2a0f6268bf5880a923932b2375a4e2883ccb5b0 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 19:38:00 -0800 Subject: [PATCH 03/15] tests: Stop constructing narrows as literals. We're going to change the internal representation of narrows soon. The internal representation isn't really any test's business in the first place, and it especially was never the business of any of these tests that aren't of the `narrow` module itself. Instead, they should use the proper constructors and constants from the `narrow` module. --- src/caughtup/__tests__/caughtUpReducer-test.js | 5 +++-- src/chat/__tests__/fetchingReducer-test.js | 16 +++++++++++----- src/chat/__tests__/narrowsReducer-test.js | 13 +++++++------ src/message/__tests__/messageActionSheet-test.js | 5 +++-- 4 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/caughtup/__tests__/caughtUpReducer-test.js b/src/caughtup/__tests__/caughtUpReducer-test.js index 83a54d811cb..dd07f4ba47b 100644 --- a/src/caughtup/__tests__/caughtUpReducer-test.js +++ b/src/caughtup/__tests__/caughtUpReducer-test.js @@ -10,6 +10,7 @@ import { HOME_NARROW_STR, ALL_PRIVATE_NARROW, ALL_PRIVATE_NARROW_STR, + SEARCH_NARROW, } from '../../utils/narrow'; describe('caughtUpReducer', () => { @@ -42,7 +43,7 @@ describe('caughtUpReducer', () => { const action = deepFreeze({ ...eg.action.message_fetch_start, - narrow: [{ operator: 'search', operand: 'some query' }], + narrow: SEARCH_NARROW('some query'), }); const newState = caughtUpReducer(initialState, action); @@ -126,7 +127,7 @@ describe('caughtUpReducer', () => { const action = deepFreeze({ ...eg.action.message_fetch_complete, - narrow: [{ operator: 'search', operand: 'some query' }], + narrow: SEARCH_NARROW('some query'), }); const newState = caughtUpReducer(initialState, action); diff --git a/src/chat/__tests__/fetchingReducer-test.js b/src/chat/__tests__/fetchingReducer-test.js index 64093bd808b..85294e84f73 100644 --- a/src/chat/__tests__/fetchingReducer-test.js +++ b/src/chat/__tests__/fetchingReducer-test.js @@ -3,7 +3,13 @@ import deepFreeze from 'deep-freeze'; import * as eg from '../../__tests__/lib/exampleData'; import fetchingReducer from '../fetchingReducer'; -import { HOME_NARROW, HOME_NARROW_STR, streamNarrow, keyFromNarrow } from '../../utils/narrow'; +import { + HOME_NARROW, + HOME_NARROW_STR, + streamNarrow, + keyFromNarrow, + SEARCH_NARROW, +} from '../../utils/narrow'; import { MESSAGE_FETCH_START, MESSAGE_FETCH_ERROR } from '../../actionConstants'; import { DEFAULT_FETCHING } from '../fetchingSelectors'; @@ -16,7 +22,7 @@ describe('fetchingReducer', () => { const action = deepFreeze({ type: MESSAGE_FETCH_START, - narrow: [], + narrow: HOME_NARROW, numBefore: 10, numAfter: 10, }); @@ -62,7 +68,7 @@ describe('fetchingReducer', () => { const action = deepFreeze({ ...eg.action.message_fetch_start, - narrow: [{ operator: 'search', operand: 'some query' }], + narrow: SEARCH_NARROW('some query'), }); const newState = fetchingReducer(initialState, action); @@ -108,7 +114,7 @@ describe('fetchingReducer', () => { const action = { ...eg.action.message_fetch_complete, - narrow: [], + narrow: HOME_NARROW, numBefore: 10, numAfter: 0, }; @@ -130,7 +136,7 @@ describe('fetchingReducer', () => { const action = deepFreeze({ ...eg.action.message_fetch_complete, - narrow: [{ operator: 'search', operand: 'some query' }], + narrow: SEARCH_NARROW('some query'), }); const newState = fetchingReducer(initialState, action); diff --git a/src/chat/__tests__/narrowsReducer-test.js b/src/chat/__tests__/narrowsReducer-test.js index f661b6dd717..bd7d05b0fc3 100644 --- a/src/chat/__tests__/narrowsReducer-test.js +++ b/src/chat/__tests__/narrowsReducer-test.js @@ -13,6 +13,7 @@ import { topicNarrow, STARRED_NARROW_STR, keyFromNarrow, + SEARCH_NARROW, } from '../../utils/narrow'; import { MESSAGE_FETCH_ERROR, @@ -324,7 +325,7 @@ describe('narrowsReducer', () => { const action = deepFreeze({ ...eg.action.message_fetch_start, - narrow: [{ operator: 'search', operand: 'some query' }], + narrow: SEARCH_NARROW('some query'), }); const newState = narrowsReducer(initialState, action); @@ -396,7 +397,7 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, anchor: 2, - narrow: [], + narrow: HOME_NARROW, messages: [ eg.streamMessage({ id: 2 }), eg.streamMessage({ id: 3 }), @@ -424,7 +425,7 @@ describe('narrowsReducer', () => { const action = deepFreeze({ type: MESSAGE_FETCH_COMPLETE, anchor: 2, - narrow: [], + narrow: HOME_NARROW, messages: [ eg.streamMessage({ id: 3, timestamp: 2 }), eg.streamMessage({ id: 4, timestamp: 1 }), @@ -451,7 +452,7 @@ describe('narrowsReducer', () => { const action = deepFreeze({ anchor: FIRST_UNREAD_ANCHOR, type: MESSAGE_FETCH_COMPLETE, - narrow: [], + narrow: HOME_NARROW, messages: [ eg.streamMessage({ id: 3, timestamp: 2 }), eg.streamMessage({ id: 4, timestamp: 1 }), @@ -477,7 +478,7 @@ describe('narrowsReducer', () => { const action = deepFreeze({ anchor: LAST_MESSAGE_ANCHOR, type: MESSAGE_FETCH_COMPLETE, - narrow: [], + narrow: HOME_NARROW, messages: [ eg.streamMessage({ id: 3, timestamp: 2 }), eg.streamMessage({ id: 4, timestamp: 1 }), @@ -502,7 +503,7 @@ describe('narrowsReducer', () => { const action = deepFreeze({ ...eg.action.message_fetch_complete, - narrow: [{ operator: 'search', operand: 'some query' }], + narrow: SEARCH_NARROW('some query'), }); const newState = narrowsReducer(initialState, action); diff --git a/src/message/__tests__/messageActionSheet-test.js b/src/message/__tests__/messageActionSheet-test.js index 5372a61585f..d6f6b9f463b 100644 --- a/src/message/__tests__/messageActionSheet-test.js +++ b/src/message/__tests__/messageActionSheet-test.js @@ -1,5 +1,6 @@ // @flow strict-local import deepFreeze from 'deep-freeze'; +import { HOME_NARROW } from '../../utils/narrow'; import * as eg from '../../__tests__/lib/exampleData'; import { constructMessageActionButtons, constructHeaderActionButtons } from '../messageActionSheet'; @@ -18,7 +19,7 @@ const baseBackgroundData = deepFreeze({ }); describe('constructActionButtons', () => { - const narrow = deepFreeze([]); + const narrow = deepFreeze(HOME_NARROW); test('show star message option if message is not starred', () => { const message = eg.streamMessage(); @@ -53,7 +54,7 @@ describe('constructActionButtons', () => { }); describe('constructHeaderActionButtons', () => { - const narrow = deepFreeze([]); + const narrow = deepFreeze(HOME_NARROW); test('show Unmute topic option if topic is muted', () => { const mute = deepFreeze([['electron issues', 'issue #556']]); From b3246e2eb4945f492b0924390f68c5628a2ab682 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 5 Feb 2020 20:11:40 -0800 Subject: [PATCH 04/15] unreads [nfc]: Simplify `getUnreadCountForNarrow` slightly. Instead of treating these items as zero when summing up, filter them out in the first place. As a bonus, this makes this case more parallel to the `isTopicNarrow` case just below. --- src/unread/unreadSelectors.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index 1b623973117..078b9178716 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -256,12 +256,8 @@ export const getUnreadCountForNarrow: Selector = createSelector( } return unreadStreams - .filter(x => x.stream_id === stream.stream_id) - .reduce( - (sum, x) => - sum + (isTopicMuted(stream.name, x.topic, mute) ? 0 : x.unread_message_ids.length), - 0, - ); + .filter(x => x.stream_id === stream.stream_id && !isTopicMuted(stream.name, x.topic, mute)) + .reduce((sum, x) => sum + x.unread_message_ids.length, 0); } if (isTopicNarrow(narrow)) { From 18a8d268489449ffaa576fc2022ba498af5f922e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 15 Dec 2020 15:25:32 -0800 Subject: [PATCH 05/15] unreads [nfc]: Simplify `getUnreadCountForNarrow` a bit more. This deduplicates a bit of code between these two very similar codepaths. --- src/unread/unreadSelectors.js | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index 078b9178716..9b4fc065595 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -244,6 +244,8 @@ export const getUnreadCountForNarrow: Selector = createSelector( unreadPms, mute, ) => { + const sumLengths = unreads => unreads.reduce((sum, x) => sum + x.unread_message_ids.length, 0); + if (isHomeNarrow(narrow)) { return unreadTotal; } @@ -255,9 +257,11 @@ export const getUnreadCountForNarrow: Selector = createSelector( return 0; } - return unreadStreams - .filter(x => x.stream_id === stream.stream_id && !isTopicMuted(stream.name, x.topic, mute)) - .reduce((sum, x) => sum + x.unread_message_ids.length, 0); + return sumLengths( + unreadStreams.filter( + x => x.stream_id === stream.stream_id && !isTopicMuted(stream.name, x.topic, mute), + ), + ); } if (isTopicNarrow(narrow)) { @@ -267,9 +271,11 @@ export const getUnreadCountForNarrow: Selector = createSelector( return 0; } - return unreadStreams - .filter(x => x.stream_id === stream.stream_id && x.topic === narrow[1].operand) - .reduce((sum, x) => sum + x.unread_message_ids.length, 0); + return sumLengths( + unreadStreams.filter( + x => x.stream_id === stream.stream_id && x.topic === narrow[1].operand, + ), + ); } if (isGroupPmNarrow(narrow)) { From 4a5fa07f17e8020022524337b849e23e1bda9b55 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 5 Feb 2020 21:55:46 -0800 Subject: [PATCH 06/15] unreads [nfc]: Unpack narrow structurally in getUnreadCountForNarrow. Use `caseNarrow` to let the specifics of narrow "operands", etc., stay an implementation detail of the `Narrow` type. This also gives us type-checking that we've thought of all the types of narrows here... and exposes that we had not in fact thought of the all-PMs narrow! That's purely a latent bug: despite its general name, this selector is used only to feed one part of the UI, namely the "N unreads" banner above a message list, and we don't expose the all-PMs narrow as a message list you can actually view. But it's good to make explicit so we aren't surprised in the future. The diff is best read with `git diff -b`, as it causes some re-indents. --- src/unread/unreadSelectors.js | 129 ++++++++++++++++------------------ 1 file changed, 62 insertions(+), 67 deletions(-) diff --git a/src/unread/unreadSelectors.js b/src/unread/unreadSelectors.js index 9b4fc065595..da982d94c89 100644 --- a/src/unread/unreadSelectors.js +++ b/src/unread/unreadSelectors.js @@ -14,13 +14,7 @@ import { import { getOwnEmail, getAllUsersByEmail } from '../users/userSelectors'; import { getSubscriptionsById } from '../subscriptions/subscriptionSelectors'; import { isTopicMuted } from '../utils/message'; -import { - isHomeNarrow, - isStreamNarrow, - isTopicNarrow, - isGroupPmNarrow, - is1to1PmNarrow, -} from '../utils/narrow'; +import { caseNarrow } from '../utils/narrow'; import { NULL_SUBSCRIPTION, NULL_USER } from '../nullObjects'; /** The number of unreads in each stream, excluding muted topics, by stream ID. */ @@ -219,7 +213,8 @@ export const getUnreadByHuddlesMentionsAndPMs: Selector = createSelector /** * Total number of unreads in the given narrow... mostly. * - * For the @-mention narrow and search narrows, just returns 0. + * For the @-mention narrow, all-PMs narrow, and search narrows, + * just returns 0. * * For the all-messages narrow, the caveats on `getUnreadTotal` apply. */ @@ -246,64 +241,64 @@ export const getUnreadCountForNarrow: Selector = createSelector( ) => { const sumLengths = unreads => unreads.reduce((sum, x) => sum + x.unread_message_ids.length, 0); - if (isHomeNarrow(narrow)) { - return unreadTotal; - } - - if (isStreamNarrow(narrow)) { - const stream = streams.find(s => s.name === narrow[0].operand); - - if (!stream) { - return 0; - } - - return sumLengths( - unreadStreams.filter( - x => x.stream_id === stream.stream_id && !isTopicMuted(stream.name, x.topic, mute), - ), - ); - } - - if (isTopicNarrow(narrow)) { - const stream = streams.find(s => s.name === narrow[0].operand); - - if (!stream) { - return 0; - } - - return sumLengths( - unreadStreams.filter( - x => x.stream_id === stream.stream_id && x.topic === narrow[1].operand, - ), - ); - } - - if (isGroupPmNarrow(narrow)) { - const userIds = [...narrow[0].operand.split(','), ownEmail] - .map(email => (allUsersByEmail.get(email) || NULL_USER).user_id) - .sort((a, b) => a - b) - .join(','); - const unread = unreadHuddles.find(x => x.user_ids_string === userIds); - return unread ? unread.unread_message_ids.length : 0; - } - - if (is1to1PmNarrow(narrow)) { - const sender = allUsersByEmail.get(narrow[0].operand); - if (!sender) { - return 0; - } - const unread = unreadPms.find(x => x.sender_id === sender.user_id); - return unread ? unread.unread_message_ids.length : 0; - } - - // Unread starred messages are impossible, so 0 is correct for the - // starred-messages narrow. - // TODO: fact-check that. - - // TODO: give a correct answer for the @-mentions narrow. - - // For search narrows, shrug, a bogus answer of 0 is fine. - - return 0; + return caseNarrow(narrow, { + home: () => unreadTotal, + + stream: name => { + const stream = streams.find(s => s.name === name); + if (!stream) { + return 0; + } + return sumLengths( + unreadStreams.filter( + x => x.stream_id === stream.stream_id && !isTopicMuted(name, x.topic, mute), + ), + ); + }, + + topic: (streamName, topic) => { + const stream = streams.find(s => s.name === streamName); + if (!stream) { + return 0; + } + return sumLengths( + unreadStreams.filter(x => x.stream_id === stream.stream_id && x.topic === topic), + ); + }, + + pm: emails => { + if (emails.length > 1) { + const userIds = [...emails, ownEmail] + .map(email => (allUsersByEmail.get(email) || NULL_USER).user_id) + .sort((a, b) => a - b) + .join(','); + const unread = unreadHuddles.find(x => x.user_ids_string === userIds); + return unread ? unread.unread_message_ids.length : 0; + } else { + const sender = allUsersByEmail.get(emails[0]); + if (!sender) { + return 0; + } + const unread = unreadPms.find(x => x.sender_id === sender.user_id); + return unread ? unread.unread_message_ids.length : 0; + } + }, + + // Unread starred messages are impossible, so 0 is correct for the + // starred-messages narrow. + // TODO: fact-check that. + starred: () => 0, + + // TODO: give a correct answer for the @-mentions narrow. + mentioned: () => 0, + + // For search narrows, shrug, a bogus answer of 0 is fine. + search: () => 0, + + // For the all-PMs narrow, a bogus answer of 0 is perfectly fine + // because we never use this selector for that narrow (because we + // don't expose it as one you can narrow to in the UI.) + allPrivate: () => 0, + }); }, ); From 3c0a42614afe9ff1313949de92139b1d1cacf154 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 5 Feb 2020 20:04:28 -0800 Subject: [PATCH 07/15] compose [nfc]: Unpack narrow structurally in getComposeInputPlaceholder. Instead of using `narrow[0].operand` and hoping the code correctly lines up its meaning with the conditionals, use the `caseNarrow` family to unpack the components of the narrow in a structured way. --- src/compose/getComposeInputPlaceholder.js | 62 +++++++++++------------ 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/src/compose/getComposeInputPlaceholder.js b/src/compose/getComposeInputPlaceholder.js index ec24a0d1f55..59a2824f394 100644 --- a/src/compose/getComposeInputPlaceholder.js +++ b/src/compose/getComposeInputPlaceholder.js @@ -1,42 +1,40 @@ /* @flow strict-local */ import type { Narrow, UserOrBot, LocalizableText } from '../types'; -import { isStreamNarrow, isTopicNarrow, isGroupPmNarrow, is1to1PmNarrow } from '../utils/narrow'; +import { caseNarrowDefault } from '../utils/narrow'; export default ( narrow: Narrow, ownEmail: string, usersByEmail: Map, -): LocalizableText => { - if (isGroupPmNarrow(narrow)) { - return { text: 'Message group' }; - } +): LocalizableText => + caseNarrowDefault( + narrow, + { + pm: emails => { + if (emails.length > 1) { + return { text: 'Message group' }; + } + const email = emails[0]; - if (is1to1PmNarrow(narrow)) { - if (ownEmail && narrow[0].operand === ownEmail) { - return { text: 'Jot down something' }; - } + if (ownEmail && email === ownEmail) { + return { text: 'Jot down something' }; + } - if (!usersByEmail) { - return { text: 'Type a message' }; - } + if (!usersByEmail) { + return { text: 'Type a message' }; + } + const user = usersByEmail.get(email) || {}; - const user = usersByEmail.get(narrow[0].operand) || {}; - return { - text: 'Message {recipient}', - values: { recipient: `@${user.full_name}` }, - }; - } - - if (isStreamNarrow(narrow)) { - return { - text: 'Message {recipient}', - values: { recipient: `#${narrow[0].operand}` }, - }; - } - - if (isTopicNarrow(narrow)) { - return { text: 'Reply' }; - } - - return { text: 'Type a message' }; -}; + return { + text: 'Message {recipient}', + values: { recipient: `@${user.full_name}` }, + }; + }, + stream: name => ({ + text: 'Message {recipient}', + values: { recipient: `#${name}` }, + }), + topic: () => ({ text: 'Reply' }), + }, + () => ({ text: 'Type a message' }), + ); From c658745407b12d27ad0d992b85f4be373e69cd4e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 8 Dec 2020 11:51:16 -0800 Subject: [PATCH 08/15] compose [nfc]: Cut a bit of code for impossible conditions. --- src/compose/getComposeInputPlaceholder.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/compose/getComposeInputPlaceholder.js b/src/compose/getComposeInputPlaceholder.js index 59a2824f394..ff7a5563633 100644 --- a/src/compose/getComposeInputPlaceholder.js +++ b/src/compose/getComposeInputPlaceholder.js @@ -16,14 +16,14 @@ export default ( } const email = emails[0]; - if (ownEmail && email === ownEmail) { + if (email === ownEmail) { return { text: 'Jot down something' }; } - if (!usersByEmail) { + const user = usersByEmail.get(email); + if (!user) { return { text: 'Type a message' }; } - const user = usersByEmail.get(email) || {}; return { text: 'Message {recipient}', From 4b68dde3981c6bc4957d6a7ad6eb8ce5217e17b9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 15 Dec 2020 15:54:36 -0800 Subject: [PATCH 09/15] narrow [nfc]: Add a getter emailsOfPmNarrow, and use it. --- src/typing/typingSelectors.js | 4 ++-- src/users/usersActions.js | 6 ++---- src/utils/narrow.js | 9 +++++++++ 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/typing/typingSelectors.js b/src/typing/typingSelectors.js index 447c324109e..61b928822cc 100644 --- a/src/typing/typingSelectors.js +++ b/src/typing/typingSelectors.js @@ -3,7 +3,7 @@ import { createSelector } from 'reselect'; import type { Narrow, Selector, UserOrBot } from '../types'; import { getTyping } from '../directSelectors'; -import { isPmNarrow } from '../utils/narrow'; +import { emailsOfPmNarrow, isPmNarrow } from '../utils/narrow'; import { normalizeRecipientsAsUserIds } from '../utils/recipient'; import { NULL_ARRAY, NULL_USER } from '../nullObjects'; import { getAllUsersById, getAllUsersByEmail } from '../users/userSelectors'; @@ -18,7 +18,7 @@ export const getCurrentTypingUsers: Selector<$ReadOnlyArray, Narrow> return NULL_ARRAY; } - const recipients = narrow[0].operand.split(',').map(email => { + const recipients = emailsOfPmNarrow(narrow).map(email => { const userId = allUsersByEmail.get(email)?.user_id; if (userId === undefined) { throw new Error(`Narrow contains email '${email}' that does not map to any user.`); diff --git a/src/users/usersActions.js b/src/users/usersActions.js index 74e5cfbe39e..3d44f4abdd3 100644 --- a/src/users/usersActions.js +++ b/src/users/usersActions.js @@ -5,7 +5,7 @@ import type { Auth, Dispatch, GetState, GlobalState, Narrow } from '../types'; import * as api from '../api'; import { PRESENCE_RESPONSE } from '../actionConstants'; import { getAuth, tryGetAuth, getServerVersion } from '../selectors'; -import { isPmNarrow, caseNarrowPartial } from '../utils/narrow'; +import { isPmNarrow, emailsOfPmNarrow } from '../utils/narrow'; import { getAllUsersByEmail, getUserForId } from './userSelectors'; import { ZulipVersion } from '../utils/zulipVersion'; @@ -65,9 +65,7 @@ export const sendTypingStart = (narrow: Narrow) => async ( } const allUsersByEmail = getAllUsersByEmail(getState()); - const recipientIds = caseNarrowPartial(narrow, { - pm: emails => emails, - }).map(email => { + const recipientIds = emailsOfPmNarrow(narrow).map(email => { const user = allUsersByEmail.get(email); if (!user) { throw new Error('unknown user'); diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 12785d4a050..3e5d9dfa864 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -384,6 +384,15 @@ export const emailsOfGroupPmNarrow = (narrow: Narrow): string[] => }, }); +/** + * The "PM key recipients" emails for a PM narrow; else error. + * + * This is the same list of users that can appear in a `PmKeyRecipients` or + * `PmKeyUsers`, but contains only their emails. + */ +export const emailsOfPmNarrow = (narrow: Narrow): string[] => + caseNarrowPartial(narrow, { pm: emails => emails }); + export const isPmNarrow = (narrow?: Narrow): boolean => !!narrow && caseNarrowDefault(narrow, { pm: () => true }, () => false); From 2edc6d4a4c37a73cac192b03a7fb369bc0e5a4d4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 19:53:37 -0800 Subject: [PATCH 10/15] narrow [nfc]: Add getters for stream name and topic; use them. As the jsdoc added here says, all these call sites should probably be doing something else: where the code is really about a stream, or a stream conversation, it should be getting passed down a stream name, or stream name and topic (better yet, a stream *ID* and possibly a topic) in the first place, rather than a Narrow. Where on the other hand the code naturally takes a whole Narrow because it handles other kinds of narrows too, it should generally be using caseNarrow. Fixing that would be its own set of refactors in a mostly independent direction from this one, though, so for now we leave that for a future project (or really series of small projects.) --- src/chat/MarkUnreadButton.js | 16 ++++++++++++---- src/chat/narrowsSelectors.js | 6 ++++-- src/compose/ComposeBox.js | 15 ++++++++++++--- src/subscriptions/subscriptionSelectors.js | 9 ++++++--- src/title-buttons/ExtraNavButtonStream.js | 4 +++- src/title-buttons/ExtraNavButtonTopic.js | 5 +++-- src/title-buttons/InfoNavButtonStream.js | 4 +++- src/title/TitleStream.js | 6 +++--- src/title/titleSelectors.js | 4 ++-- src/topics/topicActions.js | 5 +++-- src/topics/topicSelectors.js | 5 +++-- src/utils/narrow.js | 20 ++++++++++++++++++++ 12 files changed, 74 insertions(+), 25 deletions(-) diff --git a/src/chat/MarkUnreadButton.js b/src/chat/MarkUnreadButton.js index d5f6abe15ed..bdf6a04fb3a 100644 --- a/src/chat/MarkUnreadButton.js +++ b/src/chat/MarkUnreadButton.js @@ -8,7 +8,13 @@ import { connect } from '../react-redux'; import { ZulipButton } from '../common'; import * as api from '../api'; import { getAuth, getStreams } from '../selectors'; -import { isHomeNarrow, isStreamNarrow, isTopicNarrow } from '../utils/narrow'; +import { + isHomeNarrow, + isStreamNarrow, + isTopicNarrow, + streamNameOfNarrow, + topicOfNarrow, +} from '../utils/narrow'; const styles = createStyleSheet({ button: { @@ -34,7 +40,8 @@ class MarkUnreadButton extends PureComponent { markStreamAsRead = () => { const { auth, narrow, streams } = this.props; - const stream = streams.find(s => s.name === narrow[0].operand); + const streamName = streamNameOfNarrow(narrow); + const stream = streams.find(s => s.name === streamName); if (stream) { api.markStreamAsRead(auth, stream.stream_id); } @@ -42,9 +49,10 @@ class MarkUnreadButton extends PureComponent { markTopicAsRead = () => { const { auth, narrow, streams } = this.props; - const stream = streams.find(s => s.name === narrow[0].operand); + const streamName = streamNameOfNarrow(narrow); + const stream = streams.find(s => s.name === streamName); if (stream) { - api.markTopicAsRead(auth, stream.stream_id, narrow[1].operand); + api.markTopicAsRead(auth, stream.stream_id, topicOfNarrow(narrow)); } }; diff --git a/src/chat/narrowsSelectors.js b/src/chat/narrowsSelectors.js index dc1ffb53660..b62d402011f 100644 --- a/src/chat/narrowsSelectors.js +++ b/src/chat/narrowsSelectors.js @@ -29,6 +29,7 @@ import { isMessageInNarrow, caseNarrowDefault, keyFromNarrow, + streamNameOfNarrow, } from '../utils/narrow'; import { shouldBeMuted } from '../utils/message'; import { NULL_ARRAY, NULL_SUBSCRIPTION } from '../nullObjects'; @@ -125,13 +126,14 @@ export const getStreamInNarrow: Selector x.name === narrow[0].operand); + const subscription = subscriptions.find(x => x.name === streamName); if (subscription) { return subscription; } - const stream = streams.find(x => x.name === narrow[0].operand); + const stream = streams.find(x => x.name === streamName); if (stream) { return { ...stream, diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js index 7945354cc76..6fcbc82cdb8 100644 --- a/src/compose/ComposeBox.js +++ b/src/compose/ComposeBox.js @@ -33,7 +33,12 @@ import * as api from '../api'; import { FloatingActionButton, Input } from '../common'; import { showErrorAlert } from '../utils/info'; import { IconDone, IconSend } from '../common/Icons'; -import { isStreamNarrow, isStreamOrTopicNarrow, topicNarrow } from '../utils/narrow'; +import { + isStreamNarrow, + isStreamOrTopicNarrow, + streamNameOfNarrow, + topicNarrow, +} from '../utils/narrow'; import ComposeMenu from './ComposeMenu'; import getComposeInputPlaceholder from './getComposeInputPlaceholder'; import NotSubscribed from '../message/NotSubscribed'; @@ -307,8 +312,12 @@ class ComposeBox extends PureComponent { getDestinationNarrow = (): Narrow => { const { narrow } = this.props; - const topic = this.state.topic.trim(); - return isStreamNarrow(narrow) ? topicNarrow(narrow[0].operand, topic || '(no topic)') : narrow; + if (isStreamNarrow(narrow)) { + const streamName = streamNameOfNarrow(narrow); + const topic = this.state.topic.trim(); + return topicNarrow(streamName, topic || '(no topic)'); + } + return narrow; }; handleSend = () => { diff --git a/src/subscriptions/subscriptionSelectors.js b/src/subscriptions/subscriptionSelectors.js index 4d7436867f4..deac6b6de23 100644 --- a/src/subscriptions/subscriptionSelectors.js +++ b/src/subscriptions/subscriptionSelectors.js @@ -2,7 +2,7 @@ import { createSelector } from 'reselect'; import type { GlobalState, Narrow, Selector, Stream, Subscription } from '../types'; -import { isStreamOrTopicNarrow } from '../utils/narrow'; +import { isStreamOrTopicNarrow, streamNameOfNarrow } from '../utils/narrow'; import { getSubscriptions, getStreams } from '../directSelectors'; /** @@ -39,8 +39,9 @@ export const getIsActiveStreamSubscribed: Selector = createSele if (!isStreamOrTopicNarrow(narrow)) { return true; } + const streamName = streamNameOfNarrow(narrow); - return subscriptions.find(sub => narrow[0].operand === sub.name) !== undefined; + return subscriptions.find(sub => streamName === sub.name) !== undefined; }, ); @@ -79,7 +80,9 @@ export const getIsActiveStreamAnnouncementOnly: Selector = crea if (!isStreamOrTopicNarrow(narrow)) { return false; } - const stream = streams.find(stream_ => narrow[0].operand === stream_.name); + const streamName = streamNameOfNarrow(narrow); + + const stream = streams.find(stream_ => streamName === stream_.name); return stream ? stream.is_announcement_only : false; }, ); diff --git a/src/title-buttons/ExtraNavButtonStream.js b/src/title-buttons/ExtraNavButtonStream.js index 94ee3da0be9..be44468be5d 100644 --- a/src/title-buttons/ExtraNavButtonStream.js +++ b/src/title-buttons/ExtraNavButtonStream.js @@ -8,6 +8,7 @@ import { connect } from '../react-redux'; import { getStreams } from '../selectors'; import NavButton from '../nav/NavButton'; import { navigateToTopicList } from '../actions'; +import { streamNameOfNarrow } from '../utils/narrow'; type Props = $ReadOnly<{| dispatch: Dispatch, @@ -19,7 +20,8 @@ type Props = $ReadOnly<{| class ExtraNavButtonStream extends PureComponent { handlePress = () => { const { narrow, streams } = this.props; - const stream = streams.find(x => x.name === narrow[0].operand); + const streamName = streamNameOfNarrow(narrow); + const stream = streams.find(x => x.name === streamName); if (stream) { NavigationService.dispatch(navigateToTopicList(stream.stream_id)); } diff --git a/src/title-buttons/ExtraNavButtonTopic.js b/src/title-buttons/ExtraNavButtonTopic.js index 8127e58488d..a0a7e2e8903 100644 --- a/src/title-buttons/ExtraNavButtonTopic.js +++ b/src/title-buttons/ExtraNavButtonTopic.js @@ -5,7 +5,7 @@ import React, { PureComponent } from 'react'; import type { Dispatch, Narrow, Stream } from '../types'; import { connect } from '../react-redux'; import { getStreams } from '../selectors'; -import { streamNarrow } from '../utils/narrow'; +import { streamNameOfNarrow, streamNarrow } from '../utils/narrow'; import NavButton from '../nav/NavButton'; import { doNarrow } from '../actions'; @@ -19,7 +19,8 @@ type Props = $ReadOnly<{| class ExtraNavButtonTopic extends PureComponent { handlePress = () => { const { dispatch, narrow, streams } = this.props; - const stream = streams.find(x => x.name === narrow[0].operand); + const streamName = streamNameOfNarrow(narrow); + const stream = streams.find(x => x.name === streamName); if (stream) { dispatch(doNarrow(streamNarrow(stream.name))); } diff --git a/src/title-buttons/InfoNavButtonStream.js b/src/title-buttons/InfoNavButtonStream.js index 7f5db4d6baa..3f5426aff23 100644 --- a/src/title-buttons/InfoNavButtonStream.js +++ b/src/title-buttons/InfoNavButtonStream.js @@ -8,6 +8,7 @@ import { connect } from '../react-redux'; import { getStreams } from '../selectors'; import NavButton from '../nav/NavButton'; import { navigateToStream } from '../actions'; +import { streamNameOfNarrow } from '../utils/narrow'; type Props = $ReadOnly<{| dispatch: Dispatch, @@ -19,7 +20,8 @@ type Props = $ReadOnly<{| class InfoNavButtonStream extends PureComponent { handlePress = () => { const { narrow, streams } = this.props; - const stream = streams.find(x => x.name === narrow[0].operand); + const streamName = streamNameOfNarrow(narrow); + const stream = streams.find(x => x.name === streamName); if (stream) { NavigationService.dispatch(navigateToStream(stream.stream_id)); } diff --git a/src/title/TitleStream.js b/src/title/TitleStream.js index 7f4c5d5bdd2..25eefb43548 100644 --- a/src/title/TitleStream.js +++ b/src/title/TitleStream.js @@ -7,7 +7,7 @@ import type { Narrow, Stream, Subscription, Dispatch } from '../types'; import styles, { createStyleSheet } from '../styles'; import { connect } from '../react-redux'; import StreamIcon from '../streams/StreamIcon'; -import { isTopicNarrow } from '../utils/narrow'; +import { isTopicNarrow, topicOfNarrow } from '../utils/narrow'; import { getStreamInNarrow } from '../selectors'; import { showToast } from '../utils/info'; @@ -57,11 +57,11 @@ class TitleStream extends PureComponent { {isTopicNarrow(narrow) && ( { - showToast(narrow[1].operand); + showToast(topicOfNarrow(narrow)); }} > - {narrow[1].operand} + {topicOfNarrow(narrow)} )} diff --git a/src/title/titleSelectors.js b/src/title/titleSelectors.js index 2191d732487..b0db78670c6 100644 --- a/src/title/titleSelectors.js +++ b/src/title/titleSelectors.js @@ -1,6 +1,6 @@ /* @flow strict-local */ import type { Narrow, GlobalState } from '../types'; -import { isStreamOrTopicNarrow } from '../utils/narrow'; +import { isStreamOrTopicNarrow, streamNameOfNarrow } from '../utils/narrow'; import { getSubscriptionsByName } from '../subscriptions/subscriptionSelectors'; export const DEFAULT_TITLE_BACKGROUND_COLOR = 'transparent'; @@ -16,6 +16,6 @@ export const getTitleBackgroundColor = (state: GlobalState, narrow?: Narrow) => if (!narrow || !isStreamOrTopicNarrow(narrow)) { return DEFAULT_TITLE_BACKGROUND_COLOR; } - const streamName = narrow[0].operand; + const streamName = streamNameOfNarrow(narrow); return subscriptionsByName.get(streamName)?.color ?? 'gray'; }; diff --git a/src/topics/topicActions.js b/src/topics/topicActions.js index 255528d9219..0b7efe74bc3 100644 --- a/src/topics/topicActions.js +++ b/src/topics/topicActions.js @@ -2,7 +2,7 @@ import type { GetState, Dispatch, Narrow, Topic, Action, Outbox, Stream } from '../types'; import * as api from '../api'; import { INIT_TOPICS } from '../actionConstants'; -import { isStreamNarrow } from '../utils/narrow'; +import { isStreamNarrow, streamNameOfNarrow } from '../utils/narrow'; import { getAuth, getStreams } from '../selectors'; import { deleteOutboxMessage } from '../actions'; import { getOutbox } from '../directSelectors'; @@ -29,9 +29,10 @@ export const fetchTopicsForStream = (narrow: Narrow) => async ( if (!isStreamNarrow(narrow)) { return; } + const streamName = streamNameOfNarrow(narrow); const streams = getStreams(state); - const stream = streams.find(sub => narrow[0].operand === sub.name); + const stream = streams.find(sub => streamName === sub.name); if (!stream) { return; } diff --git a/src/topics/topicSelectors.js b/src/topics/topicSelectors.js index ab0e6268e6c..4d39beacbac 100644 --- a/src/topics/topicSelectors.js +++ b/src/topics/topicSelectors.js @@ -17,7 +17,7 @@ import { getMute, getStreams, getTopics, getUnreadStreams } from '../directSelec import { getShownMessagesForNarrow } from '../chat/narrowsSelectors'; import { getStreamsById } from '../subscriptions/subscriptionSelectors'; import { NULL_ARRAY } from '../nullObjects'; -import { isStreamNarrow } from '../utils/narrow'; +import { isStreamNarrow, streamNameOfNarrow } from '../utils/narrow'; export const getTopicsForNarrow: Selector = createSelector( (state, narrow) => narrow, @@ -27,8 +27,9 @@ export const getTopicsForNarrow: Selector = createSelector( if (!isStreamNarrow(narrow)) { return NULL_ARRAY; } - const stream = streams.find(x => x.name === narrow[0].operand); + const streamName = streamNameOfNarrow(narrow); + const stream = streams.find(x => x.name === streamName); if (!stream || !topics[stream.stream_id]) { return NULL_ARRAY; } diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 3e5d9dfa864..9ecb261f25f 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -393,6 +393,26 @@ export const emailsOfGroupPmNarrow = (narrow: Narrow): string[] => export const emailsOfPmNarrow = (narrow: Narrow): string[] => caseNarrowPartial(narrow, { pm: emails => emails }); +/** + * The stream name for a stream or topic narrow; else error. + * + * Most callers of this should probably be getting passed a stream name + * 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 }); + +/** + * The topic for a topic narrow; else error. + * + * Most callers of this should probably be getting passed a topic (and a + * stream name) instead of a Narrow in the first place; or if they do handle + * other kinds of narrows, should be using `caseNarrow`. + */ +export const topicOfNarrow = (narrow: Narrow): string => + caseNarrowPartial(narrow, { topic: (streamName, topic) => topic }); + export const isPmNarrow = (narrow?: Narrow): boolean => !!narrow && caseNarrowDefault(narrow, { pm: () => true }, () => false); From 7b8a4fd171b6770cba2e733f3b9f0ddc018e9a3a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 20:39:31 -0800 Subject: [PATCH 11/15] narrow [nfc]: Add getter emailOfPm1to1Narrow, and use it. --- src/title-buttons/InfoNavButtonPrivate.js | 3 ++- src/utils/narrow.js | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/title-buttons/InfoNavButtonPrivate.js b/src/title-buttons/InfoNavButtonPrivate.js index 1495200cfac..fcd70b7e076 100644 --- a/src/title-buttons/InfoNavButtonPrivate.js +++ b/src/title-buttons/InfoNavButtonPrivate.js @@ -8,6 +8,7 @@ import { connect } from '../react-redux'; import NavButton from '../nav/NavButton'; import { navigateToAccountDetails } from '../actions'; import { getUserForEmail } from '../users/userSelectors'; +import { emailOfPm1to1Narrow } from '../utils/narrow'; type SelectorProps = $ReadOnly<{| userId: number, @@ -34,5 +35,5 @@ class InfoNavButtonPrivate extends PureComponent { } export default connect((state, props) => ({ - userId: getUserForEmail(state, props.narrow[0].operand).user_id, + userId: getUserForEmail(state, emailOfPm1to1Narrow(props.narrow)).user_id, }))(InfoNavButtonPrivate); diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 9ecb261f25f..d5435175635 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -1,4 +1,5 @@ /* @flow strict-local */ +import invariant from 'invariant'; import isEqual from 'lodash.isequal'; import type { ApiNarrow, Message, Outbox, User, UserOrBot } from '../types'; @@ -384,6 +385,25 @@ export const emailsOfGroupPmNarrow = (narrow: Narrow): string[] => }, }); +/** + * The "other" user's email for a 1:1 PM narrow; else error. + * + * With "other" in scare-quotes because for the self-PM narrow, this is the + * self user. + * + * Any caller of this probably should be getting a whole user object instead + * of a Narrow in the first place. And then that wrinkle about the self-PM + * narrow vs. other 1:1 narrows is a UI decision that can get made + * explicitly at the appropriate spot. + */ +export const emailOfPm1to1Narrow = (narrow: Narrow): string => + caseNarrowPartial(narrow, { + pm: emails => { + invariant(emails.length === 1, 'emailOfPm1to1Narrow: got group-PM narrow'); + return emails[0]; + }, + }); + /** * The "PM key recipients" emails for a PM narrow; else error. * From caeaf930cc618e4be8c7399b495be65dd40c08e9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 19:43:07 -0800 Subject: [PATCH 12/15] narrow [nfc]: Use sensible explicit bits of API for home narrow, too. --- src/message/__tests__/renderMessages-test.js | 2 +- src/search/SearchMessagesCard.js | 2 +- src/utils/message.js | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/message/__tests__/renderMessages-test.js b/src/message/__tests__/renderMessages-test.js index 110111f4f7f..079c14db968 100644 --- a/src/message/__tests__/renderMessages-test.js +++ b/src/message/__tests__/renderMessages-test.js @@ -4,7 +4,7 @@ import { HOME_NARROW } from '../../utils/narrow'; import renderMessages from '../renderMessages'; describe('renderMessages', () => { - const narrow = deepFreeze([]); + const narrow = deepFreeze(HOME_NARROW); test('empty messages results in a single empty section', () => { const messageList = renderMessages([], HOME_NARROW); diff --git a/src/search/SearchMessagesCard.js b/src/search/SearchMessagesCard.js index f38dcc67b28..640f7278ca5 100644 --- a/src/search/SearchMessagesCard.js +++ b/src/search/SearchMessagesCard.js @@ -45,7 +45,7 @@ export default class SearchMessagesCard extends PureComponent { return ; } - const renderedMessages = renderMessages(messages, []); + const renderedMessages = renderMessages(messages, HOME_NARROW); return ( diff --git a/src/utils/message.js b/src/utils/message.js index ba0bce5ef65..a5e3be9cead 100644 --- a/src/utils/message.js +++ b/src/utils/message.js @@ -1,6 +1,6 @@ /* @flow strict-local */ import type { Narrow, Message, MuteState, Outbox, Subscription } from '../types'; -import { isTopicNarrow } from './narrow'; +import { isHomeNarrow, isTopicNarrow } from './narrow'; import { streamNameOfStreamMessage } from './recipient'; export const isTopicMuted = (stream: string, topic: string, mute: MuteState = []): boolean => @@ -22,7 +22,7 @@ export const shouldBeMuted = ( const streamName = streamNameOfStreamMessage(message); - if (narrow.length === 0) { + if (isHomeNarrow(narrow)) { const sub = subscriptions.find(x => x.name === streamName); if (!sub || !sub.in_home_view) { return true; From e7485e41bca5dc70ff54d55708dc9c0e13103659 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 21:06:51 -0800 Subject: [PATCH 13/15] narrow tests: Stop caring about the internal representation entirely. Instead, use the getters and predicates that make up the `narrow` module's actual interface. --- src/utils/__tests__/narrow-test.js | 53 ++++++++---------------------- 1 file changed, 14 insertions(+), 39 deletions(-) diff --git a/src/utils/__tests__/narrow-test.js b/src/utils/__tests__/narrow-test.js index b68de70f6a2..d11d60e4620 100644 --- a/src/utils/__tests__/narrow-test.js +++ b/src/utils/__tests__/narrow-test.js @@ -26,28 +26,24 @@ import { MENTIONED_NARROW, pm1to1NarrowFromUser, keyFromNarrow, + streamNameOfNarrow, + topicOfNarrow, + emailOfPm1to1Narrow, } from '../narrow'; import type { Narrow, Message } from '../../types'; import * as eg from '../../__tests__/lib/exampleData'; describe('HOME_NARROW', () => { - test('produces an empty list', () => { - expect(HOME_NARROW).toEqual([]); - }); - test('is a home narrow', () => { expect(isHomeNarrow(HOME_NARROW)).toBe(true); }); }); describe('pmNarrowFromEmail', () => { - test('produces an one item list, pm-with operator and single email', () => { - expect(pmNarrowFromEmail(eg.otherUser.email)).toEqual([ - { - operator: 'pm-with', - operand: eg.otherUser.email, - }, - ]); + test('produces a 1:1 narrow', () => { + const narrow = pmNarrowFromEmail(eg.otherUser.email); + expect(is1to1PmNarrow(narrow)).toBeTrue(); + expect(emailOfPm1to1Narrow(narrow)).toEqual(eg.otherUser.email); }); test('if operator is "pm-with" and only one email, then it is a private narrow', () => { @@ -80,15 +76,6 @@ describe('isStreamOrTopicNarrow', () => { }); describe('specialNarrow', () => { - test('produces a narrow with "is" operator', () => { - expect(STARRED_NARROW).toEqual([ - { - operator: 'is', - operand: 'starred', - }, - ]); - }); - test('only narrowing with the "is" operator is special narrow', () => { expect(isSpecialNarrow(undefined)).toBe(false); expect(isSpecialNarrow(HOME_NARROW)).toBe(false); @@ -99,12 +86,9 @@ describe('specialNarrow', () => { describe('streamNarrow', () => { test('narrows to messages from a specific stream', () => { - expect(streamNarrow('some stream')).toEqual([ - { - operator: 'stream', - operand: 'some stream', - }, - ]); + const narrow = streamNarrow('some stream'); + expect(isStreamNarrow(narrow)).toBeTrue(); + expect(streamNameOfNarrow(narrow)).toEqual('some stream'); }); test('only narrow with operator of "stream" is a stream narrow', () => { @@ -116,10 +100,10 @@ describe('streamNarrow', () => { describe('topicNarrow', () => { test('narrows to a specific topic within a specified stream', () => { - expect(topicNarrow('some stream', 'some topic')).toEqual([ - { operator: 'stream', operand: 'some stream' }, - { operator: 'topic', operand: 'some topic' }, - ]); + const narrow = topicNarrow('some stream', 'some topic'); + expect(isTopicNarrow(narrow)).toBeTrue(); + expect(streamNameOfNarrow(narrow)).toEqual('some stream'); + expect(topicOfNarrow(narrow)).toEqual('some topic'); }); test('only narrow with two items, one for stream, one for topic is a topic narrow', () => { @@ -130,15 +114,6 @@ describe('topicNarrow', () => { }); describe('SEARCH_NARROW', () => { - test('produces a narrow for a search query', () => { - expect(SEARCH_NARROW('some query')).toEqual([ - { - operator: 'search', - operand: 'some query', - }, - ]); - }); - test('narrow with "search" operand is a search narrow', () => { expect(isSearchNarrow(undefined)).toBe(false); expect(isSearchNarrow(HOME_NARROW)).toBe(false); From ca745d9efc2c5c9a10e1c822aef50d4da545cc6a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 20:47:00 -0800 Subject: [PATCH 14/15] narrow [nfc]: Explicitly convert to ApiNarrow where appropriate. This conversion is a no-op. But we'll shortly be converting `Narrow` into an opaque type -- which will make it a no-op that only the `narrow` module can perform. That means Flow will check for us that we've explicitly marked all the places where we intend this conversion to happen; these are those places. --- src/message/fetchActions.js | 5 +++-- src/utils/narrow.js | 7 ++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index 7f1c2bcc410..e90c4196513 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -24,7 +24,7 @@ import { MESSAGE_FETCH_COMPLETE, } from '../actionConstants'; import { FIRST_UNREAD_ANCHOR, LAST_MESSAGE_ANCHOR } from '../anchor'; -import { ALL_PRIVATE_NARROW } from '../utils/narrow'; +import { ALL_PRIVATE_NARROW, apiNarrowOfNarrow } from '../utils/narrow'; import { BackoffMachine } from '../utils/async'; import { initNotifications } from '../notification/notificationActions'; import { addToOutbox, sendOutbox } from '../outbox/outboxActions'; @@ -88,6 +88,7 @@ export const fetchMessages = (fetchArgs: {| try { const { messages, found_newest, found_oldest } = await api.getMessages(getAuth(getState()), { ...fetchArgs, + narrow: apiNarrowOfNarrow(fetchArgs.narrow), useFirstUnread: fetchArgs.anchor === FIRST_UNREAD_ANCHOR, // TODO: don't use this; see #4203 }); dispatch( @@ -235,7 +236,7 @@ export const fetchMessagesInNarrow = ( const fetchPrivateMessages = () => async (dispatch: Dispatch, getState: GetState) => { const auth = getAuth(getState()); const { messages, found_newest, found_oldest } = await api.getMessages(auth, { - narrow: ALL_PRIVATE_NARROW, + narrow: apiNarrowOfNarrow(ALL_PRIVATE_NARROW), anchor: LAST_MESSAGE_ANCHOR, numBefore: 100, numAfter: 0, diff --git a/src/utils/narrow.js b/src/utils/narrow.js index d5435175635..7e1bcb5eb1b 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -29,7 +29,7 @@ import { * into a data structure; * * `caseNarrow` and its relatives, for pattern-matching or destructuring; * * `ApiNarrow` for the form we put a narrow in when talking to the - * server. + * server, and `apiNarrowOfNarrow` for converting to it. */ export type Narrow = ApiNarrow; @@ -459,6 +459,11 @@ export const isStreamOrTopicNarrow = (narrow?: Narrow): boolean => export const isSearchNarrow = (narrow?: Narrow): boolean => !!narrow && caseNarrowDefault(narrow, { search: () => true }, () => false); +/** + * Convert the narrow into the form used in the Zulip API at get-messages. + */ +export const apiNarrowOfNarrow = (narrow: Narrow): ApiNarrow => narrow; + /** * True just if the given message is part of the given narrow. * From 1ca184d6cd7d4ac146e0e8fac41905c29bfc7f19 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 11 Dec 2020 17:42:21 -0800 Subject: [PATCH 15/15] narrow types: Make Narrow an opaque type. In the preceding series of commits, we've taken all the places where our code used to depend on the details of our internal representation of narrows, and converted them so they use reasonable explicit interfaces provided by the `narrow` module. This commit recruits Flow to confirm for us that that job is complete. Because `Narrow` is now an opaque type, code outside this module now has to treat it as if it could mean anything: so it can't create a value of this type any more than it could of type `empty`, and can't consume one or break it down any more than it could a value of type `mixed`, the supertype of all types. Effectively this means that code outside `narrow` can only create a `Narrow` by invoking something inside the module; can then pass it around and store it however it pleases; but then can only inspect the information inside it by passing it back to something within the module, or to an operation like `===` or `JSON.stringify` that's completely generic so that it actually accepts type `mixed` as input. (The way this series was developed is that this commit came first, immediately after distinguishing the `ApiNarrow` type and making `Narrow` an alias of it in the first place. That produced a long list of Flow errors, which served as a to-do list to make the other changes in the series. Once complete, this commit was rebased to the end so that, as always, no commit in the series breaks our tests.) --- src/utils/narrow.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/narrow.js b/src/utils/narrow.js index 7e1bcb5eb1b..5ffed03fe2b 100644 --- a/src/utils/narrow.js +++ b/src/utils/narrow.js @@ -31,7 +31,7 @@ import { * * `ApiNarrow` for the form we put a narrow in when talking to the * server, and `apiNarrowOfNarrow` for converting to it. */ -export type Narrow = ApiNarrow; +export opaque type Narrow = ApiNarrow; export const isSameNarrow = (narrow1: Narrow, narrow2: Narrow): boolean => Array.isArray(narrow1) && Array.isArray(narrow2) && isEqual(narrow1, narrow2);