feat: Refreshed UI - #1507
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1507 +/- ##
==========================================
- Coverage 90.62% 85.70% -4.92%
==========================================
Files 69 79 +10
Lines 5694 6331 +637
Branches 985 1021 +36
==========================================
+ Hits 5160 5426 +266
- Misses 516 885 +369
- Partials 18 20 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hey @andypols, we discussed this PR at #1506 and are (as a group) excited to see this get done. We debated whether to put this PR in scope for 2.1.0 or whether to go for 2.2.0... There are at least 2 PRs also scoped for 2.1.0 that will need UI additions:
SSH has been waiting for a long time so consensus at the meeting was (reluctantly) to do that first and then ask you to update #1507 to add the additional UI panels (or to collab with you on doing so). Hence, we've marked this for 2.2.0 (and as the only issue in that milestone currently). Does that work for you? Also, please see the below notes from the meeting agenda:
Agenda except from #1506
On the final question, we're expecting it to close a few renovate PRs as we drop some dependencies in favour of new ones? |
|
Hey @kriswest
I think the bigger issue is that planning has started to block releases rather than guide them. We used to release regularly; now worthwhile contributions sit for long periods and get pushed back further and further. SSH is a good example, and I’m concerned the UI work may end up the same way. I’d prefer a simpler approach: merge bug fixes and useful enhancements when they’re ready. Then make release review a standing agenda item at each meeting by asking, “Is there any reason not to release the latest changes?”
I did not implement the paging in the database - we tried to leave the db layer alone and just focus on the UI. #1464 is not redundant, although its not really needed yet and can wait until the ui changes have been made.
Yes, the caching has addressed this. It will not include any project metadata
It has probably improved but will still need to be done.
No this still needs doing. Cypress is hard for us as its blocked internally for security reasons. The existing cypress tests are working locally, no idea why they are failing in the pr tests. |
The delay on the SSH feature was a decision we made collectively because the wholesale refactor of the code conflicted with everything making it very hard to merge. For 2.1.0 we've picked that and a number of other (pretty much) dev complete PRs to get merged to try and get to release relatively quickly to start clearing our backlog, with the goal of getting back to quicker releases as soon as we've got that done. Nothing we've picked needs much new work apart from resolving conflicts and review/testing - we just had to deal with the expected conflict between your UI rebuild and a couple of the PRs that will add to the UI. There's not an easy solution to that other than one or the other going first. As per the meeting minutes, I offered to check-in with you as the meeting was keen to get both the UI and SSH etc. in ASAP. We also have some issues with Cypress testing here - but I've got a machine I can run them on. Perhaps we can get some collab going on resolving those so that the coverage doesn't dip. It might be worth merging this into a FINOS repo branch to do that? |
I think we switched from making smaller changes to great big ones which caused this. SSH and a new UI are not small, but not quite as massive as the whole codebase refactor. Hopefully after these we'll have a run of smaller changes. |
|
I think the cypress failure is down to this id: Which the test expects to be |
|
The dependency review check may be having trouble because its a PR from a fork?? Perhaps setting will help? |
jescalada
left a comment
jescalada
left a comment
There was a problem hiding this comment.
@andypols Fantastic contributions! Tailwind makes the UI code a lot more readable, and the UI is looking much, much better. 😃
I'm wondering if we could/should do some refactoring for the className strings, I think we might benefit from having these centralized (especially for certain elements such as buttons, form inputs, etc.).
Other than a few minor points, this is looking great!
There was a problem hiding this comment.
Looks good!
Can you open a follow-up issue for adding SCM token support so the remaining work stays tracked?
Also, the Linting job fails
Root cause is in
git-proxy/.github/workflows/lint.yml
Line 32 in d07c2f0
npm install --workspaces does not install root devDependencies (where eslint lives).
Suggested fix: replace with npm ci.
Extend allow-dependencies-licenses in .github/workflows/dependency-review.yml with the three packages flagged by the failing workflow: bowser, focus-visible, and spdx-exceptions.
| /** Repo card enrichment (description, language, license, fork, avatars) changes slowly. */ | ||
| const SUCCESS_TTL_MS = 24 * 60 * 60 * 1000; | ||
| /** Cooldown after a failed fetch before treating cache as stale again. */ | ||
| const FAILURE_TTL_MS = 15 * 60 * 1000; |
There was a problem hiding this comment.
Consider exposing as env vars with current values as defaults. 24h is hard to test against, and different deploys may want different values
| Map<string, RepoActivityTabCounts> | ||
| > => { | ||
| const sink = start(); | ||
| const [users, pushes] = await Promise.all([sink.getUsers(), sink.getPushes({ type: 'push' })]); |
There was a problem hiding this comment.
getUserActivityTabCountsByUsernameloads **every** push document into memory on everyGET /api/v1/user`. Cost scales with the full push history and grows forever.
Suggest a DB-side $group aggregation so Mongo returns the counts directly. Same issue in getRepoPushRollupsByCanonicalUrl.
|
|
||
| export const getSessionStore = (): MongoDBStore | undefined => start().getSessionStore(); | ||
| export const getPushes = (query: Partial<PushQuery>): Promise<Action[]> => start().getPushes(query); | ||
| export const getPushesForUserProfile = async (username: string): Promise<Action[]> => { |
There was a problem hiding this comment.
The route handler in routes/users.ts already calls db.findUser(username) for the 404 check, then this wrapper calls findUser again. Also the two paths behave differently when the user doesn't exist (route → 404, wrapper → []).
Could the wrapper accept the already loaded User?
|
|
Workaround for a bug caused by a Vite dependency regression + Material UI's circular dependencies. We should remove the override once #1507 goes in
# Conflicts: # package-lock.json
# Conflicts: # package-lock.json # package.json # src/config/generated/config.ts # src/config/index.ts # src/db/index.ts # src/db/types.ts # src/proxy/index.ts # src/service/urls.ts # src/ui/components/CustomButtons/CodeActionButton.tsx # src/ui/views/User/UserProfile.tsx # test/testProxy.test.ts
Brings in commits from main (8c629c2) including ssh-agent support, tag push, Dockerfile optimizations, and various bug fixes.
Brings in commits from main (d6264eb) including dependabot dependency updates (uuid, globals, env-paths, github-actions, types/supertest).
# Conflicts: # package.json # src/db/mongo/pushes.ts # src/db/types.ts # src/ui/utils.tsx # src/ui/views/Login/Login.tsx # src/ui/views/PushDetails/PushDetails.tsx # src/ui/views/PushRequests/components/PushesTable.tsx # test/db/mongo/push.test.ts
20a76cd to
0554c3b
Compare
Lockfile still reflected the pre-migration react@16/@material-ui tree after the main merge, causing npm ci to fail against @headlessui/react.
5 participants
Summary
This PR modernises the git-proxy UI while keeping backend changes to a minimum.
resolves #1106
resolves #1123
resolves #1176
resolves #1258
resolves #1297
resolves #1420
resolves #1495
UI L&F
Standard top navigation:
Mobile (responsive) navigation:
Repositories
Repository list view
Repository
Repository view
Activities (previously Dashboard)
Activity list view
Users
gitAccount#1258), although it remains in the database.User list view
User
User view
Backend APIs & Services
This branch adds the backend APIs and service-layer support needed for the new UI:
/api/v1/repo/:id/scm-metadataendpoint fetches and caches GitHub/GitLab repository metadata using new provider-specific logic insrc/service/gitProviders. The github api endpoint is currently unauthenticated, but the implementation is structured so future PRs can add bearer-token auth and provider-specific operations such as forking or syncing./api/v1/user/:id/activityendpoint returns all pushes attributed to a user./api/v1/user responsenow includes activity tab counts./api/v1/reporesponse now includes activity tab counts.