Skip to content

Conversation

bilkoua
Copy link

@bilkoua bilkoua commented Feb 23, 2025

pic

@robertobandini
Copy link
Member

Hi @b17k0 and thank you for your contribute!

Actually I see only the unit tests failing.
Additional columns can still be enabled or disabled like the others, right?

Thanks

@bilkoua
Copy link
Author

bilkoua commented Feb 24, 2025

HI, guys! Yes, functionally everything works as expected. However, I rushed a bit with the PR. I still need to fix the unit tests, and I can already see some areas for optimization. Unfortunately, I’ll only have time to work on this over the weekend.

Also, my TypeScript coding skills are far from perfect. There are some convenient features in the proprietary version that are missing in freelens, and this is just an attempt by a non-professional to bring something I’d love to have in freelens.

If someone with more TS experience is interested, we could do this together. I’m sure the result would be much better that way!

@robertobandini
Copy link
Member

Hi @b17k0 thank you!
I already added some reviewers.
@mariomamo do you think you can provide some help/review here?

@robertobandini robertobandini added enhancement New feature or request help wanted Extra attention is needed labels Feb 25, 2025
@mariomamo
Copy link
Collaborator

Yes of course, it already was on my list, I'll take a look tonight

Copy link
Collaborator

@mariomamo mariomamo left a comment

Choose a reason for hiding this comment

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

  1. Your classes contain too many elements. I suggest creating separate files for the non-injectable component (NonInjectablePodMetricsMemoryContent) and the injectable one (InjectablePodMetricsMemoryContent). NonInjectablePodMetricsMemoryContent should only contain the component logic: HTML elements, functions, variables, and so on, while InjectablePodMetricsMemoryContent should only return the component wrapped in getInjectable, as you already did. You can take pod-logs-menu.tsx and pod-logs-menu.injectable.ts as examples.

  2. Some functions are duplicated: calculatePodKubeMetrics and getPodKubeMetrics. Could you refactor them into a separate file and call them where needed? I suggest using a hook that exposes these two functions, something you can use like this:

    const kubeMetrics = useKubeMetrics();
    kubeMetrics.getPodKubeMetrics();
  3. The functions getPodKubeMetricsMemoryDisplayValue and getPodKubeMetricsCpuDisplayValue are declared outside the PodMetricsMemoryContent and PodMetricsCpuContent declaration, respectively. I would move these functions inside them.

  4. Could you please add a unit test to verify that everything is working correctly? Unit tests are very useful to prevent regressions when someone else updates the code.

These might seem like many points, but they are just small improvements that can make the code more readable, in my opinion 😄. Please reach me out if you need help or if you disagree with something, and we can discuss it.

@dex4er
Copy link
Collaborator

dex4er commented Feb 26, 2025

Does it crash if metrics-server is missing in the cluster?

@mariomamo
Copy link
Collaborator

mariomamo commented Feb 26, 2025

I ran the code, and the app does not crash if a metric server is missing.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants