Skip to content

[7839] Data Farm > Applications 4 - remaining vue + loaders - #8052

Open
n-lark wants to merge 18 commits into
mainfrom
7839-df-applications-loaders
Open

[7839] Data Farm > Applications 4 - remaining vue + loaders#8052
n-lark wants to merge 18 commits into
mainfrom
7839-df-applications-loaders

Conversation

@n-lark

@n-lark n-lark commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Description

See test plan: #7839 (comment)

Related Issue(s)

Resolves #7839

  • Single-application views now read the app from the data-farm store instead of local fetch.
  • Removes mixins/Application.js, replaced by a useActiveApplication composable.
  • Detail-page Page Loader is now a shell skeleton ApplicationDetailSkeleton.

The loading work for the ux store will be done in a follow up - Applications 5 - UXStore Loader Registry #8053

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production
  • Link to Changelog Entry PR, or note why one is not needed.

Labels

  • Includes a DB migration? -> add the area:migration label

@n-lark
n-lark requested review from andypalmi, cstns and hardillb August 3, 2026 18:07
@n-lark n-lark self-assigned this Aug 3, 2026
@n-lark
n-lark marked this pull request as draft August 3, 2026 18:16
@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.18%. Comparing base (ab721cb) to head (2785d4e).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8052   +/-   ##
=======================================
  Coverage   76.18%   76.18%           
=======================================
  Files         440      440           
  Lines       23607    23607           
  Branches     6288     6288           
=======================================
  Hits        17984    17984           
  Misses       5623     5623           
Flag Coverage Δ
backend 76.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@n-lark
n-lark requested review from andypalmi and cstns and removed request for cstns August 3, 2026 21:56
@n-lark
n-lark marked this pull request as ready for review August 3, 2026 21:56
@n-lark
n-lark requested a review from hardillb August 3, 2026 21:58
@n-lark n-lark linked an issue Aug 4, 2026 that may be closed by this pull request
2 tasks
@andypalmi

Copy link
Copy Markdown
Contributor

Nice migration, the context.application getter deriving from the store is much cleaner than the old mixin watch.

One thing on createInstance.vue: the breadcrumb at lines 7-8 reads application.id and application.name directly, but application now comes from the store's activeApplication, which is null until loadActiveApplication resolves in created (the old mixin defaulted it to {}). That breadcrumb sits in the always-rendered header and is only guarded by v-if="team", and team is usually already present from session storage, so could it throw during the first render before the app loads? Line 28 in the same file already uses application?.id, so mirroring that (application?.id / application?.name, or v-if="team && application") would probably cover it. index.vue and CreateInstanceMultiStep.vue already look guarded.

Would it be worth a small render test for that page with an unloaded store, just so this path is covered? The store additions themselves have good coverage.

@andypalmi

Copy link
Copy Markdown
Contributor

Two small non-blocking things for possible follow-up:

Stale app data in the list cache. context.setApplication now delegates to setActiveApplication, which merges its argument into applicationsById. That argument now includes the app object nested in an instance or device (setInstance/setDevice pass instance.application / device.application). If that nested object is partial or stale relative to the list, opening an instance would overwrite the list card's name/description with the instance payload's values until the next full refetch. Is instance.application guaranteed to carry the current, complete name and description, or could this surface stale values on the Applications list?

Delete-while-viewing, as a seam with #8030. If a deleted event arrives for the app you're currently on, removeApplication drops it from applicationsById, so activeApplication becomes null and index.vue's !application?.id guard flips the page back to the loading skeleton permanently. Is that the intended outcome, or should a delete-while-viewing redirect to the Applications list (or show a "removed" state) instead of an indefinite skeleton?

Base automatically changed from 7652-df-applications-sub to main August 4, 2026 18:42
@n-lark

n-lark commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Hey @andypalmi ty for the review - I guarded the breadcrumb with v-if="team && application", and added a watcher so a delete redirects to the Applications list with a notice instead of stranding on the skeleton.

Comment thread frontend/src/composables/useActiveApplication.ts Outdated

