Allow viewing message edit history #4173

agrawal-d · 2020-06-29T08:26:07Z

chrisbobbe

I'm not sure why, but I'm just getting a blank page in the WebView on my iOS device; it just says about:blank. Did you run into this on Android, and was there something special you had to do to get it to work?

chrisbobbe · 2020-06-30T20:32:30Z

src/webview/static/base.css

+  background-color: #fcdcd8;
+  text-decoration: line-through;
+  word-break: break-all;
+}


Nit: newline at end of file

chrisbobbe · 2020-06-30T22:05:21Z

src/api/modelTypes.js


-  /** Docs unclear but suggest absent if only content edited. */
-  topic?: string,


by experimenting it's possible to determine which fields are always present.

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. 🙂

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:

I want the API docs to converge on being right too 🙂 -- if we have a steady state where our types say one thing and the API docs say another, that'll be confusing to a later reader

If for whatever reason the API docs don't get updated promptly, I'll want the comments in our code to be super explicit that the API docs say X but the truth is Y, and here's how we've determined the truth is Y. That gives the future reader a hope of not being confused by the contradiction, because at least they can see that the author of the types was aware of what the API docs said, and wasn't just misreading them, or working from what at that future time is an out-of-date version

The answer might be that the documented API is as intended, and the server's behavior that disagrees is a bug!

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.

agrawal-d · 2020-07-01T17:43:07Z

I'm not sure why, but I'm just getting a blank page in the WebView on my iOS device; it just says about:blank. Did you run into this on Android, and was there something special you had to do to get it to work?

Oh no, that's bad!
Nope, I never ran into this on Android, and didn't do anything special to get it to work. The only command I needed to run this is yarn run android.

Just tested this again, and it worked.

agrawal-d · 2020-07-01T18:11:24Z

The API Guide says:

Note that using static HTML requires the WebView property originWhiteList to ['*'].

@chrisbobbe Does changing originWhitleist in EditHistory.js to this fix the issue for you on iOS? I can't test on iOS as I don't have an iOS device.

chrisbobbe · 2020-07-03T20:35:08Z

src/webview/html/editHistoryHtml.js

+
+    default:
+      // eslint-disable-next-line no-console
+      console.error('Invalid edit_flag');


Let's get this logged to Sentry, with logging.error, and include the value of edit_flag in the second argument, extras.

Ok
(It's actually impossible to reach here :p)

(It's actually impossible to reach here :p)

Quick tip: you can add ensureUnreachable(edit_flag); here to assert this to the type-checker (and ask it to verify that it can prove it.) That helps prevent errors before they even reach users 🙂 if say a new case is added here in the future.

I'm getting the following error on adding that:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/webview/html/editHistoryHtml.js:76:25 Cannot call ensureUnreachable with edit_flag bound to x because: • string [1] is incompatible with empty [2]. • string [3] is incompatible with empty [2]. • string [4] is incompatible with empty [2]. • string [5] is incompatible with empty [2]. src/webview/html/editHistoryHtml.js [1] 10│ const TOPIC_EDITED = 'TOPIC_EDITED'; [3] 11│ const CONTENT_EDITED = 'CONTENT_EDITED'; [4] 12│ const BOTH_EDITED = 'BOTH_EDITED'; [5] 13│ const NOT_EDITED = 'NOT_EDITED'; : 73│ } 74│ 75│ default: 76│ ensureUnreachable(edit_flag); 77│ logging.error('Invalid edit_flag', { editFlag: edit_flag }); 78│ break; 79│ } src/types.js [2] 42│ export function ensureUnreachable(x: empty) {} Found 4 errors

I think Flow is not able to understand that this case is unreachable?

Ah -- it's that the type of edit_flag is simply string, instead of being an enum 'TOPIC_EDITED' | 'CONTENT_EDITED' | 'BOTH_EDITED' | 'NOT_EDITED'.

You could make it an enum by annotating it with that type (and then probably doing something to avoid initializing to undefined and mutating.)

Or, looking a bit more at the code: this flag is so short-lived, just from an if/else-if chain to a switch right after it. I think it'd simplify things to just fuse those together: have an if/else-if chain with those conditions, and with the cases' bodies as their bodies. Or, a bit further: a two-level tree of if/else statements:

if (snapshot.prev_content === undefined) { if (snapshot.prev_topic === undefined) { … } else { … } } else { … }

src/webview/html/editHistoryHtml.js

agrawal-d · 2020-07-04T08:10:39Z

Thanks for the review, @chrisbobbe, and for the detailed investigation on CZO. I've pushed some changes, and I think it may work on iOS too, for now.

chrisbobbe · 2020-07-06T18:42:12Z

(For the record: the comments I made on CZO were here).

gnprice · 2020-07-07T18:27:01Z

src/webview/MessageList.js

    // 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.


No need to have three copies of this comment 🙂

I think the jsdoc on the newly-factored-out function is the right place to keep it -- it's describing the function's interface. (Potentially we could have some other WebView in the future that is actually meant to load subsequent pages, and then this paragraph would explain why this function would no longer be the right logic to use for that WebView.)

gnprice · 2020-07-07T18:36:37Z

src/webview/getOnShouldStartLoadWithRequest.js

+ * 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.
+ */
+export default (baseUrl: string) => {


Let's move the assetsPath logic out to this new file too. Both that logic and this are basically gritty details of how the webview works -- we're interested in one at the same times we're interested in the other -- and both are distractions, where they currently live in MessageList, from the main job of that component.

gnprice · 2020-07-07T19:04:04Z

src/api/modelTypes.js


-  /** Docs unclear but suggest absent if only content edited. */
-  topic?: string,


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:

I want the API docs to converge on being right too 🙂 -- if we have a steady state where our types say one thing and the API docs say another, that'll be confusing to a later reader

If for whatever reason the API docs don't get updated promptly, I'll want the comments in our code to be super explicit that the API docs say X but the truth is Y, and here's how we've determined the truth is Y. That gives the future reader a hope of not being confused by the contradiction, because at least they can see that the author of the types was aware of what the API docs said, and wasn't just misreading them, or working from what at that future time is an out-of-date version

The answer might be that the documented API is as intended, and the server's behavior that disagrees is a bug!

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.

gnprice · 2020-07-07T19:07:59Z

src/api/modelTypes.js

+  // The prev_* field is only present if the respective attribute
+  // of the message was changed.
  prev_content?: string,
  prev_rendered_content?: string,
  content_html_diff?: string,
+  prev_topic?: string,


This summary is concise in a similar way to the corresponding bit of the API docs -- but I think it would actually be clearer (and not even many more words, if more at all) to be less abstract, more concrete and explicit:

// These are present just if the content was edited. prev_content?: string, prev_rendered_content?: string, content_html_diff?: string, // Present just if the topic was edited. prev_topic?: string,

(... OK, and I just counted: that's 16 words instead of 15. 😁 )

gnprice · 2020-07-07T19:28:00Z

src/webview/html/editHistoryHtml.js

+
+    default:
+      // eslint-disable-next-line no-console
+      console.error('Invalid edit_flag');


Ah -- it's that the type of edit_flag is simply string, instead of being an enum 'TOPIC_EDITED' | 'CONTENT_EDITED' | 'BOTH_EDITED' | 'NOT_EDITED'.

You could make it an enum by annotating it with that type (and then probably doing something to avoid initializing to undefined and mutating.)

Or, looking a bit more at the code: this flag is so short-lived, just from an if/else-if chain to a switch right after it. I think it'd simplify things to just fuse those together: have an if/else-if chain with those conditions, and with the cases' bodies as their bodies. Or, a bit further: a two-level tree of if/else statements:

if (snapshot.prev_content === undefined) { if (snapshot.prev_topic === undefined) { … } else { … } } else { … }

gnprice

Thanks @agrawal-d ! This will be great.

Some comments below, and also above because GitHub sometimes feels like doing that 😝 (Specifically, it feels like doing that when I reply to an existing comment thread while partway through preparing a review.)

gnprice · 2020-07-07T19:32:50Z

src/webview/html/editHistoryHtml.js

+  let displayedContent: string = '';
+  let tag: string = '';
+  let edit_flag;


This is a lot of mutable let bindings. In general we prefer const -- it makes the code simpler to reason about both for us human readers, and for the type-checker.

Looking at how they're used, each of these ends up always being mutated exactly once before it's used. So that suggests that what's "really" going on is we're initializing each of these bindings to a value and then never mutating it thereafter -- which is what const expresses. I think we can pretty naturally rearrange this logic so that it works that way explicitly.

Specifically, one natural way to refactor this that expresses the real structure more directly would be to pull out a helper function that returns tag and displayedContent. (And that in turn could use a helper returning edit_flag, or the logic that produces and consumes that flag could just be fused together as I mentioned at #4173 (comment) .)

gnprice · 2020-07-07T19:42:43Z

src/webview/html/editHistoryHtml.js

+        user ? user.full_name : 'unknown user'
+      }</span>`;
+
+      // prev_topic is always present for this case. $FlowFixMe.


Hmm, I missed these fixmes the first couple of times I looked at this code!

Please always put fixme comments at the start of the comment line. (Ditto for TODO, and for any other special keywords that are meant to jump out at the reader to draw scrutiny.)

gnprice · 2020-07-07T19:50:31Z

src/webview/html/editHistoryHtml.js

+      // prev_topic and content_html_diff are always present for this case. $FlowFixMe.
+      displayedContent = template`Topic:
+      <span class="highlight_text_inserted">${snapshot.topic}</span>
+      <span class="highlight_text_deleted">${snapshot.prev_topic}</span>
+      <br/>$!${snapshot.content_html_diff}`;


And now that I look at the fixmes, I definitely prefer the plan where these cases just get pushed into the if and else bodies. 😄

If this were inside the if or else, Flow would immediately follow this logic around prev_topic. It doesn't know the correlation between prev_content and content_html_diff... but we could simply use the latter in the conditional, since that's what we're actually relying on.

(Alternatively, in some other circumstances it can certainly make sense to have this flavor of "if this condition, then that property is present / non-nullish" logic that's twistier than the type-checker is able to follow. When we do, instead of a fixme what I'd much rather have is an assert, throwing an exception if the impossible happens -- that way (a) it's explicit in the actual code exactly what assumption we're making, (b) Flow can check that we're relying only on those assumptions we say we're making, and (c) if we're wrong (perhaps after future changes to the code by someone else), we get an exception so we can learn about the problem.)

gnprice · 2020-07-07T20:06:15Z

src/webview/html/editHistoryHtml.js

+  return template`
+$!${script(null, auth)}
+$!${css(theme)}
+<meta name="viewport" content="width=device-width, initial-scale=1, user-scalable=no">
+<body style="overflow-x: hidden;">
+$!${html}
+</body>`;


There are some subtle bits here 🙂. Particularly that meta element -- I can only guess at what it does, and would have to look it up.

And this sure has a lot in common with the code in html.js! In particular, the subtle bits are exactly the same. (Then the latter has one additional subtle bit, webkitBugWorkaround, which is likely unnecessary here but would be harmless.)

Anything that's subtle, I am keen on not duplicating. When code is duplicated, future bugfixes and incidental changes tend to go on just one or the other -- and then it's self-reinforcing: as they diverge, it gets harder to make the same change on both of them or be sure the change would do the same thing on both, so more changes get applied to just the one where the bug happened to be spotted. So because we need this in two places, let's factor it out into one place we can call from both of them.

gnprice · 2020-07-07T20:36:03Z

src/webview/html/editHistoryHtml.js

+  return template`
+    <div class="edit-history-block">
+      <div class="static-timestamp">${editedTime}</div>
+      <span class="edit-history-content">$!${displayedContent}</span>
+      <div class="message-tags">$!${tag}</div>


I appreciate the careful use of template`…` , ${…}, and $!${…}! You may already know that this is an important surface area for preventing HTML injections.

Which we actually had, until a couple of years ago -- the day I discovered that, I fixed it and then built this little templating/escaping system to help us prevent it ever happening again.

I've looked closely at these templates, and I believe everything gets escaped correctly.

A couple of things I try to do to help keep it that way, as people make changes in the future:

Background ideas:

Conceptually there are two types here: text string, and HTML string. Sadly we haven't taught the type-checker the difference -- they're both string -- so we have to keep track of the distinction ourselves.

The ultimate return value is an HTML string. Most of the inputs are text strings, and in general any string that isn't explicitly meant to be used as raw HTML is a text string. The only way to get an HTML string from a text string is to hand it to template; and an interpolation in template should have $!$ if it's an HTML string and plain $ if it's a text string.

So the goal is to be able to look at a use of $ or $!$ -- especially a use of $!$ -- and immediately tell, without a lot of reading through other code, whether it looks right, or should be the other way instead.

One thing that helps: For each variable that represents a fragment that'll get used in templates, give it a name that's explicit about whether it's a text string or an HTML string. Especially if it's an HTML string, make that explicit. That way when there's an interpolation like $!${someVar}, and it's not a name like $!${someHtml}, it jumps out as suspicious.

This is why messageHeaderAsHtml.js has variables topicHtml, for example.

So here, tagHtml would be a good name for tag. Similarly displayedContentHtml.

Another thing that helps: make it so that every last backtick-interpolated string in the HTML-rendering code uses template. That way any that don't are suspicious.

This commit adds one non-template backtick-literal: editedTime. The behavior is fine, because it truly is a text string, and it gets duly escaped when it's used. But it'd be good to maintain that invariant, to help make any future oversights stand out. One simple way would be to just inline that expression in the place where editedTime is used.

For history see 9941453, where I fixed the HTML injection (in 2018-05.) Some discussion of these hygiene practices in af5a56f, a few commits later; template itself was introduced in the latter's parent.

Thanks for the detailed explaination!

gnprice · 2020-07-07T20:41:17Z

src/webview/static/base.css

+.edit-history-block{
+  padding:8px;
+  border-bottom:1px solid hsla(0, 0%, 50%, 0.25);
+}
+.edit-history-block .static-timestamp{
+  margin-top: 8px;
+  text-align: right;
+}
+.edit-history-content{
+  -webkit-user-select: text; /* Safari 3.1+ */
+  -moz-user-select: text; /* Firefox 2+ */
+  -ms-user-select: text; /* IE 10+ */
+  user-select: text; /* Standard syntax */
+  -khtml-user-select: text;
+  -webkit-touch-callout: text;
+  word-wrap: break-word;
+}
+.highlight_text_inserted {
+  color: #9effa1;
+  background-color: hsla(120,64%,95%,.3);
+}
+.highlight_text_deleted {
+  color: red;
+  background-color: #fcdcd8;
+  text-decoration: line-through;
+  word-break: break-all;
+}


Are these based on something in the webapp, or did you develop them afresh?

Either is fine 🙂 -- I'd just like to know (and have the commit message say so for future readers), and if it's from the webapp have a quick comment in the file saying where exactly it's from. (Including if it's based loosely or in part on something in the webapp.) That will save us some archaeological work in any future effort to unify the message CSS on web and mobile.

The .highlight_text_* CSS is borrowed form the web-app, the others are new.

gnprice · 2020-07-07T20:48:10Z

src/message/EditHistory.js

+        <WebView
+          source={{ baseUrl, html }}
+          originWhitelist={['file://']}
+          style={{ backgroundColor: 'transparent' }}
+          onShouldStartLoadWithRequest={getOnShouldStartLoadWithRequest(baseUrl)}
+          onError={(msg: mixed) => {
+            // eslint-disable-next-line no-console
+            console.error(msg);
+          }}
+        />


This is another example of a subtle set of details I'm keen on not duplicating 🙂 .

Most of this should get factored out to go along with getOnShouldStartLoadWithRequest -- the originWhitelist, the transparent-background style, the onShouldStartLoadWithRequest. The bits that the factored-out code (perhaps a new React component?) should take as parameters (or React props) are something like: html, the onMessage and onError callbacks, and maybe baseUrl.

agrawal-d

Thanks for the detailed review @gnprice! 😄

I've made the changes you suggested.

agrawal-d · 2020-07-08T07:48:04Z

src/webview/html/editHistoryHtml.js

+  return template`
+    <div class="edit-history-block">
+      <div class="static-timestamp">${editedTime}</div>
+      <span class="edit-history-content">$!${displayedContent}</span>
+      <div class="message-tags">$!${tag}</div>


Thanks for the detailed explaination!

agrawal-d · 2020-07-08T07:51:43Z

src/webview/static/base.css

+.edit-history-block{
+  padding:8px;
+  border-bottom:1px solid hsla(0, 0%, 50%, 0.25);
+}
+.edit-history-block .static-timestamp{
+  margin-top: 8px;
+  text-align: right;
+}
+.edit-history-content{
+  -webkit-user-select: text; /* Safari 3.1+ */
+  -moz-user-select: text; /* Firefox 2+ */
+  -ms-user-select: text; /* IE 10+ */
+  user-select: text; /* Standard syntax */
+  -khtml-user-select: text;
+  -webkit-touch-callout: text;
+  word-wrap: break-word;
+}
+.highlight_text_inserted {
+  color: #9effa1;
+  background-color: hsla(120,64%,95%,.3);
+}
+.highlight_text_deleted {
+  color: red;
+  background-color: #fcdcd8;
+  text-decoration: line-through;
+  word-break: break-all;
+}


The .highlight_text_* CSS is borrowed form the web-app, the others are new.

agrawal-d · 2020-07-08T11:53:18Z

I've somehow requested a review from you twice 😛

chrisbobbe · 2020-07-15T17:42:50Z

src/webview/MessageList.js

 class MessageList extends Component<Props> {
  static contextType = ThemeContext;
  context: ThemeColors;

-  webview: ?WebView;
+  webview = null;


Hmm, what was the reason to lose the type annotation here? Is it possible to bring it back? If it can't be brought back simply as-is, maybe we could try with the newer React.createRef API? (My example at #4101 (comment) might be helpful.)

The innerRef is actually a ref to react-native-webview, so to annotate the type, I need to import both ZulipWebView and WebView in MessageList.js. Done this now.

src/common/WebView.js

chrisbobbe · 2020-07-15T18:30:15Z

src/common/WebView.js

@@ -0,0 +1,165 @@
+/* @flow strict-local */


f1a7aca webview: Refactor core logic into a common component.

Cool! It's great to see this core logic centralized so we don't have to duplicate it.

With the choice to do it in a new React component, one thing to be aware of (even if nothing actually changes, and it turns out to be fine) is that it adds a layer of complexity if we want to change the behavior (or debug something) around React components' lifecycles, particularly updates, a.k.a. re-renders. I mentioned this briefly here but didn't go into very much detail.

The WebView component from react-native-webview has its own ideas about when it should update; it can choose to be a stateless "function component", a PureComponent, or a regular Component, and, if the latter, it can give hints about its desired behavior in shouldComponentUpdate. We should assume its updates may be triggered by updates in its parent component.

There's now another layer between that WebView and our MessageList (and between that WebView and the new EditHistory, but I think it's much more important to think about MessageList).

Our MessageList component implements shouldComponentUpdate to say that we just never want the MessageList component to update. (React would probably say this is controversial; they recommend, "Do not rely on it to “prevent” a rendering, as this can lead to bugs", in this doc. But so far we haven't run into problems with this.)

I haven't tested, but I think everything should actually turn out fine. I would be concerned if our new WebView component did some additional updates of its own, not prompted by an update in its parent. The only case I can think of where this would likely happen is if our new WebView component had some state that it decided, on its own, to change. (From the shouldComponentUpdate doc: "Note that returning false does not prevent child components from re-rendering when their state changes.")

One (optional) simple thing that could be done about this case, for a bit of peace of mind, I think, is to make our new WebView a stateless "function component" (doc) and add a note that we shouldn't introduce state without considering the extra complexity it would add around re-rendering. I very rarely use "function component"s but I think it would be appropriate in this case. If we're going this route, maybe let's not use an arrow function, because, for the sake of logging output, we don't want it to be anonymous. A regular function declaration would be okay, as in the example in the doc (function Welcome(props) {). But maybe better (so we don't have to think about hoisting) is to use const with a named function expression, like const ZulipWebView = function ZulipWebView(props) { (here's a quick example).

I also think, just in case we do eventually have to do some hairy debugging around components' lifecycles, or just to be clear about what's what, in general, it might be nice to name our new WebView something like ZulipWebView. I think of these as reasons for the naming of ZulipButton (but actually, looking closer, it doesn't actually wrap a component called Button, hmm).

chrisbobbe · 2020-07-15T18:56:27Z

Thanks, @agrawal-d! I think #4183 may be about to land (I'll go resolve some conflicts), which will change the logic to get the baseUrl.

agrawal-d · 2020-07-16T11:44:21Z

Thanks for the review @chrisbobbe, I've made the changes you suggested. Now I guess I'll wait for #4183 to land.

chrisbobbe · 2020-07-24T23:41:15Z

Now I guess I'll wait for #4183 to land.

Just landed, in the series ending in ae7ad99! 🙂

agrawal-d · 2020-07-27T16:30:51Z

@chrisbobbe I've rebased this PR. It was a bit convoluted, so if I missed something from your commits, please let me know.

CC @gnprice

While the docs for the API response of get-message-history[1] are still unclear, by experimenting it's possible to determine which fields are always present. So, improve the type definition based on these findings. [1] https://zulipchat.com/api/get-message-history

Our usage of 'react-native-webview' is configured intricately to migigate bugs and provide basic scaffolding. So, to make it reusable, refactor it into a common component -ZulipWebView, so that it can be reused later. Sets up Ref Forwarding [1] to ensure that web-view events still work. [1]: https://reactjs.org/docs/forwarding-refs.html

Refactors the default export so that it can be used in different contexts, and now also accepts an `htmlBody` parameter, which should contain the content to be rendered. This file will later be used to render edit message history.

Creates `editHistoryHtml.js` and adds styles to `base.css` to generate markup that can be used to show message edit history, in a webview. The CSS for the diff highlight was borrowed from the webapp, but the CSS for rendering the edit history blocks is new.

Creates a component 'EditHistory' that renders a webview to show the edit history of a message. This webview is non-interactive - the user can select/copy message contents, but can not interact with them - like open links/navigate to narrows etc. Fixes: zulip#4134

chrisbobbe · 2020-12-09T01:07:08Z

I just rebased and resolved some conflicts. I also made a small change (getting the theme name for EditHistory using react-redux instead of as a navigation param), but I haven't yet done a more thorough review. I hope to do so soon! 🙂

agrawal-d requested review from gnprice and chrisbobbe June 29, 2020 08:26

agrawal-d force-pushed the edit-history branch 2 times, most recently from 685401d to 89b89f4 Compare June 29, 2020 08:41

chrisbobbe reviewed Jul 1, 2020

View reviewed changes

chrisbobbe reviewed Jul 3, 2020

View reviewed changes

src/webview/html/editHistoryHtml.js Outdated Show resolved Hide resolved

agrawal-d force-pushed the edit-history branch from 89b89f4 to 04056e0 Compare July 4, 2020 08:09

agrawal-d requested a review from chrisbobbe July 4, 2020 08:12

gnprice reviewed Jul 7, 2020

View reviewed changes

agrawal-d commented Jul 8, 2020

View reviewed changes

agrawal-d force-pushed the edit-history branch from 04056e0 to 4d72839 Compare July 8, 2020 11:40

agrawal-d requested review from gnprice July 8, 2020 11:41

agrawal-d force-pushed the edit-history branch from 4d72839 to 0867041 Compare July 8, 2020 12:36

chrisbobbe reviewed Jul 15, 2020

View reviewed changes

src/common/WebView.js Outdated Show resolved Hide resolved

chrisbobbe reviewed Jul 15, 2020

View reviewed changes

agrawal-d force-pushed the edit-history branch from 0867041 to 17fefd4 Compare July 16, 2020 11:43

agrawal-d requested a review from chrisbobbe July 16, 2020 11:43

agrawal-d force-pushed the edit-history branch from 17fefd4 to c3b2679 Compare July 27, 2020 16:29

agrawal-d force-pushed the edit-history branch 2 times, most recently from f6a3e5b to b922e90 Compare July 27, 2020 16:36

agrawal-d added 5 commits December 8, 2020 19:34

webview: Refactor 'html.js' to reuse base HTML.

fd9c98a

Refactors the default export so that it can be used in different contexts, and now also accepts an `htmlBody` parameter, which should contain the content to be rendered. This file will later be used to render edit message history.

chrisbobbe force-pushed the edit-history branch from b922e90 to 2e45013 Compare December 9, 2020 01:05

chrisbobbe added the webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity. label Jul 23, 2021

chrisbobbe mentioned this pull request Feb 24, 2022

Mostly handle topic edits / stream moves #5259

Merged

chrisbobbe mentioned this pull request Oct 19, 2022

msglist [nfc]: Separate out "single-page webview" code from "message list" code #5523

Merged


		/** Docs unclear but suggest absent if only content edited. */
		topic?: string,

		// 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.

Allow viewing message edit history #4173

Are you sure you want to change the base?

Allow viewing message edit history #4173

Conversation

agrawal-d commented Jun 29, 2020

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d commented Jul 1, 2020 • edited Loading

agrawal-d commented Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d commented Jul 4, 2020

chrisbobbe commented Jul 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d commented Jul 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Jul 15, 2020

agrawal-d commented Jul 16, 2020

chrisbobbe commented Jul 24, 2020

agrawal-d commented Jul 27, 2020 • edited Loading

chrisbobbe commented Dec 9, 2020

agrawal-d commented Jul 1, 2020 •

edited

Loading

agrawal-d commented Jul 1, 2020 •

edited

Loading

agrawal-d left a comment •

edited

Loading

chrisbobbe Jul 15, 2020 •

edited

Loading

chrisbobbe Jul 15, 2020 •

edited

Loading

agrawal-d commented Jul 27, 2020 •

edited

Loading