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.21% 85.19% -5.03%
==========================================
Files 69 79 +10
Lines 5511 6148 +637
Branches 944 980 +36
==========================================
+ Hits 4972 5238 +266
- Misses 521 890 +369
- Partials 18 20 +2 ☔ View full report in Codecov by Sentry. 🚀 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!
| row[tab] += 1; | ||
| } | ||
|
|
||
| function bumpMaxTimestampMs( |
There was a problem hiding this comment.
It'd be great to add a docstring to explain what bumpMaxTimestampMs and bumpCount are doing; it isn't obvious at first glance 🙂
There was a problem hiding this comment.
Otherwise, more descriptive function names could also work as with bumpUserActivityCount below
There was a problem hiding this comment.
Thats fair - not the clearest naming. Although they are placeholders and will be probably get replaced when we implement #1464
| row[tab] += 1; | ||
| } | ||
|
|
||
| export const getUserActivityTabCountsByUsername = async (): Promise< |
There was a problem hiding this comment.
Is there a reason why these functions (bumpUserActivityCount, getUserActivityCountsByUsername) were extracted to src/db/index.ts? Meanwhile the equivalent ones for repos are split into each of the database implementations (src/db/file/pushes.ts, src/db/mongo/pushes.ts).
I'm wondering since we'll add PostgreSQL support soon, perhaps we want to make this more consistent (by either extracting or splitting all helpers) or otherwise explain why not all functions were extracted.
There was a problem hiding this comment.
I just verified that the bumpMaxTimestampMs function is identical in both file and mongo implementations - seems like a prime candidate for extraction 😃
| */ | ||
|
|
||
| export function escapeRegex(s: string): string { | ||
| return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); |
There was a problem hiding this comment.
Should we make a string helpers file just for processing strings, or perhaps import an existing library for this? We're messing with strings in various ways elsewhere and might benefit from having it centralized.
Some other examples are:
normalizeRepoPathForMatchinsrc/activity/canonicalRemoteUrl.tstrimTrailingDotGitinsrc/db/helper.ts
Feel free to ignore if too much hassle, this seems more appropriate for a separate PR anyways 🙂
| status: 'pending', | ||
| icon: EyeIcon, | ||
| className: | ||
| 'inline-flex items-center gap-1 rounded-full border border-[var(--borderColor-attention-muted)] bg-[var(--bgColor-attention-muted)] px-2 py-0.5 text-xs font-medium text-[var(--fgColor-attention)] !no-underline hover:border-[var(--borderColor-attention-emphasis)]', |
There was a problem hiding this comment.
Would it make sense to extract the common elements in these className strings into a separate file for Tailwind classes? I think it might prevent issues with inconsistent or unmaintainable styles down the road.
There was a problem hiding this comment.
I’d avoid extracting Tailwind class strings into a separate file just for reuse. Tailwind’s philosophy is utility-first and markup-local: you generally keep the Tailwind classes on the element they style. A core benefit of Tailwind is that styles stay colocated with the markup, which makes components easier to understand and reduces the chance of orphaned styles.
If the same group of classes is repeated in several places, I’d treat that as a signal that we may want to extract a component, or possibly a variant helper, rather than creating shared class constants.
|
|
||
| /** Git-style green (matches common "Code" affordance). */ | ||
| const codeButtonGreenClassName = | ||
| '!border-0 !shadow-none !bg-[#1a7f37] !text-white hover:!bg-[#136c2e] hover:!text-white active:!bg-[#115f2a]'; |
There was a problem hiding this comment.
Another candidate for class string extraction 🙂
There was a problem hiding this comment.
Might be a good candidate for class string extraction to a shared file, though I'm not sure if it's more appropriate to have these split into style files for each individual component as we did here (we would sacrifice consistency for the sake of quick lookups vs. putting everything in a single src/ui/tailwindClasses.ts).
| type='button' | ||
| data-testid='dashboard-header-sign-in' | ||
| onClick={goLogin} | ||
| className='rounded-md border border-solid border-[rgba(240,246,252,0.35)] bg-black px-3 py-1 text-sm font-medium leading-normal text-[rgba(240,246,252,0.92)] antialiased [font-family:var(--fontStack-sansSerif)] hover:border-[rgba(240,246,252,0.5)] hover:bg-neutral-950 hover:text-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-[var(--header-accent)] focus-visible:ring-offset-2 focus-visible:ring-offset-[var(--header-bgColor)]' |
There was a problem hiding this comment.
Perhaps button styles and colors could be extracted here?
|
|
||
| export const sortRepoViews = ( | ||
| repos: RepoView[], | ||
| sort: RepoSortField, |
There was a problem hiding this comment.
I think the import for RepoSortField is missing in this file 🤔
| <div key={logIndex} className={classes.logItem}> | ||
| <div | ||
| key={logIndex} | ||
| className='min-w-0 rounded-sm border-l-2 border-(--borderColor-muted) bg-(--bgColor-muted) px-2 py-1 font-mono text-xs break-words text-(--fgColor-default)' |
There was a problem hiding this comment.
These style strings look like they could be extracted
4 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.