-
-
Notifications
You must be signed in to change notification settings - Fork 676
Allow viewing message edit history #4173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
agrawal-d
wants to merge
5
commits into
zulip:main
Choose a base branch
from
agrawal-d:edit-history
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
a22382f
types: Improve type definition of MessageSnapshot.
agrawal-d 6f10593
webview: Refactor core logic into a common component.
agrawal-d fd9c98a
webview: Refactor 'html.js' to reuse base HTML.
agrawal-d e53b460
webview: Set up pipeline to render edit history.
agrawal-d 2e45013
message: Add feature to view edit history.
agrawal-d File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| /* @flow strict-local */ | ||
| import React from 'react'; | ||
| import { Platform, NativeModules } from 'react-native'; | ||
| import { WebView } from 'react-native-webview'; | ||
| import type { WebViewMessageEvent, WebViewNavigation } from 'react-native-webview'; | ||
| import { tryParseUrl } from '../utils/url'; | ||
| import * as logging from '../utils/logging'; | ||
|
|
||
| /** | ||
| * Returns the `onShouldStartLoadWithRequest` function for webviews for a | ||
| * given base URL. | ||
| * | ||
| * Paranoia^WSecurity: only load `baseUrl`, and only load it once. Any other | ||
| * requests should be handed off to the OS, not loaded inside the WebView. | ||
| */ | ||
| const getOnShouldStartLoadWithRequest = (baseUrl: string) => { | ||
| const onShouldStartLoadWithRequest: (event: WebViewNavigation) => boolean = (() => { | ||
| // Inner closure to actually test the URL. | ||
| const urlTester: (url: string) => boolean = (() => { | ||
| // On Android this function is documented to be skipped on first load: | ||
| // therefore, simply never return true. | ||
| if (Platform.OS === 'android') { | ||
| return (url: string) => false; | ||
| } | ||
|
|
||
| // Otherwise (for iOS), return a closure that evaluates to `true` _exactly | ||
| // once_, and even then only if the URL looks like what we're expecting. | ||
| let loaded_once = false; | ||
| return (url: string) => { | ||
| const parsedUrl = tryParseUrl(url); | ||
| if (!loaded_once && parsedUrl && parsedUrl.toString() === baseUrl.toString()) { | ||
| loaded_once = true; | ||
| return true; | ||
| } | ||
| return false; | ||
| }; | ||
| })(); | ||
|
|
||
| // Outer closure to perform logging. | ||
| return (event: WebViewNavigation) => { | ||
| const ok = urlTester(event.url); | ||
| if (!ok) { | ||
| logging.warn('webview: rejected navigation event', { | ||
| navigation_event: { ...event }, | ||
| expected_url: baseUrl.toString(), | ||
| }); | ||
| } | ||
| return ok; | ||
| }; | ||
| })(); | ||
|
|
||
| return onShouldStartLoadWithRequest; | ||
| }; | ||
|
|
||
| /** | ||
| * The URL of the platform-specific assets folder. | ||
| * | ||
| * - On iOS: We can't easily hardcode this because it includes UUIDs. | ||
| * So we bring it over the React Native bridge in ZLPConstants.m. | ||
| * | ||
| * - On Android: Different apps' WebViews see different (virtual) root | ||
| * directories as `file:///`, and in particular the WebView provides | ||
| * the APK's `assets/` directory as `file:///android_asset/`. [1] | ||
| * We can easily hardcode that, so we do. | ||
| * | ||
| * [1] Oddly, this essential feature doesn't seem to be documented! It's | ||
| * widely described in how-tos across the web and StackOverflow answers. | ||
| * It's assumed in some related docs which mention it in passing, and | ||
| * treated matter-of-factly in some Chromium bug threads. Details at: | ||
| * https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/android.20filesystem/near/796440 | ||
| */ | ||
| const assetsUrl = | ||
| Platform.OS === 'ios' | ||
| ? new URL(NativeModules.ZLPConstants.resourceURL) | ||
| : new URL('file:///android_asset/'); | ||
|
|
||
| /** | ||
| * The URL of the webview-assets folder. | ||
| * | ||
| * This is the folder populated at build time by `tools/build-webview`. | ||
| */ | ||
| const webviewAssetsUrl = new URL('webview/', assetsUrl); | ||
|
|
||
| type Props = $ReadOnly<{| | ||
| html: string, | ||
| onMessage?: WebViewMessageEvent => mixed, | ||
| onError?: (event: mixed) => void, | ||
| |}>; | ||
|
|
||
| /** | ||
| * A wrapper over React Native Webview. Internally configures basic functionality | ||
| * and security. | ||
| * | ||
| * This is a stateless function component, to ensure there are no un-necessary | ||
| * re-renders of the webview due to state changes. | ||
| * | ||
| * @prop html The HTML to be rendered. | ||
| * @prop onMessage Message event handler for the webview. | ||
| * @prop onError Error event handler for the webview. | ||
| */ | ||
| const ZulipWebView = function ZulipWebView( | ||
| props: $ReadOnly<{| | ||
| ...Props, | ||
| innerRef: ((null | WebView) => mixed) | { current: null | WebView }, | ||
| |}>, | ||
| ) { | ||
| const { onMessage, onError, innerRef } = props; | ||
|
|
||
| /** | ||
| * Effective URL of the MessageList webview. | ||
| * | ||
| * It points to `index.html` in the webview-assets folder, which | ||
| * doesn't exist. | ||
| * | ||
| * It doesn't need to exist because we provide all HTML at | ||
| * creation (or refresh) time. This serves only as a placeholder, | ||
| * so that relative URLs (e.g., to `base.css`, which does exist) | ||
| * and cross-domain security restrictions have somewhere to | ||
| * believe that this document originates from. | ||
| */ | ||
| const baseUrl = new URL('index.html', webviewAssetsUrl); | ||
|
|
||
| // Needs to be declared explicitly to prevent Flow from getting confused. | ||
| const html: string = props.html; | ||
|
|
||
| // The `originWhitelist` and `onShouldStartLoadWithRequest` props are | ||
| // meant to mitigate possible XSS bugs, by interrupting an attempted | ||
| // exploit if it tries to navigate to a new URL by e.g. setting | ||
| // `window.location`. | ||
| // | ||
| // Note that neither of them is a hard security barrier; they're checked | ||
| // only against the URL of the document itself. They cannot be used to | ||
| // validate the URL of other resources the WebView loads. | ||
| // | ||
| // Worse, the `originWhitelist` parameter is completely broken. See: | ||
| // https://github.com/react-native-community/react-native-webview/pull/697 | ||
| return ( | ||
| <WebView | ||
| source={{ baseUrl: (baseUrl.toString(): string), html }} | ||
| originWhitelist={['file://']} | ||
| onShouldStartLoadWithRequest={getOnShouldStartLoadWithRequest(baseUrl.toString())} | ||
| style={{ backgroundColor: 'transparent' }} | ||
| onMessage={onMessage} | ||
| onError={onError} | ||
| ref={innerRef} | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
| export default React.forwardRef<Props, WebView>((props, ref) => ( | ||
| <ZulipWebView innerRef={ref} {...props} /> | ||
| )); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| /* @flow strict-local */ | ||
|
|
||
| import React from 'react'; | ||
| import { View } from 'react-native'; | ||
| import type { NavigationScreenProp } from 'react-navigation'; | ||
| import type { Dispatch, Auth, ThemeName, UserOrBot } from '../types'; | ||
| import SpinningProgress from '../common/SpinningProgress'; | ||
| import type { MessageSnapshot } from '../api/modelTypes'; | ||
| import { connect } from '../react-redux'; | ||
| import { getAuth, getSettings, getAllUsersById } from '../selectors'; | ||
| import { Screen, ZulipWebView } from '../common'; | ||
| import { showToast } from '../utils/info'; | ||
| import * as api from '../api'; | ||
| import editHistoryHtml from '../webview/html/editHistoryHtml'; | ||
|
|
||
| type SelectorProps = {| | ||
| auth: Auth, | ||
| usersById: Map<number, UserOrBot>, | ||
| themeName: ThemeName, | ||
| |}; | ||
|
|
||
| type Props = $ReadOnly<{| | ||
| navigation: NavigationScreenProp<{ params: {| messageId: number |} }>, | ||
|
|
||
| dispatch: Dispatch, | ||
| ...SelectorProps, | ||
| |}>; | ||
|
|
||
| type State = $ReadOnly<{| | ||
| messageHistory: MessageSnapshot[] | null, | ||
| |}>; | ||
|
|
||
| class EditHistory extends React.Component<Props, State> { | ||
| state = { | ||
| messageHistory: null, | ||
| }; | ||
|
|
||
| componentDidMount() { | ||
| const { auth, navigation } = this.props; | ||
|
|
||
| api | ||
| .getMessageHistory(auth, navigation.state.params.messageId) | ||
| .then(response => { | ||
| this.setState({ | ||
| messageHistory: response.message_history, | ||
| }); | ||
| }) | ||
| .catch(err => { | ||
| navigation.goBack(); | ||
| showToast('An error occurred while loading edit history'); | ||
| }); | ||
| } | ||
|
|
||
| render() { | ||
| const { messageHistory } = this.state; | ||
|
|
||
| if (messageHistory === null) { | ||
| return ( | ||
| <Screen title="Edit History"> | ||
| <View style={{ justifyContent: 'center', alignItems: 'center', flex: 1 }}> | ||
| <SpinningProgress color="white" size={48} /> | ||
| </View> | ||
| </Screen> | ||
| ); | ||
| } | ||
| const { usersById, auth, themeName } = this.props; | ||
|
|
||
| return ( | ||
| <Screen title="Edit History"> | ||
| <ZulipWebView | ||
| html={editHistoryHtml(messageHistory, themeName, usersById, auth)} | ||
| onError={(msg: mixed) => { | ||
| // eslint-disable-next-line no-console | ||
| console.error(msg); | ||
| }} | ||
| /> | ||
| </Screen> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| export default connect<SelectorProps, _, _>((state, props) => ({ | ||
| auth: getAuth(state), | ||
| usersById: getAllUsersById(state), | ||
| themeName: getSettings(state).theme, | ||
| }))(EditHistory); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like you've tested at least the common cases (a topic was edited but not the content, or vice versa) and seen that these fields fields were there in those cases.
I suppose, to make this entirely true, we have to also be sure that there won't be any surprising cases we haven't tested yet. It might be easier to post in #mobile-team with any questions about the docs, and that may have the side-effect of improving the docs for the next person who has the same questions. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That discussion here:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/get-message-history.20docs/near/921587
Whenever we find a place where the API docs seem to be wrong, I always want to discuss it in chat and bring it to Tim's attention there. Reasons include:
As of right now (since a few minutes ago), the status is that Tim confirms the current behavior is what you're seeing empirically -- but potentially wants to fix it by changing the behavior to match the docs instead of vice versa.