@andypalmi andypalmi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Both fixes look great. The v-if="team && application" guard resolves the breadcrumb null-deref, and the delete watcher is nicely guarded — previous && !application keeps it off the initial load, and !this.loading.deleting cleanly separates a remote delete from a self-initiated one so there's no double alert or double redirect. The redirect-to-Applications-with-notice is a good seam with #8030's realtime path.

Approving. Two optional follow-ups, neither blocking merge:

  • setActiveApplication's {...existing, ...application} merge can still let a partial instance.application/device.application overwrite a list card's present keys until the next full refetch — worth a look at some point, but fine to defer.
  • The store additions have good coverage; if it's ever convenient, a small render test for createInstance.vue with an unloaded store (and one for the delete watcher) would lock in these two paths.

@cstns

cstns commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

I would have preferred to hold off on the skeleton loaders a bit, because there are some prerequisites that depend on them.

  1. Clicking an application from the applications list flashes an empty state container between the skeleton loader and the loaded application.

    • Throttling to GPRS shows this clearly.
    _-2026-08-07_13.34.20.mp4
  2. Switching between tabs triggers the skeleton for the tabs section too, even though the tabs are static and don't change.

    • The SectionNavigationHeader skeleton should be independent of the rest of the page.
    • the rest of the page should be within its own dedicated wrapper as well
    _-2026-08-07_13.34.20.mp4
  3. Switching to the remote instances page:

    • Displays the skeleton loader while loading.
    • Finishes loading and hides the skeleton, but then shows the old loader while the devices are loading.
    • It should keep showing the skeleton until the devices API call completes.
    • We should drop the old loader in favor of the skeleton.
    _-2026-08-07_13.39.41.mp4
  4. Switching to the hosted instances page:

    • Same as the first point, it flashes the empty state container.
    • There's no reason to keep the empty state container while loading; it should only appear when there actually are no instances.
  5. Double loader:

    • Dashboard, device groups, snapshots, and pipelines all have an intermediate stage where they show the old loader while gathering data.
  6. The skeleton loader doesn't match the page structure:

    • This applies to the pipelines, logs, audit log, and settings pages.
    • Not all application pages are a datatable list.
  7. Device group pages:

    • These pages don't match the format of the applications page.
    • They don't occupy the full available height and have a gray underlay.

Fixing all of this would extend the scope of this task, which is why I would have preferred to go with the page loader approach first and tackle these as a follow-up feature focused on skeleton loaders. That would also have given us a chance to identify these niche cases up front and land on a cohesive approach when rolling skeleton loaders out, not just for applications, but across the other entities and pages in the app as well.

Comment thread frontend/src/mixins/Application.js
Comment thread frontend/src/pages/application/index.vue Outdated
Comment thread frontend/src/pages/application/index.vue
this.clearActiveApplication()
},
methods: {
async loadApplicationData () {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't these be in the application data-farm?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

or are they moved in a follow-up? I didn't read the follow-ups yet

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The app load already uses the store loadActiveApplication. The instances will get moved under the hosted instances work #8076

@cstns

cstns commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

I stand corrected, I just noticed that you removed the skeleton loaders in the follow-up

@n-lark

n-lark commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Hey @cstns! For this comment the Page Loader PR is merged in, dropping skeletons entirely, so the skeleton-specific points no longer apply.

The rest is pre-existing behavior in files this PR doesn't touch, handled in the per-entity loader follow-ups:

  1. Double loaders (dashboards, device groups, snapshots, pipelines, remote instances) - each tab loads its own data, the page loader only gates the application.
  2. Hosted instances empty-state flash - instances load after the app hydrates, same as before this PR.
  3. Device group layout (height / gray underlay) - that page is untouched here.

These land as each child entity gets its own store + loader. This PR just swaps the old per-page ff-loading for the shared page loader. If there is anything you specifically want me to address here please lmk!

@n-lark
n-lark requested a review from cstns August 7, 2026 14:21
data () {
return {
applicationInstances: new Map(),
loadingInstanceStatuses: false

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will address this under #8076

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Applications 4 - remaining vue + loaders

4 participants