Skip to content

refactor: remove unnecessary getUserLoggedIn API call in User component#1257

Merged
jescalada merged 7 commits intofinos:mainfrom
andypols:remove-unnecessary-admin-user-fetch
Oct 27, 2025
Merged

refactor: remove unnecessary getUserLoggedIn API call in User component#1257
jescalada merged 7 commits intofinos:mainfrom
andypols:remove-unnecessary-admin-user-fetch

Conversation

@andypols
Copy link
Copy Markdown
Contributor

Summary

This PR simplifies and improves the logic of the User.tsx view.

Previously, the component was making an unnecessary API call to getUserLoggedIn to check if the current user is an admin — information that’s already available through the UserContext provided by the Dashboard layout. Other components (e.g., RepoDetails.tsx and Repositories.tsx) already rely on this context, making the extra call redundant.

Additionally, the component contained several code quality issues:

  • Redundant isAdmin state variable
  • Duplicate logic within the useEffect branches
  • Unnecessary state for derived values (e.g., isProfile)

Fix

  1. Use UserContext for admin checks: Replaced the getUserLoggedIn API call with context access to determine admin status.
  2. Remove redundant API call and state: Eliminated the getUserLoggedIn call and the isAdmin state variable.
  3. Simplify derived state: Replaced the isProfile state with a derived constant: const isProfile = !id;
  4. Refactor useEffect logic: Consolidated duplicate if/else branches into a single getUser call for cleaner, more maintainable code.

@netlify
Copy link
Copy Markdown

netlify Bot commented Oct 25, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 20f4bd0
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68fdfa5dda366800089083a3

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.64%. Comparing base (fdd808d) to head (20f4bd0).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1257   +/-   ##
=======================================
  Coverage   82.64%   82.64%           
=======================================
  Files          70       70           
  Lines        3007     3007           
  Branches      501      501           
=======================================
  Hits         2485     2485           
  Misses        419      419           
  Partials      103      103           

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Copy Markdown
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

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

LGTM! I was wondering if we should do a bit of renaming here to make things more obvious for future contributors?

  • The User view is really just the User page or User Profile, so renaming it to UserProfile would be better.
  • The isProfile boolean could also use a better name - although it's not very clear what it's being used for. Perhaps something like const editable = !id || loggedInUser.admin might work?
  • This is beyond the scope of this PR, but it would be nice to rename all usages of gitAccount (which is actually the GH username) at some point since it no longer makes sense now that we're identifying users by email. Added #1258 to track this

@andypols
Copy link
Copy Markdown
Contributor Author

andypols commented Oct 26, 2025

@jescalada thanks for looking at this - I have:

  • renamed file to UserProfile to match the component
  • and renamed isProfile to isOwnProfile - its the difference when you are looking at the your profile (My Account) vs looking at a user in the list.

Copy link
Copy Markdown
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

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

LGTM!

@jescalada jescalada merged commit ae97d83 into finos:main Oct 27, 2025
14 checks passed
@andypols andypols deleted the remove-unnecessary-admin-user-fetch branch October 28, 2025 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants