clean up the deep link code #17452

ntdiary · 2023-04-14T08:26:38Z

Details

This is a followup PR for #16043.
After we remove the redirection ui in the deep link component, We no longer need to update the appInstallationCheckStatus variable.
We can also remove the call to Network.post().
We need to implement the transition process: oldDot --> newDot --> Desktop app

Fixed Issues

$ #17391

Tests

Note: If we can't start OldDot locally, we can open the developer tools and add a breakpoint in the source panel to get the transition path, just like the image above.

Open the local newDot and login with account A (tab 1).
Open the local oldDot and login with the same account (tab 2).
Open the local Desktop App and stay logged out.
Click on the concierge button at bottom right on oldDot/inbox (tab 2).
(Optional) Execute console.dir(t.replace('https://staging.new.expensify.com', '')) in console to get the /transition... path part, create a new tab and type localhost:8080 and append the transition path part and then open this url (tab 3).
Tap on the 'Open Electron.app' button in the tab 3.
Verify that account A is also logged in the desktop app.

Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Open staging.new.expensify.com and login with account A (tab 1).
Open staging.expensify.com and login with the same account (tab 2).
Open the staging Desktop App and stay logged out.
Click on the concierge button at bottom right on staging.expensify.com/inbox (tab 2).
Tap on the 'Open New Expensify' button in the tab 3.
Verify that account A is also logged in the desktop app.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

test.mov

Mobile Web - Chrome

N/A

Mobile Web - Safari

N/A

Desktop

N/A

iOS

N/A

Android

N/A

MelvinBot · 2023-04-14T09:25:14Z

@arosiclair Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

tgolen

This looks 💯 times better! Thank you

tgolen · 2023-04-14T13:56:58Z

src/components/DeeplinkWrapper/index.website.js

+        if (prevProps.session.shortLivedAuthToken || !this.props.session.shortLivedAuthToken) {
+            return;
+        }
+        this.openRouteInDesktopApp();


Could you also please add a code comment here explaining something like "Now that there is a shortLivedAuthToken, the route to the desktop app can be opened"?

tgolen · 2023-04-14T13:58:26Z

src/components/DeeplinkWrapper/index.website.js

@@ -101,7 +95,6 @@ class DeeplinkWrapper extends PureComponent {
                if (!iframe.parentNode) {
                    return;
                }
-
                iframe.parentNode.removeChild(iframe);
            }, 100);


Were you ever able to figure out with this setTimeout is necessary? I could see it maybe needing the code to run on the next loop (ie. setTimeout(0)), but 100 seems arbitrary.

I didn't pay attention to this setTimeout before. I refresh page dozens of times, and then print its timestamp difference, even if the actual time difference is only a few milliseconds, setTimeout(0) seems to work well.

What's interesting is that it doesn't seem to just make the code run on the next loop, I also tested with setImmediate/MessageChannel and it made a difference of a few hundred milliseconds, but the popup did not appear.

tgolen · 2023-04-14T13:59:09Z

src/libs/actions/Session/index.js

-        }
-        Navigation.navigate();
-    });
+    API.read('SignInWithShortLivedAuthToken', {authToken, oldPartnerUserID, shouldRetry: false}, {optimisticData, successData, failureData});


Should this be a write()? I think it was originally a write?

If we use write, and if the token has expired, the request will always be in the networkRequestQueue, and the user can't perform other operations in the future. Is there any good way we can solve this problem?

Scenario: click to open the desktop app one minute after the pop-up appears.

tgolen · 2023-04-14T14:00:23Z

src/libs/actions/Session/index.js

@@ -642,6 +621,23 @@ function authenticatePusher(socketID, channelName, callback) {
    });
 }

+function getShortLivedAuthToken() {
+    // TODO: If we have any way to read the shortLivedAuthToken and save it, we can also no longer call makeRequestWithSideEffects.


I'm not sure what "and save it" implies. We are saving it in Onyx, but that isn't enough? Also, TODOs are not allowed in our code. Instead, you should open a GH issue and point to this line of code and then detail what needs to be done.

tgolen · 2023-04-14T14:01:05Z

src/libs/actions/Session/index.js

+        'OpenOldDotLink', {shouldRetry: false}, {},
+    ).then((response) => {
+        const {shortLivedAuthToken} = response;
+        Onyx.merge(ONYXKEYS.SESSION, {shortLivedAuthToken});


Shouldn't the OpenOldDotLink command be sending Onyx instructions back in the response? That's the way all of our other API commands work.

original context: https://github.com/Expensify/Expensify/issues/218742

Yeah it looks like this should just be an API.read now. We're just listening for the short lived token to change in Onyx instead of blocking on the request.

tgolen · 2023-04-14T14:01:44Z

src/components/DeeplinkWrapper/index.website.js

+            return;
+        }
+        this.openRouteInDesktopApp();
+        Session.removeShortLivedAuthToken();


Why is it being removed here? (please also add the answer as a code comment)

tgolen · 2023-04-14T14:02:24Z

src/pages/LogInWithShortLivedAuthTokenPage.js

@@ -62,7 +74,12 @@ class LogInWithShortLivedAuthTokenPage extends Component {
    }

    render() {
-        if (this.props.isTokenValid) {
+        // TODO: Not sure if we should redirect the user to the login page or just show the expiration screen or offline message


Same comment as above about TODOs (unless you are meaning to clean up this TODO in this PR)

Yeah, these two todos will be handled in this PR, I think if if there is a sudden disconnection, we can redirect the user to the login page, do you think that's ok?

marcaaron

@ntdiary I am working on a clean up for this in another PR... it would have been nice to have a heads up that you are going to tackle improving this code so that we are not duplicating any effort and more generally agree on a plan for how to do it.

marcaaron · 2023-04-14T18:22:58Z

src/components/DeeplinkWrapper/index.website.js

@@ -96,7 +101,7 @@ class DeeplinkWrapper extends PureComponent {
                    return;
                }
                iframe.parentNode.removeChild(iframe);
-            }, 100);
+            }, 0);


Why do we actually need this setTimeout()? The explanation above is not very satisfying...

we need to give this iframe some time for it to do what it needs to do

What does it mean?

I don't know the original purpose either, but according to my tests, without this delay, the pop-up window can't appear.

marcaaron · 2023-04-14T18:23:13Z

src/components/DeeplinkWrapper/index.website.js

@@ -48,6 +48,10 @@ class DeeplinkWrapper extends PureComponent {
        if (!this.isMacOSWeb() || CONFIG.ENVIRONMENT === CONST.ENVIRONMENT.DEV) {
            return;
        }
+
+        // We need to clear the old short-lived auth token if it exists.


Why do we need to do this?

We need to ensure that the short-lived auth token doesn't exist, then get the new short-lived auto token from the server, and then the pop-up window can be opened.

Please update the code comment to explain the "why" and not just "what" the code is doing. Why do we need to clear out the previous one before fetching a new one?

@tgolen , sorry, my english is not good. I'm not sure how to describe it. Since we open the popup window based on whether there is an short-lived auth token. If we don't clear it first, the popup won't open?

I think that's a perfect explanation 👍 Here is how I would write it:

Since there is no way to know if a previous short-lived authToken is still valid, any previous short-lived authToken must be cleared out and a new one must be fetched so that the popup window will only open when we know the short-lived authToken is valid.

marcaaron · 2023-04-14T18:24:33Z

src/pages/LogInWithShortLivedAuthTokenPage.js

@@ -74,7 +74,7 @@ class LogInWithShortLivedAuthTokenPage extends Component {
    }

    render() {
-        // TODO: Not sure if we should redirect the user to the login page or just show the expiration screen or offline message
+        // redirect the user to the login page if there is a sudden disconnection
        if (this.props.network.isOffline) {
            Navigation.navigate();


Should we be passing something more explicit here? Calling navigate() with no arguments seems strange to me.

marcaaron · 2023-04-14T18:25:45Z

src/pages/LogInWithShortLivedAuthTokenPage.js

@@ -74,7 +74,7 @@ class LogInWithShortLivedAuthTokenPage extends Component {
    }

    render() {
-        // TODO: Not sure if we should redirect the user to the login page or just show the expiration screen or offline message
+        // redirect the user to the login page if there is a sudden disconnection


I'm unsure what the exact edge case we're trying to handle is based on this comment. The user hits this page and in between downloading the JS bundle and executing it they somehow become offline? Can you expand on what exactly we are preventing and why we are doing this?

Eh, does the desktop app also need to download the js bundle?
After the user clicks Allow button in the popup window to open the desktop app, if the network is disconnected at this time, I'm trying to redirect the user to the login page, prevent them being stuck on the transition page.

Without this condition, the session expired ui will be displayed, if we think this page is also ok, then we can remove this condition.

I'm sorry I don't think I fully understood the previous comments. I don't think the desktop app needs to download the js bundle because it's an electron app and should be bundled with the app (IIRC).

if the network is disconnected at this time

If they are disconnected then how can they access the page at all 🤔

Without this condition, the session expired ui will be displayed, if we think this page is also ok, then we can remove this condition.

I think it's fine but the message about the session being expired seems wrong?

I thought that event if the user is disconnected the desktop app would still display the page, only the request SignInWithShortLivedAuthToken would fail.
Or maybe we can just remove the condition as this seems like a very edge case?

I think it's fine but the message about the session being expired seems wrong?

Or we can display an appropriate message for this case.

marcaaron · 2023-04-14T18:26:57Z

src/components/DeeplinkWrapper/index.website.js

@@ -59,8 +63,9 @@ class DeeplinkWrapper extends PureComponent {
        if (prevProps.session.shortLivedAuthToken || !this.props.session.shortLivedAuthToken) {


Are we not handling the errors we were handling before anymore?

Yeah, the redirection UI has been removed in PR #16383, there would be no point in updating the appInstallationCheckStatus variable in the error handler.

Can you remind me what errors we were handling and why they are not necessary anymore? I looked at the linked PR but I'm not seeing anything about errors or error handling (maybe I missed it).

Yeah, the redirection UI has been removed in PR #16383, there would be no point in updating the appInstallationCheckStatus variable in the error handler.

Eh, Error handling was newly added by me in PR #16043. At that time, I just thought that due to the complexity of the network transmission, we could not guarantee that a request would always succeed. And If the request failed (e.g. due to timeouts), we needed to update appInstallationCheckStatus from CHECKING to NOT_INSTALLED to allow sub components to render properly.
In fact, after #16383, we will always render the sub components, so updating appInstallationCheckStatus no longer makes sense, which also means that error handling is no longer needed.

marcaaron · 2023-04-14T18:33:06Z

src/pages/LogInWithShortLivedAuthTokenPage.js

@@ -107,6 +124,7 @@ LogInWithShortLivedAuthTokenPage.defaultProps = defaultProps;
 export default compose(
    withLocalize,
    withOnyx({
-        isTokenValid: {key: ONYXKEYS.IS_TOKEN_VALID},


Just curious, but what was it doing before and why are we removing it now?

When the user clicks Allow to open the desktop app one minute after the pop-up window appears, the token will become invalid, and we will prompt the user that the session has expired.
Based on this #16043 (comment) , we can just use ONYXKEYS.ACCOUNT.isLoading

ntdiary · 2023-04-19T09:57:10Z

Hi, I think the logic of deep link redirection is relatively clear now, could we merge this PR first?
As for adding the BeginDeepLinkRedirect new command, we will do it in PR #17364?

If there are any other questions or anything I can do, please feel free to let me know.

cc @tgolen, @marcaaron

tgolen · 2023-04-19T13:05:36Z

Yeah, I'd like to get this merged. Can you please complete the PR description and checklist? It'd also be nice to have a C+ review/test this.

ntdiary · 2023-04-19T14:51:25Z

@tgolen of course, please feel free to let me know if there are any mistakes. : )

tgolen · 2023-04-20T14:41:39Z

bump @arosiclair for review and checklist #17452 (comment)

tgolen · 2023-04-20T14:56:50Z

@arosiclair sorry about the bump, I'm still confused on if we are wanting to merge this or not. I'm going to place a HOLD on it until we decide.

marcaaron · 2023-04-25T19:38:06Z

What's the status of this PR? I'm thinking maybe we can create an issue for this to go over the general the plan then treat it like a normal "Bug" issue so that it doesn't fall off our radar? Does that sound OK to you @tgolen ?

tgolen · 2023-04-25T19:41:59Z

I thought the plan was to update this PR with all the necessary fixes. There was some discussion about it over here: #17391 (comment)

That's a good reminder that this can be taken off HOLD (but it still needs updated before it can be reviewed).

ntdiary · 2023-07-06T11:50:52Z

@0xmiroslav, the conflict has been resolved. : )

0xmiros · 2023-07-08T16:05:28Z

@ntdiary sorry, conflicts again

ntdiary · 2023-07-09T08:46:49Z

@0xmiroslav , fixed. : )

pecanoro · 2023-07-11T21:22:32Z

@0xmiroslav Friendly bump to review the last changes asap!

0xmiros · 2023-07-12T11:47:22Z

will do in a few hrs

0xmiros · 2023-07-13T18:15:51Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web: logged in, desktop: logged out

web-desktop.mov

web logged out, desktop: logged out

web-logged.out.mov

web: logged in, desktop: other user logged in

desktop.other.account.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

pecanoro · 2023-07-14T16:35:26Z

src/libs/Browser/index.web.js

+function openRouteInDesktopApp(shortLivedAuthToken = '', email = '') {
+    const params = new URLSearchParams();
+    params.set('exitTo', `${window.location.pathname}${window.location.search}${window.location.hash}`);
+    if (email && shortLivedAuthToken) {


So this means that either both of them are passed or none of them are?

Yeah, These two are the required parameters for signing in. If these two parameters are not present, such as when the login page is opened in a browser, the use can also open the login page in the desktop app via deep-link. This is consistent with our production environment. : )

pecanoro · 2023-07-14T16:37:34Z

@marcaaron @arosiclair In general, it looks good to me but I am not great with the transitioning logic so it would be great if you guys can give it a review as well!

arosiclair

LGTM as well. All yours @marcaaron

pecanoro · 2023-07-18T00:47:11Z

Merge conflicts!

marcaaron · 2023-07-18T02:11:57Z

Sorry, I gotta pass on the review for this one as I am scrambling to make up other work and really don't want this to be blocked on my review. I tried to unassign myself a while back, but looks like it's not possible after you've left a comment. 🙃 Anyways, I trust you guys got this.

OSBotify · 2023-07-18T16:13:29Z

🚀 Deployed to staging by https://github.com/arosiclair in version: 1.3.42-15 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

jasperhuangg · 2023-07-18T22:06:47Z

Tests well! Gonna check this off :)

OSBotify · 2023-07-19T17:58:44Z

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.42-26 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-07-21T18:53:30Z

🚀 Deployed to staging by https://github.com/arosiclair in version: 1.3.44-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-07-25T01:27:27Z

🚀 Deployed to production by https://github.com/marcaaron in version: 1.3.44-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

clean up the deep link code

db996cb

ntdiary mentioned this pull request Apr 14, 2023

Pass user session information when opening the electron app from web platform #16043

Merged

55 tasks

ntdiary marked this pull request as ready for review April 14, 2023 09:24

ntdiary requested a review from a team as a code owner April 14, 2023 09:24

melvin-bot bot requested review from arosiclair and removed request for a team April 14, 2023 09:25

ntdiary mentioned this pull request Apr 14, 2023

[HOLD] [WIP] Remove usage of Network.post() #17364

Closed

56 tasks

tgolen requested changes Apr 14, 2023

View reviewed changes

Add some comments to explain the deep link code

cc48aaf

marcaaron requested changes Apr 14, 2023

View reviewed changes

marcaaron reviewed Apr 14, 2023

View reviewed changes

ntdiary added 4 commits April 17, 2023 21:43

Update some comments to explain the deep link code

728cc18

Update comment for removing the short-lived auth token

f1f19a7

specify the route to redirect

4945eb1

Update comment for removing the short-lived auth token

6dc06df

makeRequestWithSideEffects can be removed in the PR 17364

cff9643

tgolen previously approved these changes Apr 19, 2023

View reviewed changes

pecanoro mentioned this pull request Apr 19, 2023

[HOLD for payment 2023-07-26] [$2000] Desktop - App is show endless spinner when tap popup Open app in "New Expensify #17391

Closed

6 tasks

tgolen changed the title ~~clean up the deep link code~~ [HOLD] clean up the deep link code Apr 20, 2023

rename TRANSITION_FROM_OLD_DOT to TRANSITION

3c73416

ntdiary dismissed tgolen’s stale review via 3c73416 April 25, 2023 15:07

tgolen changed the title ~~[HOLD] clean up the deep link code~~ clean up the deep link code Apr 25, 2023

Merge branch 'main' into clean-up-deep-link-code

b6b672f

ntdiary added 2 commits July 9, 2023 10:05

Merge branch 'main' into clean-up-deep-link-code

1298b0f

keep variable name consistent.

2bb7f31

Merge branch 'main' into clean-up-deep-link-code

61acb1b

Merge branch 'main' into clean-up-deep-link-code

80101fa

0xmiros approved these changes Jul 13, 2023

View reviewed changes

pecanoro reviewed Jul 14, 2023

View reviewed changes

Merge branch 'main' into clean-up-deep-link-code

6d3bdce

arosiclair previously approved these changes Jul 17, 2023

View reviewed changes

Merge branch 'main' into clean-up-deep-link-code

bf37347

ntdiary dismissed arosiclair’s stale review via bf37347 July 18, 2023 02:56

Merge branch 'main' into clean-up-deep-link-code

3120a38

arosiclair approved these changes Jul 18, 2023

View reviewed changes

arosiclair merged commit e776ac7 into Expensify:main Jul 18, 2023

OSBotify mentioned this pull request Jul 18, 2023

Deploy Checklist: New Expensify 2023-07-17 #23033

Closed

OSBotify mentioned this pull request Jul 21, 2023

Deploy Checklist: New Expensify 2023-07-21 #23362

Closed

		@@ -59,8 +63,9 @@ class DeeplinkWrapper extends PureComponent {
		if (prevProps.session.shortLivedAuthToken \|\| !this.props.session.shortLivedAuthToken) {

clean up the deep link code #17452

clean up the deep link code #17452

Conversation

ntdiary commented Apr 14, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Apr 14, 2023

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntdiary Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntdiary Apr 14, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Apr 14, 2023 • 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

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

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

ntdiary commented Apr 19, 2023

tgolen commented Apr 19, 2023

ntdiary commented Apr 19, 2023

tgolen commented Apr 20, 2023

tgolen commented Apr 20, 2023

marcaaron commented Apr 25, 2023

tgolen commented Apr 25, 2023

ntdiary commented Jul 6, 2023

0xmiros commented Jul 8, 2023

ntdiary commented Jul 9, 2023

pecanoro commented Jul 11, 2023

0xmiros commented Jul 12, 2023

0xmiros commented Jul 13, 2023 • edited Loading

Reviewer Checklist

Screenshots/Videos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pecanoro commented Jul 14, 2023

arosiclair left a comment

Choose a reason for hiding this comment

pecanoro commented Jul 18, 2023

marcaaron commented Jul 18, 2023

OSBotify commented Jul 18, 2023

jasperhuangg commented Jul 18, 2023

OSBotify commented Jul 19, 2023

OSBotify commented Jul 21, 2023

OSBotify commented Jul 25, 2023

ntdiary commented Apr 14, 2023 •

edited

Loading

ntdiary Apr 14, 2023 •

edited

Loading

ntdiary Apr 14, 2023 •

edited

Loading

marcaaron Apr 14, 2023 •

edited

Loading

0xmiros commented Jul 13, 2023 •

edited

Loading