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.

Sign up for GitHub

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

Jump to bottom

frontend: overhaul validator dashboard fetching logic #1166

Open
wants to merge 7 commits into
base: staging
Choose a base branch
from
Open

frontend: overhaul validator dashboard fetching logic #1166

wants to merge 7 commits into from

Conversation

LuccaBitfly
Copy link
Contributor

@LuccaBitfly LuccaBitfly commented Nov 26, 2024 •
edited by enzo-bitfly
Loading

In the current validator dashboard fetching flow, components will either fetch their own data by reacting to changes in other unrelated components or manually trigger refreshes in unrelated components, making tracing bugs a hassle.
This PR aims to bring back order by making sure that most data fetching happens in the dashboards root component and that these are only triggered through emits. As a consequence of removing sibling component coupling, most of the dashboards Pinia stores won't be necessary anymore.

Edit 06/02/2025:

Branch updated with following adjustments:

  • Add resetOverviewData function to validatorDashboardStore - needed when there's no dashboard key (guest dashboard with no validators), so overview isn't refetched and the store needs to reflect empty state
  • Prevent expanded summary rewards rows from collapsing on slot-viz refetch - now data won't update on that table when polling slot-viz data, but this is the way it worked before this PR anyway - fixing this is a bigger issue involving changes in BcTable and all it's implementations.
  • Rename isAllSelected to hasNoGroupsOrAllSelected in dashboard root component
  • Organize all logic by topic inside root component

@cloudflare-workers-and-pages Cloudflare Workers and Pages
Copy link

cloudflare-workers-and-pages bot commented Nov 26, 2024 •
edited
Loading

Deploying beaconchain with  Cloudflare Pages  Cloudflare Pages

Latest commit: cea818f
Status: ✅  Deploy successful!
Preview URL: https://a06908e3.beaconchain.pages.dev
Branch Preview URL: https://beds-914-better-dashboard-fe.beaconchain.pages.dev

View logs

@LuccaBitfly LuccaBitfly force-pushed the BEDS-914/better-dashboard-fetching branch from 3cbdf1c to cab1b5d Compare November 27, 2024 08:21
@LuccaBitfly LuccaBitfly changed the title refactor(dashboard): better overview and slotviz data fetching frontend: overhaul validator dashboard fetching logic Nov 27, 2024
@LuccaBitfly LuccaBitfly force-pushed the BEDS-914/better-dashboard-fetching branch 4 times, most recently from 190d26c to ce80077 Compare November 27, 2024 14:45
marcel-bitfly
marcel-bitfly reviewed Nov 28, 2024
View reviewed changes
Copy link
Contributor

@marcel-bitfly marcel-bitfly left a comment

Choose a reason for hiding this comment

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

very nice work so far 💪

frontend/components/dashboard/DashboardValidatorOverview.vue Outdated Show resolved Hide resolved
frontend/composables/useValidatorDashboardOverview.ts Outdated Show resolved Hide resolved
frontend/composables/useValidatorSlotViz.ts Outdated Show resolved Hide resolved
frontend/composables/useValidatorSlotViz.ts Outdated Show resolved Hide resolved
frontend/pages/dashboard/[[id]]/index.vue Outdated Show resolved Hide resolved
frontend/pages/dashboard/[[id]]/index.vue Outdated Show resolved Hide resolved
frontend/pages/dashboard/[[id]]/index.vue Outdated Show resolved Hide resolved
frontend/components/dashboard/DashboardControls.vue Outdated Show resolved Hide resolved
frontend/components/dashboard/GroupManagementModal.vue Outdated Show resolved Hide resolved
frontend/stores/dashboard/useValidatorDashboardStore.ts
'validator_dashboard_store',
() => {
const { t: $t } = useTranslation()
// TODO: bring ID in here and remove the provider composable!!
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ love this -> #promotioncantidate

Copy link
Contributor

Choose a reason for hiding this comment

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

That's where it belongs and we'll add it here when we remove the provider in the near future

@LuccaBitfly LuccaBitfly force-pushed the BEDS-914/better-dashboard-fetching branch 3 times, most recently from 45e540d to 60fe37d Compare November 29, 2024 07:55
@LuccaBitfly LuccaBitfly force-pushed the BEDS-914/better-dashboard-fetching branch 5 times, most recently from e932972 to 0fdeb08 Compare December 17, 2024 08:21
marcel-bitfly
marcel-bitfly reviewed Dec 17, 2024
View reviewed changes
frontend/components/dashboard/DashboardValidatorManagementModal.vue
return addUpValues(overview.value?.validators as unknown as Record<string, number>)
})

