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

No access to device logs on staging / local dev env #2620

Closed
Steve-Mcl opened this issue Aug 20, 2023 · 8 comments · Fixed by #2645
Closed

No access to device logs on staging / local dev env #2620

Steve-Mcl opened this issue Aug 20, 2023 · 8 comments · Fixed by #2645
Labels
area:frontend For any issues that require work in the frontend/UI bug Something isn't working priority:high High Priority size:XS - 1 Sizing estimation point
Milestone

Comments

@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented Aug 20, 2023

Current Behavior

Cloud / Prod - OK ✅

image

Staging - NG ❌

image

Local (debugging with Vue Dev Tools)

image

"Device Logs" entry is correctly added to the navigation array - BUT - I suspect the SectionNavigationHeader component doesnt support dynamic tabs.

(The Device Logs entry is added to the navigation array after the page loads / behind a feature flag: https://github.com/flowforge/flowforge/blob/e225b7457d6683bb148f5b5969ac404e3e78b57f/frontend/src/pages/device/index.vue#L147-L152

Expected Behavior

Access to device logs

Steps To Reproduce

  1. Run from source (Main/head as of today) OR visit staging env
  2. Go to the overview of any device e.g. https://<FF_NAME_OR_IP>/device/<DEVICE_ID>/overview

Environment

  • FlowForge version: Current - live on staging as of writing (not yet live on Production)
  • Platform/OS: Windows
  • Browser: Chrome

Have you provided an initial effort estimate for this issue?

I have provided an initial effort estimate

@Steve-Mcl Steve-Mcl added the needs-triage Needs looking at to decide what to do label Aug 20, 2023
@Steve-Mcl Steve-Mcl changed the title No access to device logs No access to device logs on staging / local dev env Aug 20, 2023
@Steve-Mcl Steve-Mcl added bug Something isn't working area:frontend For any issues that require work in the frontend/UI priority:high High Priority and removed needs-triage Needs looking at to decide what to do labels Aug 20, 2023
@Steve-Mcl Steve-Mcl added this to the 1.11 milestone Aug 20, 2023
@Steve-Mcl Steve-Mcl added the size:XS - 1 Sizing estimation point label Aug 20, 2023
@Steve-Mcl
Copy link
Contributor Author

Steve-Mcl commented Aug 20, 2023

UPDATE:

I have found the issue.

Navigation entries need a to property to be set.

Currently, the navigation entry for "Device logs" has a path property. Changing this to to fixes the issue.

ALTERNATIVELY:

Support both to and path in the SectionNavigationHeader component in case there are other issues in the code base lurking?

@joepavitt
Copy link
Contributor

It's now "to" rather than "path" as a property. It should support async setting of tabs, suspect the main issue is the change in key/property.

@Steve-Mcl
Copy link
Contributor Author

Steve-Mcl commented Aug 20, 2023

Yeah, I can confirm the component does work (even dynamically) when the to prop is provided.

Unless we are confident there are no more of these gremlins, we might be want to consider supporting both path and to in the component?

@Steve-Mcl
Copy link
Contributor Author

Also, I noted the component has a featureUnavailable property. Perhaps instead of adding the navigation entry dynamically in JS it should be added in the HTML and utilise that?

@joepavitt
Copy link
Contributor

we might be want to consider supporting both path and to in the component

No harm in it

@joepavitt
Copy link
Contributor

Also, I noted the component has a featureUnavailable property.

Need to remind myself what this does on Tuesday. IIRC it's for advertising premium features, but want to make sure before I give a thumbs up

@Steve-Mcl
Copy link
Contributor Author

@joepavitt

This had to be addressed quickly today during weekly release due to unforeseen circumstances.

The simple fix of changing from to to as first discussed in this comment was applied to get us over the hump.

A quick search of the code for more instances of dynamically adding nav items with the old path property threw up some peculiar findings (i.e. there are lots of navigation.push({..., path: ""} but these seemed to be working). So there is a subtly somewhere that should ideally be fully understood and handled if necessary.

We may wish to evaluate the various dynamic tab/menu/list item "things" and settle on a convention to avoid future issues?

@Steve-Mcl Steve-Mcl linked a pull request Aug 24, 2023 that will close this issue
11 tasks
@knolleary
Copy link
Member

Given the immediate issue is resolved, I'm closing this issue.

If there's a desire for a follow-up task, please raise a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:frontend For any issues that require work in the frontend/UI bug Something isn't working priority:high High Priority size:XS - 1 Sizing estimation point
Projects
Archived in project
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants