Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Improve widget rendering on prop updates #1548

Merged
merged 28 commits into from
Nov 10, 2017
Merged

Improve widget rendering on prop updates #1548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
7662b5a
Unfinished, non-working changes to try and handle URL changes gracefully
rxl881 Oct 27, 2017
0a7273b
Add URL search paramas polyfill
rxl881 Oct 27, 2017
3756ce6
Check URL prop updates and ensure that widgets are refreshed.
rxl881 Oct 27, 2017
648b295
Removed comment
rxl881 Oct 31, 2017
1cb878b
Formatting
rxl881 Oct 31, 2017
355d69b
Add querystring dep. already used by things like url_utils.js.
rxl881 Oct 31, 2017
6012b35
Use querystring lib instead of URLSearchParams
rxl881 Oct 31, 2017
35b3326
Use querystring lib
rxl881 Oct 31, 2017
758df29
Fix onLoad on wrong element.
rxl881 Oct 31, 2017
17c0405
Restructure to pass props from componentWillRecieveProps.
rxl881 Oct 31, 2017
a52bb9d
Pass URL to check.
rxl881 Nov 2, 2017
0e854ee
Fix loading and initialisation spinners.
rxl881 Nov 2, 2017
853ada0
Merge branch 'develop' of https://github.com/matrix-org/matrix-react-…
rxl881 Nov 2, 2017
70c4100
Merge branch 'develop' of https://github.com/matrix-org/matrix-react-…
rxl881 Nov 7, 2017
eb8c150
Fix url params parsing.
rxl881 Nov 7, 2017
d3784b4
Fix URL parameter encoding.
rxl881 Nov 7, 2017
96de72a
Switch to using existing dep "qs" and record in package.json
rxl881 Nov 7, 2017
ca1ffdf
Remove unused dep.
rxl881 Nov 8, 2017
be0a76d
Update variable name and JSdoc for improved clarity.
rxl881 Nov 8, 2017
b2b07d9
Formatting
rxl881 Nov 8, 2017
56581ef
Fix various loadingElement related issues.
rxl881 Nov 8, 2017
8016fb8
Fix broken commit.
rxl881 Nov 8, 2017
f796bc7
Fix addition of scalar token to widget URL.
rxl881 Nov 9, 2017
da8b1ff
Ensure that loading state is reset when showing app panel.
rxl881 Nov 9, 2017
98ac3dd
Explicitly set initialisation state.
rxl881 Nov 10, 2017
bd6b5c4
Improve function name.
rxl881 Nov 10, 2017
d2070a0
Replace 'qs' dep. with 'querystring'
rxl881 Nov 10, 2017
ba8a9f2
Comment length
rxl881 Nov 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
"matrix-js-sdk": "0.8.5",
"optimist": "^0.6.1",
"prop-types": "^15.5.8",
"querystring": "^0.2.0",
"react": "^15.4.0",
"react-addons-css-transition-group": "15.3.2",
"react-dom": "^15.4.0",
Expand Down
144 changes: 101 additions & 43 deletions src/components/views/elements/AppTile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
'use strict';

