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

fix: re-fetch appData on talk hash invalidate #247

Merged
merged 4 commits into from
Jun 27, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Jun 26, 2023

☑️ Resolves

🖼️ Screenshots

No visual changes

🚧 Tasks

AppData includes capabilities and userMetadata.

Before appData was collected on the first login, then persist in localStorage until logout. It makes it invalid on Talk update or Talk settings update.

This PR:

  • Adds talk hash and dirty flag to appData
  • Adds appData.service with functions to re-fetch capabilities and userMetadata
  • Adds more utils methods in AppData
  • Restores appData during the Welcome screen, without dirty executeJavascript in the main process
    • And re-fetch if talk hash is dirty
  • Initializes Talk's talkHashStore with the hash restored with appData
  • Subscribe Talk's setInitialNextcloudTalkHash mutation to save initial hash
  • Subscribe Talk's markNextcloudTalkHashDirty mutation to handle invalide hash and relaunch the app

Possible scenarios:

Scenario 1 - appData invalidates between launches

  1. App restores the appData with the old hash
  2. The old hash is set to the Talk's store
  3. On Talk app init (conversations loading) it receives new hash
  4. Hash is dirty - save it to the appData
  5. App is relaunching
  6. On relaunch during the welcome screen new capabilities and userMetadata are fetched and persisted

Scenario 2 - appData invalidates after the launch

The same

Scenario 3 - old appData before this update

The same. Having now talkHashDirty flag considers dirty.

Drawbacks and alternative solutions

  1. Show a message instead of the hard relaunch
    • On hash invalidate Talk web shows a toast asking to reload and makes a web page barely working
    • It requires patching Talk web to change the toast behavior
    • But in Talk Web, we cannot call, save drafts or join chats on invalidate anyway
    • Drawbacks:
      • Possibility of saving a draft of the current chats (other drafts are lost anyway)
      • Hard relaunch doesn't look good in UX
    • I prefer the current solution on desktop and think, the game is not worth the candle
  2. Always re-fetch new appData on launch
    • Currently, if appData is invalid, it would be known only after launch and fetching conversations
    • It makes the launch fast, but relaunches app on invalidates
    • Alternative - make launch always slower, but without relaunch on invalidate between launches
    • I prefer the current solution because a typical launch doesn't require re-fetch

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme added bug Something isn't working 3. to review labels Jun 26, 2023
@ShGKme ShGKme added this to the v0.7.0 milestone Jun 26, 2023
@ShGKme ShGKme requested review from nickvergessen and Antreesy June 26, 2023 22:23
@ShGKme ShGKme self-assigned this Jun 26, 2023
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Can't read it from the code really, so just commenting here:
In case it is not happening already, we should also refetch the data if the desktop client version changed. So that when people update the client they always get newest data.

src/app/appData.service.js Outdated Show resolved Hide resolved
src/app/appData.service.js Outdated Show resolved Hide resolved
src/authentication/renderer/AuthenticationApp.vue Outdated Show resolved Hide resolved
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 27, 2023

In case it is not happening already, we should also refetch the data if the desktop client version changed. So that when people update the client they always get newest data.

Do you mean, on any update, even if a talk hash was not changed?

ShGKme added 2 commits June 27, 2023 09:48
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme linked an issue Jun 27, 2023 that may be closed by this pull request
@nickvergessen
Copy link
Member

Do you mean, on any update, even if a talk hash was not changed?

yes, if the talk client version changes, refetch the data. Maybe at some point we don't expose a feature to specific client versions or something alike.

@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 27, 2023

yes, if the talk client version changes, refetch the data. Maybe at some point we don't expose a feature to specific client versions or something alike.

Done in the last commit

@ShGKme ShGKme requested a review from nickvergessen June 27, 2023 10:39
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Didn't test, but looks good

Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
Copy link
Contributor Author

ShGKme commented Jun 27, 2023

Small update, also save new desktop client version in the appData on re-fetch

@ShGKme ShGKme merged commit f8f0b7c into main Jun 27, 2023
@delete-merged-branch delete-merged-branch bot deleted the fix/198/refetch-app-data-on-invalidate branch June 27, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refresh AppData and capabilities without re-login
2 participants