const totalValidators = computed(() => validatorCount.value ?? 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe validatorCount should always be 0 -> default from store

Copy link
Contributor

@enzo-bitfly enzo-bitfly Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I normally initialize values from fetched data as null to make clear that the data is not yet in the store and avoid hidden bugs.

@LuccaBitfly LuccaBitfly force-pushed the BEDS-914/better-dashboard-fetching branch 2 times, most recently from 4f66a32 to f5ac9ed Compare December 17, 2024 09:40
marcel-bitfly
marcel-bitfly reviewed Dec 17, 2024
View reviewed changes
frontend/components/dashboard/ValidatorSlotViz.vue Outdated
return []
}
return orderBy(
overview.value.groups.filter(g => !!g.count),
dashboardGroups.value.filter(g => !!g.count),
[ g => g.name.toLowerCase() ],
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this even necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't hurt to keep an alphabetical order, since this will make it easier to find groups if there are too many.

marcel-bitfly
marcel-bitfly reviewed Dec 17, 2024
View reviewed changes
frontend/components/dashboard/table/DashboardTableBlocks.vue Outdated
const groupNameLabel = (groupId?: number) => {
return getGroupLabel($t, groupId, groups.value, 'Σ')
}

const onSort = (sort: DataTableSortEvent) => {
loadData(setQuerySort(sort, lastQuery.value))
emitUpdate(setQuerySort(sort, props.query))
Copy link
Contributor

Choose a reason for hiding this comment

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

plz don't stop half way...find a better name for setQuerySort.... as I do not understand what happens here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

marcel-bitfly
marcel-bitfly reviewed Dec 17, 2024
View reviewed changes
frontend/components/dashboard/table/DashboardTableSummary.vue Outdated
(e: 'update', timeframe: SummaryTimeFrame, query: TableQueryParams): void,
}>()
const emitUpdate = (query: TableQueryParams) => {
emit('update', props.timeFrame, query)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is timeframe not part of the query

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently the summary table is the only component which uses timeframe, in future more components will use it. would suggest doing this when adding it to the other components so that this PR doesn't get more bloated

marcel-bitfly
marcel-bitfly reviewed Dec 17, 2024
View reviewed changes
frontend/components/dashboard/table/DashboardTableSummary.vue Outdated
:options="timeFrames"
option-value="id"
option-label="name"
class="small"
:placeholder="$t('dashboard.group.selection.placeholder')"
@select="($event) => { emit('update', $event, props.query) }"
Copy link
Contributor

Choose a reason for hiding this comment

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

you did tightly couple us here to an implementation detail of prime vue here....🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

this might already work with @update

Copy link
Contributor

Choose a reason for hiding this comment

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

The event name wasn't changed in this MR and it belongs to BcDropdown not to Select from vue prime.
Still this line was changed and now looks like this: @select="value => timeFrame = value"

@LuccaBitfly LuccaBitfly force-pushed the BEDS-914/better-dashboard-fetching branch from f5ac9ed to 9fac002 Compare December 19, 2024 14:35
@LuccaBitfly LuccaBitfly force-pushed the BEDS-914/better-dashboard-fetching branch 3 times, most recently from 6ecb02e to e1231e7 Compare January 2, 2025 08:56
@enzo-bitfly enzo-bitfly force-pushed the BEDS-914/better-dashboard-fetching branch 8 times, most recently from f568333 to 0451ac5 Compare February 7, 2025 08:14
@LuccaBitfly @enzo-bitfly
refactor: overhaul overview and slotviz data flow
09e45c4 
- Overview and SlotViz data is now fetched in `index.vue` and propped down
- SlotViz updates are handled through update emits
- Introduces proper SSR fetching
- removes all direct sibling dependencies on the overview (including the dashboard group composable)
- introduces new validator dashboard store
- dashboard modyfing actions now emit an `updateAll` event instead of directly calling `refreshOverview`

See: BEDS-914
@enzo-bitfly enzo-bitfly force-pushed the BEDS-914/better-dashboard-fetching branch from 0451ac5 to 314bbed Compare February 7, 2025 12:03
LuccaBitfly and others added 6 commits February 7, 2025 13:05
@LuccaBitfly @enzo-bitfly
refactor: move dashboard table fetching to index
dddd445 
- Changes all dashboard table logic to emit update events instead of fetching data themselves
- Data is now fetched in the dashboard index component and propped down
- Removes all table stores
- Introduces dashboard composable to hold fetching logic
- Dashboard modifications now trigger only one table request

See: BEDS-914
@LuccaBitfly @enzo-bitfly
refactor: rename query funcs
1b11bb4
@LuccaBitfly @enzo-bitfly
refactor: rename loading bool prop to isLoading
4e352f5
@LuccaBitfly @enzo-bitfly
refactor: spread props in dashboard overview component
145e1e4
@enzo-bitfly
feat: keep slot-viz visible during loading state
8261c21 
- (BcLoadingSpinner) add background color to backdrop
- (DashboardSlotVizDutyVisibilityToggle) prevent hydration mismatch by using `fallback` slot in `ClientOnly` component
- Remove previous guest dashboard key from storage when the dashboard is updated
@enzo-bitfly
refactor: flatten data and paging props in BcTable
cea818f
@enzo-bitfly enzo-bitfly force-pushed the BEDS-914/better-dashboard-fetching branch from 314bbed to cea818f Compare February 7, 2025 12:05
@enzo-bitfly
Copy link
Contributor

@marcel-bitfly see edit in the PR description and go through open threads.
It may be helpful to take a fresh look at the root dashboard component

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcel-bitfly marcel-bitfly marcel-bitfly left review comments

@enzo-bitfly enzo-bitfly enzo-bitfly left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LuccaBitfly @enzo-bitfly @marcel-bitfly