Skip to content
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

[Image][Performance] Fast Image with Auth Token Header Caching #12648

Merged
merged 18 commits into from
Nov 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,8 @@ const CONST = {
},

TFA_CODE_LENGTH: 6,

CHAT_ATTACHMENT_TOKEN_KEY: 'X-Chat-Attachment-Token',
};

export default CONST;
7 changes: 3 additions & 4 deletions src/components/AttachmentModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class AttachmentModal extends PureComponent {
* @param {String} sourceURL
*/
downloadAttachment(sourceURL) {
fileDownload(sourceURL, this.props.originalFileName);
fileDownload(this.props.isAuthTokenRequired ? addEncryptedAuthTokenToURL(sourceURL) : sourceURL, this.props.originalFileName);
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason this line is confusing me - how are we NOT adding the encrypted auth token to the URL for native here? I just tested Android and saw that the image request has the encrypted auth token in the correct header, NOT in the URL - but this line makes me think we're putting the encryptedAuthToken in the URL ALWAYS - can someone explain why I'm confused? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is purely for the file download - both web and native get the encrypted auth token added to the URL here if required for the attachment. Attachments could be a PDF or image or some other file so I wanted to make sure that still used the encryptedAuthToken param here so all files download correctly.

As for the actual image request the source URL (without any auth token query param) and the this.props.isAuthTokenRequired gets passed down to the ImageView so that it can correctly add the auth token in the correct way for web (as a query param) and native (as a header token).

I hope that clears it up a bit 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

@Beamanator
We can update fileDownload to also work with headers.
Not sure whether any backend handling would need to be updated

making fileDownload work with headers is not as important as images, because it's used a lot less often
on the other hand the logic would be cleaner and allow automatic cache usage

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure whether any backend handling would need to be updated

As far as I know, we only currently use auth Tokens to get chat attachments, and I'm pretty sure all of that is working - passing the auth token in the header as opposed to the URL. If there's other places we pass encryptedAuthToken in a URL, we will probably have to modify the backend for those cases

making fileDownload work with headers is not as important as images, because it's used a lot less often
on the other hand the logic would be cleaner and allow automatic cache usage

Good call, I thinkkk the benefits are useful enough to try to implement that on the next PR 👍

Copy link
Contributor Author

@thomas-coldwell thomas-coldwell Dec 1, 2022

Choose a reason for hiding this comment

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

Thanks for the review @kidroca @Beamanator , just going through your comments now 🙌 Just to confirm have we decided here to just use the encryptedAuthToken query param for file download then and set this up to use headers in a subsequent PR?


// At ios, if the keyboard is open while opening the attachment, then after downloading
// the attachment keyboard will show up. So, to fix it we need to dismiss the keyboard.
Expand Down Expand Up @@ -229,9 +229,7 @@ class AttachmentModal extends PureComponent {
}

render() {
const sourceURL = this.props.isAuthTokenRequired
? addEncryptedAuthTokenToURL(this.state.sourceURL)
: this.state.sourceURL;
const sourceURL = this.state.sourceURL;

const {fileName, fileExtension} = FileUtils.splitExtensionFromFileName(this.props.originalFileName || lodashGet(this.state, 'file.name', ''));

Expand Down Expand Up @@ -266,6 +264,7 @@ class AttachmentModal extends PureComponent {
<View style={styles.imageModalImageCenterContainer}>
{this.state.sourceURL && (
<AttachmentView
isAuthTokenRequired={this.props.isAuthTokenRequired}
sourceURL={sourceURL}
file={this.state.file}
onToggleKeyboard={this.updateConfirmButtonVisibility}
Expand Down
13 changes: 11 additions & 2 deletions src/components/AttachmentView.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,13 @@ import compose from '../libs/compose';
import Text from './Text';
import Tooltip from './Tooltip';
import themeColors from '../styles/themes/default';
import addEncryptedAuthTokenToURL from '../libs/addEncryptedAuthTokenToURL';

const propTypes = {

/** Do the urls require an authToken? */
isAuthTokenRequired: PropTypes.bool,

/** URL to full-sized attachment */
sourceURL: PropTypes.string.isRequired,

Expand All @@ -35,6 +40,7 @@ const propTypes = {
};

const defaultProps = {
isAuthTokenRequired: false,
file: {
name: '',
},
Expand All @@ -48,9 +54,12 @@ const AttachmentView = (props) => {
// will appear with a sourceURL that is a blob
if (Str.isPDF(props.sourceURL)
|| (props.file && Str.isPDF(props.file.name || props.translate('attachmentView.unknownFilename')))) {
const sourceURL = props.isAuthTokenRequired
? addEncryptedAuthTokenToURL(props.sourceURL)
: props.sourceURL;
return (
<PDFView
sourceURL={props.sourceURL}
sourceURL={sourceURL}
style={styles.imageModalPDF}
onToggleKeyboard={props.onToggleKeyboard}
/>
Expand All @@ -61,7 +70,7 @@ const AttachmentView = (props) => {
// both PDFs and images will appear as images when pasted into the the text field
if (Str.isImage(props.sourceURL) || (props.file && Str.isImage(props.file.name))) {
return (
<ImageView url={props.sourceURL} />
<ImageView url={props.sourceURL} isAuthTokenRequired={props.isAuthTokenRequired} />
);
}

Expand Down
5 changes: 3 additions & 2 deletions src/components/Avatar.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, {PureComponent} from 'react';
import {Image, View} from 'react-native';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
import stylePropTypes from '../styles/stylePropTypes';
Expand All @@ -9,6 +9,7 @@ import CONST from '../CONST';
import * as StyleUtils from '../styles/StyleUtils';
import * as Expensicons from './Icon/Expensicons';
import getAvatarDefaultSource from '../libs/getAvatarDefaultSource';
import FastImage from './FastImage';

const propTypes = {
/** Source for the avatar. Can be a URL or an icon. */
Expand Down Expand Up @@ -72,7 +73,7 @@ class Avatar extends PureComponent {
/>
)
: (
<Image
<FastImage
source={{uri: this.props.source}}
defaultSource={getAvatarDefaultSource(this.props.source)}
style={imageStyle}
Expand Down
61 changes: 61 additions & 0 deletions src/components/FastImage/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import React from 'react';
import {Image} from 'react-native';
import addEncryptedAuthTokenToURL from '../../libs/addEncryptedAuthTokenToURL';

const RESIZE_MODES = {
contain: 'contain',
cover: 'cover',
stretch: 'stretch',
center: 'center',
};

class FastImage extends React.Component {
constructor(props) {
super(props);

this.state = {
imageSource: undefined,
};
}

componentDidMount() {
this.configureImageSource();
}

componentDidUpdate(prevProps) {
if (prevProps.source === this.props.source) {
return;
}
this.configureImageSource();
}

configureImageSource() {
const source = this.props.source;
let imageSource = source;
if (typeof source !== 'number' && source.headers != null) {
imageSource = {uri: addEncryptedAuthTokenToURL(source.uri)};
}
this.setState({imageSource});
if (this.props.onLoad == null) {
return;
}
const uri = typeof imageSource === 'number'
? Image.resolveAssetSource(imageSource).uri
: imageSource.uri;
Image.getSize(uri, (width, height) => {
this.props.onLoad({nativeEvent: {width, height}});
});
}

render() {
// eslint-disable-next-line
const { source, onLoad, ...rest } = this.props;

// eslint-disable-next-line
return <Image {...rest} source={this.state.imageSource} />;
}
}

FastImage.propTypes = Image.propTypes;
FastImage.resizeMode = RESIZE_MODES;
export default FastImage;
9 changes: 9 additions & 0 deletions src/components/FastImage/index.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import RNFastImage from '@pieter-pot/react-native-fast-image';

// eslint-disable-next-line
const FastImage = (props) => <RNFastImage {...props} />;

FastImage.displayName = 'FastImage';
FastImage.propTypes = RNFastImage.propTypes;
FastImage.resizeMode = RNFastImage.resizeMode;
export default FastImage;
49 changes: 38 additions & 11 deletions src/components/ImageView/index.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,33 @@
import React, {PureComponent} from 'react';
import PropTypes from 'prop-types';
import {
View, Image, Pressable,
View, Pressable,
} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import FastImage from '../FastImage';
thomas-coldwell marked this conversation as resolved.
Show resolved Hide resolved
import styles from '../../styles/styles';
import * as StyleUtils from '../../styles/StyleUtils';
import canUseTouchScreen from '../../libs/canUseTouchscreen';
import withWindowDimensions, {windowDimensionsPropTypes} from '../withWindowDimensions';
import FullscreenLoadingIndicator from '../FullscreenLoadingIndicator';
import compose from '../../libs/compose';
import ONYXKEYS from '../../ONYXKEYS';
import chatAttachmentTokenHeaders from '../../libs/chatAttachmentTokenHeaders';

const propTypes = {

/** Do the urls require an authToken? */
isAuthTokenRequired: PropTypes.bool,

/** URL to full-sized image */
url: PropTypes.string.isRequired,
...windowDimensionsPropTypes,
};

const defaultProps = {
isAuthTokenRequired: false,
};

class ImageView extends PureComponent {
constructor(props) {
super(props);
Expand All @@ -23,6 +36,7 @@ class ImageView extends PureComponent {
this.onContainerLayoutChanged = this.onContainerLayoutChanged.bind(this);
this.onContainerPressIn = this.onContainerPressIn.bind(this);
this.onContainerPress = this.onContainerPress.bind(this);
this.imageLoad = this.imageLoad.bind(this);
this.imageLoadingStart = this.imageLoadingStart.bind(this);
this.imageLoadingEnd = this.imageLoadingEnd.bind(this);
this.trackMovement = this.trackMovement.bind(this);
Expand All @@ -46,9 +60,6 @@ class ImageView extends PureComponent {
}

componentDidMount() {
Image.getSize(this.props.url, (width, height) => {
this.setImageRegion(width, height);
});
if (this.canUseTouchScreen) {
return;
}
Expand Down Expand Up @@ -207,6 +218,10 @@ class ImageView extends PureComponent {
this.setState(prevState => ({isDragging: prevState.isMouseDown}));
}

imageLoad({nativeEvent}) {
this.setImageRegion(nativeEvent.width, nativeEvent.height);
}

imageLoadingStart() {
this.setState({isLoading: true});
}
Expand All @@ -216,24 +231,29 @@ class ImageView extends PureComponent {
}

render() {
const headers = this.props.isAuthTokenRequired ? chatAttachmentTokenHeaders() : undefined;
if (this.canUseTouchScreen) {
return (
<View
style={[styles.imageViewContainer, styles.overflowHidden]}
onLayout={this.onContainerLayoutChanged}
>
<Image
source={{uri: this.props.url}}
<FastImage
source={{
uri: this.props.url,
headers,
}}
style={this.state.zoomScale === 0 ? undefined : [
styles.w100,
styles.h100,
]} // Hide image until zoomScale calculated to prevent showing preview with wrong dimensions.

// When Image dimensions are lower than the container boundary(zoomscale <= 1), use `contain` to render the image with natural dimensions.
// Both `center` and `contain` keeps the image centered on both x and y axis.
resizeMode={this.state.zoomScale > 1 ? 'center' : 'contain'}
resizeMode={this.state.zoomScale > 1 ? FastImage.resizeMode.center : FastImage.resizeMode.contain}
onLoadStart={this.imageLoadingStart}
onLoadEnd={this.imageLoadingEnd}
onLoad={this.imageLoad}
/>
{this.state.isLoading && (
<FullscreenLoadingIndicator
Expand Down Expand Up @@ -265,15 +285,19 @@ class ImageView extends PureComponent {
onPressIn={this.onContainerPressIn}
onPress={this.onContainerPress}
>
<Image
source={{uri: this.props.url}}
<FastImage
source={{
uri: this.props.url,
headers,
}}
style={this.state.zoomScale === 0 ? undefined : [
styles.h100,
styles.w100,
]} // Hide image until zoomScale calculated to prevent showing preview with wrong dimensions.
resizeMode="contain"
resizeMode={FastImage.resizeMode.contain}
onLoadStart={this.imageLoadingStart}
onLoadEnd={this.imageLoadingEnd}
onLoad={this.imageLoad}
/>
</Pressable>

Expand All @@ -288,4 +312,7 @@ class ImageView extends PureComponent {
}

ImageView.propTypes = propTypes;
export default withWindowDimensions(ImageView);
ImageView.defaultProps = defaultProps;
export default compose(withWindowDimensions, withOnyx({
session: {key: ONYXKEYS.SESSION},
}))(ImageView);
Loading