import url from 'url';
import qs from 'querystring';
import React from 'react';
import MatrixClientPeg from '../../../MatrixClientPeg';
import PlatformPeg from '../../../PlatformPeg';
Expand Down Expand Up @@ -51,42 +52,63 @@ export default React.createClass({
creatorUserId: React.PropTypes.string,
},

getDefaultProps: function() {
getDefaultProps() {
return {
url: "",
};
},

getInitialState: function() {
const widgetPermissionId = [this.props.room.roomId, encodeURIComponent(this.props.url)].join('_');
/**
* Set initial component state when the App wUrl (widget URL) is being updated.
* Component props *must* be passed (rather than relying on this.props).
* @param {Object} newProps The new properties of the component
* @return {Object} Updated component state to be set with setState
*/
_getNewState(newProps) {
const widgetPermissionId = [newProps.room.roomId, encodeURIComponent(newProps.url)].join('_');
const hasPermissionToLoad = localStorage.getItem(widgetPermissionId);
return {
loading: false,
widgetUrl: this.props.url,
initialising: true, // True while we are mangling the widget URL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally, this would be set to true in the function that also sets it to false, then the explanation isn't needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the initialising and loading flags need to be set as initial state (before other functions are called). Normally this would be set in getInitialState. However, as this code is reused to reset state at other points in the lifecycle (on URL change), I've pulled it out in to a separate function that is called from getInitialState. Unless you have major concerns, I'll leave this as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just nicer to read, instead of wondering "oh why did loading get set to false here?... where did it get to true... ah way over here"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that makes sense. However, given the reason for doing it like this (as described in the comment above), do you still think that changes are needed? If so, I'm not understanding the change that you are suggesting (sorry for the confusion).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've discussed this out of band and (hopefully) cleared up the confusion on this one. I have changed it to explicitly set the state to 'initialising: true' in the setScalarToken function.

loading: true, // True while the iframe content is loading
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise for this one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, leaving this for the time being. However, may need a refactor for improved clarity in future.

widgetUrl: newProps.url,
widgetPermissionId: widgetPermissionId,
// Assume that widget has permission to load if we are the user who added it to the room, or if explicitly granted by the user
hasPermissionToLoad: hasPermissionToLoad === 'true' || this.props.userId === this.props.creatorUserId,
// Assume that widget has permission to load if we are the user who
// added it to the room, or if explicitly granted by the user
hasPermissionToLoad: hasPermissionToLoad === 'true' || newProps.userId === newProps.creatorUserId,
error: null,
deleting: false,
};
},

// Returns true if props.url is a scalar URL, typically https://scalar.vector.im/api
isScalarUrl: function() {
getInitialState() {
return this._getNewState(this.props);
},

/**
* Returns true if specified url is a scalar URL, typically https://scalar.vector.im/api
* @param {[type]} url URL to check
* @return {Boolean} True if specified URL is a scalar URL
*/
isScalarUrl(url) {
if (!url) {
console.error('Scalar URL check failed. No URL specified');
return false;
}

let scalarUrls = SdkConfig.get().integrations_widgets_urls;
if (!scalarUrls || scalarUrls.length == 0) {
scalarUrls = [SdkConfig.get().integrations_rest_url];
}

for (let i = 0; i < scalarUrls.length; i++) {
if (this.props.url.startsWith(scalarUrls[i])) {
if (url.startsWith(scalarUrls[i])) {
return true;
}
}
return false;
},

isMixedContent: function() {
isMixedContent() {
const parentContentProtocol = window.location.protocol;
const u = url.parse(this.props.url);
const childContentProtocol = u.protocol;
Expand All @@ -98,43 +120,73 @@ export default React.createClass({
return false;
},

componentWillMount: function() {
if (!this.isScalarUrl()) {
componentWillMount() {
window.addEventListener('message', this._onMessage, false);
this.setScalarToken();
},

/**
* Adds a scalar token to the widget URL, if required
* Component initialisation is only complete when this function has resolved
*/
setScalarToken() {
this.setState({initialising: true});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do this immediately preceding the getScalarToken? Then you don't have to do the initialising: false on line 138.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do still need to always set initialising to false, as it is always set to true in the initialiser. Leaving this as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's true.


if (!this.isScalarUrl(this.props.url)) {
console.warn('Non-scalar widget, not setting scalar token!', url);
this.setState({
error: null,
widgetUrl: this.props.url,
initialising: false,
});
return;
}
// Fetch the token before loading the iframe as we need to mangle the URL
this.setState({
loading: true,
});
this._scalarClient = new ScalarAuthClient();

// Fetch the token before loading the iframe as we need it to mangle the URL
if (!this._scalarClient) {
this._scalarClient = new ScalarAuthClient();
}
this._scalarClient.getScalarToken().done((token) => {
// Append scalar_token as a query param
// Append scalar_token as a query param if not already present
this._scalarClient.scalarToken = token;
const u = url.parse(this.props.url);
if (!u.search) {
u.search = "?scalar_token=" + encodeURIComponent(token);
} else {
u.search += "&scalar_token=" + encodeURIComponent(token);
const params = qs.parse(u.query);
if (!params.scalar_token) {
params.scalar_token = encodeURIComponent(token);
// u.search must be set to undefined, so that u.format() uses query paramerters - https://nodejs.org/docs/latest/api/url.html#url_url_format_url_options
u.search = undefined;
u.query = params;
}

this.setState({
error: null,
widgetUrl: u.format(),
loading: false,
initialising: false,
});
}, (err) => {
console.error("Failed to get scalar_token", err);
this.setState({
error: err.message,
loading: false,
initialising: false,
});
});
window.addEventListener('message', this._onMessage, false);
},

componentWillUnmount() {
window.removeEventListener('message', this._onMessage);
},

componentWillReceiveProps(nextProps) {
if (nextProps.url !== this.props.url) {
this._getNewState(nextProps);
this.setScalarToken();
} else if (nextProps.show && !this.props.show) {
this.setState({
loading: true,
});
}
},

_onMessage(event) {
if (this.props.type !== 'jitsi') {
return;
Expand All @@ -154,11 +206,11 @@ export default React.createClass({
}
},

_canUserModify: function() {
_canUserModify() {
return WidgetUtils.canUserModifyWidgets(this.props.room.roomId);
},

_onEditClick: function(e) {
_onEditClick(e) {
console.log("Edit widget ID ", this.props.id);
const IntegrationsManager = sdk.getComponent("views.settings.IntegrationsManager");
const src = this._scalarClient.getScalarInterfaceUrlForRoom(
Expand All @@ -168,9 +220,10 @@ export default React.createClass({
}, "mx_IntegrationsManager");
},

/* If user has permission to modify widgets, delete the widget, otherwise revoke access for the widget to load in the user's browser
/* If user has permission to modify widgets, delete the widget,
* otherwise revoke access for the widget to load in the user's browser
*/
_onDeleteClick: function() {
_onDeleteClick() {
if (this._canUserModify()) {
// Show delete confirmation dialog
const QuestionDialog = sdk.getComponent("dialogs.QuestionDialog");
Expand Down Expand Up @@ -202,6 +255,10 @@ export default React.createClass({
}
},

_onLoaded() {
this.setState({loading: false});
},

// Widget labels to render, depending upon user permissions
// These strings are translated at the point that they are inserted in to the DOM, in the render method
_deleteWidgetLabel() {
Expand All @@ -224,15 +281,15 @@ export default React.createClass({
this.setState({hasPermissionToLoad: false});
},

formatAppTileName: function() {
formatAppTileName() {
let appTileName = "No name";
if(this.props.name && this.props.name.trim()) {
appTileName = this.props.name.trim();
}
return appTileName;
},

onClickMenuBar: function(ev) {
onClickMenuBar(ev) {
ev.preventDefault();

// Ignore clicks on menu bar children
Expand All @@ -247,7 +304,7 @@ export default React.createClass({
});
},

render: function() {
render() {
let appTileBody;

// Don't render widget if it is in the process of being deleted
Expand All @@ -269,29 +326,30 @@ export default React.createClass({
}

if (this.props.show) {
if (this.state.loading) {
appTileBody = (
<div className='mx_AppTileBody mx_AppLoading'>
<MessageSpinner msg='Loading...' />
</div>
);
const loadingElement = (
<div className='mx_AppTileBody mx_AppLoading'>
<MessageSpinner msg='Loading...' />
</div>
);
if (this.state.initialising) {
appTileBody = loadingElement;
} else if (this.state.hasPermissionToLoad == true) {
if (this.isMixedContent()) {
appTileBody = (
<div className="mx_AppTileBody">
<AppWarning
errorMsg="Error - Mixed content"
/>
<AppWarning errorMsg="Error - Mixed content" />
</div>
);
} else {
appTileBody = (
<div className="mx_AppTileBody">
<div className={this.state.loading ? 'mx_AppTileBody mx_AppLoading' : 'mx_AppTileBody'}>
{ this.state.loading && loadingElement }
<iframe
ref="appFrame"
src={safeWidgetUrl}
allowFullScreen="true"
sandbox={sandboxFlags}
onLoad={this._onLoaded}
></iframe>
</div>
);
Expand Down