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

feat(conversationsStore) - cache conversations to BrowserStorage #10203

Merged
merged 2 commits into from
Aug 18, 2023

Conversation

Antreesy
Copy link
Contributor

☑️ Resolves

  • Allow to store fetched conversations in BrowserStorage, and recover it instantly from cache, while waiting for a server response
  • Conversations updated with patchConversations() action, which is called approx. every 30 seconds
  • If conversation was deleted, it will be removed from the view with the next full fetch, with redirect to /apps/spreed/not-found

🖼️ Screenshots

🏚️ Before 🏡 After
image image

🚧 Tasks

  • Manual testing
  • Code review

🏁 Checklist

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Tested, works fine. Only some naming preferences.

Checked that LS is pruned on logout, not security issues ✅


From the UX side: after reload, a user sees an old conversations list with old last messages. That could be confusing. Should we add any small load indicator?


BrowserStorage is reused by tabs. If we store conversations in the browser storage, we should do it only once.

Currently it is used only for initialization. But if we open 5 tabs with Talk, then we have 5 apps with 5 conversation re-fetchings and 5 parallel updating localStorage. The most optimal solution here could be to run re-fetching only on one tab/worker and listen to the storage windows event or BroadcastChannel.

src/store/conversationsStore.js Outdated Show resolved Hide resolved
src/store/conversationsStore.js Outdated Show resolved Hide resolved
src/store/conversationsStore.js Outdated Show resolved Hide resolved
@ShGKme
Copy link
Contributor

ShGKme commented Aug 15, 2023

Upd: instead of shared worker, it could be easier and better for browser support to use Web Locks API

@ShGKme
Copy link
Contributor

ShGKme commented Aug 16, 2023

Notes about using Web Locks API here.

We don't have actually a "resource" that we want to block. But we can use a Leader pattern.
With Locks API we can lock a tab to be a "Leader" who is responsible for everything that should be done only in one tab. Then let's call "being a leader" a resource that is lock for infinity time (until the tab is not closed).

Then we can create a helper like

async function onTalkLeaderTab(() => {
  // Promise of waiting to be a leader
  return new Promise((resolve) =>
    // Request "being a leader"
    navigator.locks.request('talk:leader', () => {
      // Since we got this "resource" this tab is a leader. Resolve promise
      resolve()
      
      // Infinity promise, resource is blocked forever, while tab is not closed
      return new Promise(() => {})
  }
}

onTalkLeaderTab()
  .then(() => {
    // Now we are the leader
    // Start the setInterval for re-fetching or anything
  })

One important thing: it only works with HTTPS ⚠️

Then we can use the BroadcastChannel to inform other tabs about new conversations (cc nextcloud-libraries/nextcloud-event-bus#600)

@Antreesy Antreesy force-pushed the feat/noid/cache-conversations-to-storage branch from 3263f12 to b12044b Compare August 16, 2023 14:27
src/store/conversationsStore.js Outdated Show resolved Hide resolved
src/store/conversationsStore.js Outdated Show resolved Hide resolved
src/store/conversationsStore.js Outdated Show resolved Hide resolved
src/components/LeftSidebar/LeftSidebar.vue Outdated Show resolved Hide resolved
src/components/LeftSidebar/LeftSidebar.vue Outdated Show resolved Hide resolved
src/components/LeftSidebar/LeftSidebar.spec.js Outdated Show resolved Hide resolved
@Antreesy Antreesy force-pushed the feat/noid/cache-conversations-to-storage branch from b12044b to 2b2cdb2 Compare August 17, 2023 11:06
@Antreesy Antreesy requested a review from ShGKme August 17, 2023 11:07
src/utils/talkLeader.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Tested, everything seems to work fine. Tested with 2-3 tabs, with changing leader, creating new conversations, deleting conversations.

@ShGKme
Copy link
Contributor

ShGKme commented Aug 17, 2023

Follow-up: because x-talk-hash header is checked not in any HTTP response, but only in re-fetching conversations for the LeftSidebar, it is not checked not in sub-tabs.

@Antreesy Antreesy force-pushed the feat/noid/cache-conversations-to-storage branch from d9aab00 to 8a94079 Compare August 17, 2023 18:37
@Antreesy
Copy link
Contributor Author

Follow-up: because x-talk-hash header is checked not in any HTTP response, but only in re-fetching conversations for the LeftSidebar, it is not checked not in sub-tabs.

Rebased, follow-up fixed with additional broadcasted message specifically for talk hash.
Also a logic has been moved to the store and App.vue, to be able to use a 'leader' locked status within different processes in one tab.

image

@Antreesy Antreesy requested a review from ShGKme August 17, 2023 18:37
@Antreesy Antreesy force-pushed the feat/noid/cache-conversations-to-storage branch from 112a2a4 to 358ea29 Compare August 18, 2023 12:52
src/stores/leaderTab.js Outdated Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
src/stores/leaderTab.js Outdated Show resolved Hide resolved
src/utils/talkLeader.js Outdated Show resolved Hide resolved
src/store/talkHashStore.js Outdated Show resolved Hide resolved
@Antreesy Antreesy force-pushed the feat/noid/cache-conversations-to-storage branch from f9f8028 to 7d86c4d Compare August 18, 2023 17:10
…ery 30 seconds only from one source

Signed-off-by: Maksim Sukharev <[email protected]>
@ShGKme ShGKme force-pushed the feat/noid/cache-conversations-to-storage branch from 7d86c4d to 2bd8ce1 Compare August 18, 2023 18:08
@ShGKme
Copy link
Contributor

ShGKme commented Aug 18, 2023

Follow-ups:

  • Don't cache conversations when nothing actually changed (we know it from the patching action)
  • Add methods to post specific messages into the broadcast channel into the talkBroadcastChannel service as well as methods to add message handlers
  • Think about centralized way to control what and when is synced, when to post messages

@Antreesy Antreesy merged commit 0796db6 into master Aug 18, 2023
18 checks passed
@Antreesy Antreesy deleted the feat/noid/cache-conversations-to-storage branch August 18, 2023 19:17
@Antreesy
Copy link
Contributor Author

/backport to stable27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants