feat(GitLab): Browse GitLab issues and merge requests (frontend)#7273
feat(GitLab): Browse GitLab issues and merge requests (frontend)#7273
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
github-actions
bot
added
the
front-end
emyller
changed the title
emyller
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
github-actions
bot
added
feature
Docker builds report
|
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
|
Visual Regression16 screenshots compared. See report for details. |
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Good one. Couple of comments, the encodeURIComponent is the one worth fixing while we are at it. Ideally the error message in the Select.
I allowed myself a couple of less important and style comments that could be tackled at the same time.
And whether you want to use a flag
| query: (query: Req['getGitLabIssues']) => ({ | ||
| url: | ||
| `projects/${query.project_id}/gitlab/issues/` + | ||
| `?gitlab_project_id=${query.gitlab_project_id}&page_size=${query.page_size ?? 100}&page=${query.page ?? 1}&search_text=${query.q || ''}&state=opened`, |
There was a problem hiding this comment.
Ideally we would use encodeURIComponent here to avoid corrupting the url with spaces for example
There was a problem hiding this comment.
Also state is part of the type and hardcoded here. If we don't plan to use it and leave it hardcoded here, we should remove it from the type (as unused).
I would prefer instead to show the intention and have state={query.state || "opened"} here to avoid surprises later.
Same below
There was a problem hiding this comment.
Ideally we would use
encodeURIComponenthere to avoid corrupting the url with spaces for example
There was a problem hiding this comment.
Also state is part of the type and hardcoded here. If we don't plan to use it and leave it hardcoded here, we should remove it from the type (as unused).
Good call. I made it clearer in the code (see aee2bda) that we only intend to query open items.
| query: (query: Req['getGitLabMergeRequests']) => ({ | ||
| url: | ||
| `projects/${query.project_id}/gitlab/merge-requests/` + | ||
| `?gitlab_project_id=${query.gitlab_project_id}&page_size=${query.page_size ?? 100}&page=${query.page ?? 1}&search_text=${query.q || ''}&state=opened`, |
There was a problem hiding this comment.
Same with encodeUriComponent
| githubTypes[0]?.resourceType, | ||
| ) | ||
|
|
||
| const addGithubResource = (githubResource: GithubResource) => { |
There was a problem hiding this comment.
NIT: I know this was legacy but it would be nice to catch and toast the error here. And would be great to turn it async/await at the same time
There was a problem hiding this comment.
I agree, but I realise now these changes actually belong in #7274. I've removed the code from this branch, and will address it in the next one in series.
| value={ | ||
| data?.results | ||
| ? data.results | ||
| .map((p) => ({ | ||
| label: p.path_with_namespace, | ||
| value: p.id, | ||
| })) | ||
| .find((o) => o.value === value) | ||
| : null | ||
| } |
There was a problem hiding this comment.
| value={ | |
| data?.results | |
| ? data.results | |
| .map((p) => ({ | |
| label: p.path_with_namespace, | |
| value: p.id, | |
| })) | |
| .find((o) => o.value === value) | |
| : null | |
| } | |
| value={data?.results | |
| ?.map((p) => ({ label: p.path_with_namespace, value: p.id })) | |
| .find((o) => o.value === value) ?? null} |
| options={data?.results | ||
| ?.filter( | ||
| (r: GitLabIssue | GitLabMergeRequest) => | ||
| !linkedUrls.includes(r.web_url), | ||
| ) | ||
| .map((r: GitLabIssue | GitLabMergeRequest) => ({ | ||
| label: `${r.title} #${r.iid}`, | ||
| value: r, | ||
| }))} |
There was a problem hiding this comment.
NIT: we could avoid this heavy syntax in the JSX and extract it in a variable (I think it would make the type inference straightforward too)
| isLoading | ||
| ? 'Loading...' | ||
| : isError | ||
| ? 'Failed to load projects' | ||
| : 'Select GitLab Project' | ||
| } |
There was a problem hiding this comment.
It doesn't look like the right place to show the error. On top of the disabled state of the select, it will be easy to miss out.
Maybe use <ErrorMessage /> below the select?
There was a problem hiding this comment.
Would you prefer the <ErrorMessage /> for this particular use case, or a toast? I wonder where to draw the line.
There was a problem hiding this comment.
I think I know where the line is drawn: toast happens if you click a button.
See ed874d6 — I wish ErrorMessage would support adding a "Retry" action, and a link to Integrations as convenience.
There was a problem hiding this comment.
Also added to the PR description:
talissoncosta
left a comment
talissoncosta
left a comment
There was a problem hiding this comment.
Really solid review from @Zaimwa9
Agree with all of it. Just adding a few small points on top, just to prevent the any's to spread around.
| >({ | ||
| providesTags: [{ id: 'LIST', type: 'GitLab' }], | ||
| query: (query: Req['getGitLabIssues']) => ({ | ||
| url: |
There was a problem hiding this comment.
These URLs aren't encoded, so a search like foo&state=closed sneaks in as extra query params. Any chance we can switch to URLSearchParams here? Same thing exists in useGithub.ts already, but nice not to spread it further.
There was a problem hiding this comment.
URLSearchParams was also my instinct, but I learned we already ship with an utility. See above.
| githubId={githubId} | ||
| resourceType={githubResourceType} | ||
| setResourceType={setGithubResourceType} | ||
| onChange={addGithubResource as any} |
There was a problem hiding this comment.
Not trying to nitpick — just keen to avoid the two any casts slipping in here. The request types downstream (getGithubRepositories, getGithubResources) are already organisation_id: number / github_id: number, but the select components above them are typed as string, which is what forces the casts at the call site.
The onChange cast is a separate issue: the prop is declared (value: string) => void but the component actually passes a GithubResource (see GitHubResourcesSelect.tsx:104-107), which is why addGithubResource as any is needed.
If you're up for it, a few small edits clear both anys and make the chain consistent:
// useHasGithubIntegration.ts — drop the '' fallback
githubId: data?.results?.[0]?.id, // number | undefined// MyRepositoriesSelect.tsx props
githubId: number // was string
orgId: number // was string// GitHubResourcesSelect.tsx props
onChange: (value: GithubResource) => void // was (value: string) => void
githubId: number // was string
orgId: number // was string// GitHubLinkSection.tsx props
githubId: number // was stringThen the JSX in GitHubLinkSection is just:
onChange={addGithubResource}
orgId={organisationId}GitHubLinkSection is the only consumer of GitHubResourcesSelect, and MyRepositoriesSelect is only used inside GitHubResourcesSelect itself — so no wider fallout. Happy for it to be a follow-up PR if you'd rather keep this one tight.
There was a problem hiding this comment.
Disclaimer: I am not the author of this code, and I intended minimal-to-no changes to existing code. That said, as per this other comment, I may address this if it's a quick win, but in the next pull request in the series (#7274).
| import GitLabSearchSelect from 'components/GitLabSearchSelect' | ||
| import type { GitLabIssue, GitLabMergeRequest } from 'common/types/responses' | ||
|
|
||
| type GitLabLinkType = 'issue' | 'merge_request' |
There was a problem hiding this comment.
This is defined inlined again as linkType: 'issue' | 'merge_request' in GitLabSearchSelect. Worth pulling it out once in common/types/responses.ts) and importing in both places — keeps them from drifting if we ever add e.g. 'epic'.
There was a problem hiding this comment.
Good callout! 6477ce8
Side note: it bugs me a bit that this module is named common/types/responses, and yet, this, and some other types already in it, are not related to API responses.
github-actions
bot
added
feature
emyller
force-pushed
the
feat/gitlab-browse-frontend
branch
from
f48050b to
ed874d6
Compare
github-actions
bot
added
feature
|
Thanks for such a great review @Zaimwa9 and @talissoncosta, and for the patience guiding me through 2026 frontend land! All comments addressed — please resolve each thread, or respond with advice. Thanks in advance. |
| {isProjectsError && ( | ||
| <ErrorMessage error='Failed to load GitLab projects' /> | ||
| )} |
There was a problem hiding this comment.
Feel free to resolve.
Your question when to use toast versus ErrorMessage made me think and what could make sense to me is to show a toast following a punctual action that failed (button click etc, PUT / POST) and using an ErrorMessage on a component that can't be used persistently because fetching the data failed without action from the user.
Wdyt @talissoncosta ?
3 participants
I have added information todocs/if required so people know about the feature.Changes
Contributes to #7160
Stack: #7270 (backend) → #7273 (this) → #7274 (linking)
With the backend proxy endpoints in place (#7270), the frontend can now let users browse their GitLab projects, issues, and merge requests directly from the feature flag Links tab.
How to review
modals/create-feature/index.tsx.GitLabLinkSectionandGitHubLinkSectionare siblings.Review effort: 2/5
How did you test this code?
Manual UI testing. Screenshots: