From 3a78192e7492d0b0e7d72a530a34f8a86d28f1ee Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Tue, 8 Oct 2024 21:55:51 +0530 Subject: [PATCH] fix: Resolve accountId from the route, initialize route-sync before the app is loaded (#10245) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On production on multiple instances it may happen that the UI is rendered in correctly, with a lot of options in the sidebar not available. On further investigation I found out that the feature flag checks were disabling multiple of those, and also we could see many correlated errors that pointed towards missing information. So, there were two problems here 1. The `vuex-router-sync` was not very reliable in some cases 2. In `App.vue` the watch on `currentAccountId` didn't always trigger. ## Fix Tested on Staging Basically tried to reload the page ~50 times with cache enabled, disabled, throttling, navigating different pages. https://www.loom.com/share/1bb27294aa364ac4acfb647780d6385a?sid=87e31330-8cb7-4ded-8616-5e95e2ae3516
#### What I thought was the fix

### My chain of actions Replacing vuex-router-sync at first worked fine, but then I saw it was still failing in some cases, I assumed (I was half-correct tho) that the rendering of the `App.vue` and syncing of the route to the store was not happening in a synchronous pattern. So I decided, let's not rely on the store when the route is directly available in the App context. Following this, I refactored `useAccount` composable to use `useRoute` directly, instead of the store, and then replaced the getter inside `App.vue`. What this did was surface the issue but more consistently 🤯 I saw the watcher, added some console logs, and turns out it was not getting triggered in all those cases. So I added an `immediate` to it. And viola, it works! At the moment, this is deployed to staging and seems to be working correctly. But we still need to verify it for sure, since how this issue was surfaced is still a mystery. All we know is that it shows up when the widget is also loaded alongside the app (if it loads before or after the app, it works fine) ### What about the route in the store? Well I have used the `route` usage there with fallback to the store state. Since Vuex exists in the app context, the route should always be available to it. But after today I have lost all trust in JavaScript and will worship rails until end of my life, so I added that in a `try-catch` block, logged the error to Sentry

## Here's the real fix If you read the explanation I wrote earlier, I thought I fixed the issue, but then the chat list navigation completely broke. So I removed the custom route sync implementation and added the original package back. Turns out the vuex-router-sync earlier was placed after the app was initalized, however for it to work, the vue app context is not required. And it's best to run it before the app is even bootstrapped, so I added it back and placed it correctly. So the following changes fixes this problem 1. Hoisting the `sync` function call to before we call `createApp` this ensures that the stores and route hooks are in place before even the app is created 2. Ensuring the `initializeAccount` is run immediately when watching `currentAccountId` 4. Source `currentAccountId` for critical top of the tree components directly from the route instead of the store --- app/javascript/dashboard/App.vue | 16 ++++++++++------ .../dashboard/components/layout/Sidebar.vue | 4 +++- .../composables/spec/useAccount.spec.js | 13 +++++++------ .../dashboard/composables/useAccount.js | 10 ++++++---- .../routes/dashboard/settings/account/Index.vue | 5 +++-- app/javascript/entrypoints/dashboard.js | 5 +++-- 6 files changed, 32 insertions(+), 21 deletions(-) diff --git a/app/javascript/dashboard/App.vue b/app/javascript/dashboard/App.vue index 977f8565c..c32cb98d2 100644 --- a/app/javascript/dashboard/App.vue +++ b/app/javascript/dashboard/App.vue @@ -13,6 +13,7 @@ import { useStore } from 'dashboard/composables/store'; import WootSnackbarBox from './components/SnackbarContainer.vue'; import { setColorTheme } from './helper/themeHelper'; import { isOnOnboardingView } from 'v3/helpers/RouteHelper'; +import { useAccount } from 'dashboard/composables/useAccount'; import { registerSubscription, verifyServiceWorkerExistence, @@ -35,8 +36,9 @@ export default { setup() { const router = useRouter(); const store = useStore(); + const { accountId } = useAccount(); - return { router, store }; + return { router, store, currentAccountId: accountId }; }, data() { return { @@ -52,7 +54,6 @@ export default { currentUser: 'getCurrentUser', authUIFlags: 'getAuthUIFlags', accountUIFlags: 'accounts/getUIFlags', - currentAccountId: 'getCurrentAccountId', }), hasAccounts() { const { accounts = [] } = this.currentUser || {}; @@ -69,10 +70,13 @@ export default { this.showAddAccountModal = true; } }, - currentAccountId() { - if (this.currentAccountId) { - this.initializeAccount(); - } + currentAccountId: { + immediate: true, + handler() { + if (this.currentAccountId) { + this.initializeAccount(); + } + }, }, }, mounted() { diff --git a/app/javascript/dashboard/components/layout/Sidebar.vue b/app/javascript/dashboard/components/layout/Sidebar.vue index a08b99a2b..81539c1bb 100644 --- a/app/javascript/dashboard/components/layout/Sidebar.vue +++ b/app/javascript/dashboard/components/layout/Sidebar.vue @@ -2,6 +2,7 @@ import { mapGetters } from 'vuex'; import { getSidebarItems } from './config/default-sidebar'; import { useKeyboardEvents } from 'dashboard/composables/useKeyboardEvents'; +import { useAccount } from 'dashboard/composables/useAccount'; import { useRoute, useRouter } from 'vue-router'; import PrimarySidebar from './sidebarComponents/Primary.vue'; @@ -33,6 +34,7 @@ export default { setup(props, { emit }) { const route = useRoute(); const router = useRouter(); + const { accountId } = useAccount(); const toggleKeyShortcutModal = () => { emit('openKeyShortcutModal'); @@ -72,6 +74,7 @@ export default { return { toggleKeyShortcutModal, + accountId, }; }, data() { @@ -82,7 +85,6 @@ export default { computed: { ...mapGetters({ - accountId: 'getCurrentAccountId', currentUser: 'getCurrentUser', globalConfig: 'globalConfig/get', inboxes: 'inboxes/getInboxes', diff --git a/app/javascript/dashboard/composables/spec/useAccount.spec.js b/app/javascript/dashboard/composables/spec/useAccount.spec.js index aaee9825d..5523c6d91 100644 --- a/app/javascript/dashboard/composables/spec/useAccount.spec.js +++ b/app/javascript/dashboard/composables/spec/useAccount.spec.js @@ -1,14 +1,15 @@ -import { ref } from 'vue'; -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { useAccount } from '../useAccount'; -import { useStoreGetters } from 'dashboard/composables/store'; +import { useRoute } from 'vue-router'; -vi.mock('dashboard/composables/store'); +vi.mock('vue-router'); describe('useAccount', () => { beforeEach(() => { - useStoreGetters.mockReturnValue({ - getCurrentAccountId: ref(123), + useRoute.mockReturnValue({ + params: { + accountId: 123, + }, }); }); diff --git a/app/javascript/dashboard/composables/useAccount.js b/app/javascript/dashboard/composables/useAccount.js index 0c3a7b611..0cc0663b4 100644 --- a/app/javascript/dashboard/composables/useAccount.js +++ b/app/javascript/dashboard/composables/useAccount.js @@ -1,18 +1,20 @@ import { computed } from 'vue'; -import { useStoreGetters } from 'dashboard/composables/store'; +import { useRoute } from 'vue-router'; /** * Composable for account-related operations. * @returns {Object} An object containing account-related properties and methods. */ export function useAccount() { - const getters = useStoreGetters(); - /** * Computed property for the current account ID. * @type {import('vue').ComputedRef} */ - const accountId = computed(() => getters.getCurrentAccountId.value); + const route = useRoute(); + + const accountId = computed(() => { + return Number(route.params.accountId); + }); /** * Generates an account-scoped URL. diff --git a/app/javascript/dashboard/routes/dashboard/settings/account/Index.vue b/app/javascript/dashboard/routes/dashboard/settings/account/Index.vue index 7344bc81e..3a604503f 100644 --- a/app/javascript/dashboard/routes/dashboard/settings/account/Index.vue +++ b/app/javascript/dashboard/routes/dashboard/settings/account/Index.vue @@ -5,6 +5,7 @@ import { mapGetters } from 'vuex'; import { useAlert } from 'dashboard/composables'; import { useUISettings } from 'dashboard/composables/useUISettings'; import { useConfig } from 'dashboard/composables/useConfig'; +import { useAccount } from 'dashboard/composables/useAccount'; import { FEATURE_FLAGS } from '../../../../featureFlags'; import semver from 'semver'; import { getLanguageDirection } from 'dashboard/components/widgets/conversation/advancedFilterItems/languages'; @@ -13,9 +14,10 @@ export default { setup() { const { updateUISettings } = useUISettings(); const { enabledLanguages } = useConfig(); + const { accountId } = useAccount(); const v$ = useVuelidate(); - return { updateUISettings, v$, enabledLanguages }; + return { updateUISettings, v$, enabledLanguages, accountId }; }, data() { return { @@ -46,7 +48,6 @@ export default { globalConfig: 'globalConfig/get', getAccount: 'accounts/getAccount', uiFlags: 'accounts/getUIFlags', - accountId: 'getCurrentAccountId', isFeatureEnabledonAccount: 'accounts/isFeatureEnabledonAccount', }), showAutoResolutionConfig() { diff --git a/app/javascript/entrypoints/dashboard.js b/app/javascript/entrypoints/dashboard.js index 7938e20eb..857a62d12 100644 --- a/app/javascript/entrypoints/dashboard.js +++ b/app/javascript/entrypoints/dashboard.js @@ -10,7 +10,6 @@ import Multiselect from 'vue-multiselect'; import { plugin, defaultConfig } from '@formkit/vue'; import WootSwitch from 'components/ui/Switch.vue'; import WootWizard from 'components/ui/Wizard.vue'; -import { sync } from 'vuex-router-sync'; import FloatingVue from 'floating-vue'; import WootUiKit from 'dashboard/components'; import App from 'dashboard/App.vue'; @@ -18,6 +17,7 @@ import i18nMessages from 'dashboard/i18n'; import createAxios from 'dashboard/helper/APIHelper'; import commonHelpers, { isJSONValid } from 'dashboard/helper/commons'; +import { sync } from 'vuex-router-sync'; import router, { initalizeRouter } from 'dashboard/routes'; import store from 'dashboard/store'; import constants from 'dashboard/constants/globals'; @@ -42,6 +42,8 @@ const i18n = createI18n({ messages: i18nMessages, }); +sync(store, router); + const app = createApp(App); app.use(i18n); app.use(store); @@ -97,7 +99,6 @@ app.component('fluent-icon', FluentIcon); app.directive('resize', vResizeObserver); app.directive('on-clickaway', onClickaway); -sync(store, router); // load common helpers into js commonHelpers(); window.WOOT_STORE = store;