Tweaking repositories list UI#115534
Conversation
github-actions
Bot
added
the
Scope: Frontend
📊 Type Coverage Diff✅ No new type safety issues introduced. Coverage: 93.51% |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| <InstallationActions installation={merged} /> | ||
| </Flex> | ||
| </RowButton> | ||
| {expanded && !expandDisabled && ( |
There was a problem hiding this comment.
Bug: A test was not updated after a UI text change from "Manage repositories" to "Manage", causing a guaranteed test failure.
Severity: LOW
Suggested Fix
Update the test in scmRepositoryTable.spec.tsx to assert the presence of the new text, for example by using screen.getByText('Manage').
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
static/app/views/settings/organizationRepositories/components/scmRepositoryTable.tsx#L316
Potential issue: A test in `scmRepositoryTable.spec.tsx` expects to find a link with the
text "Manage repositories", but the component was updated to render the text "Manage"
instead. This discrepancy will cause the test to fail, blocking CI/CD pipelines.
Also affects:
static/app/views/settings/organizationRepositories/components/scmRepositoryTable.spec.tsx
Did we get this right? 👍 / 👎 to inform future reviews.
|
Standard practice for commit messages in this repo can be found here https://develop.sentry.dev/engineering-practices/commit-messages/, since we squash and merge PRs by default this becomes most relevant in the context of the PR title, which will in turn become the commit message on master. If you are creating the PR / committing with claude it should be using Sentry skills / AGENTS.md / CLAUDE.md to follow those practices by default. Looks like some tests need to be updated. |
| <RowButton | ||
| <Stack | ||
| role="region" | ||
| aria-label={merged.integration.name} |
There was a problem hiding this comment.
This was previously labeled with provider.name (e.g. GitHub). It's now just the integration name (e.g. @my-org), so a screen reader navigating by landmarks loses the provider context. Worth combining them, like ${provider.name} ${integration.name}.
| if (visibleRepos.length === 0) { | ||
| return ( | ||
| <Flex padding="xl xl" justify="center" borderTop="primary"> | ||
| {renderEmptyMessage()} | ||
| </Flex> | ||
| ) : ( | ||
| <Grid | ||
| column={outerColumn} | ||
| columns={outerColumns} | ||
| position="relative" | ||
| style={{height: virtualizer.getTotalSize()}} | ||
| > | ||
| {virtualizer.getVirtualItems().map(virtualItem => { | ||
| const repo = visibleRepos[virtualItem.index]!; | ||
| const nameMatch = repoMatches?.[repo.id]?.find(m => m.key === 'name'); | ||
| const isLast = virtualItem.index === visibleRepos.length - 1; | ||
| return ( | ||
| <Fragment key={virtualItem.key}> | ||
| <Container | ||
| column="1/-1" | ||
| position="absolute" | ||
| top="0" | ||
| left="0" | ||
| right="0" | ||
| borderBottom={isLast ? undefined : 'secondary'} | ||
| style={{ | ||
| transform: `translateY(${virtualItem.start}px)`, | ||
| height: virtualItem.size, | ||
| }} | ||
| /> | ||
| <RepoRow | ||
| role="listitem" | ||
| ref={virtualizer.measureElement} | ||
| data-index={virtualItem.index} | ||
| column={contentColumn} | ||
| position="absolute" | ||
| top="0" | ||
| left="0" | ||
| right="0" | ||
| align="center" | ||
| justify="between" | ||
| gap="sm" | ||
| padding={nested ? 'xs xl xs 0' : 'xs lg'} | ||
| style={{transform: `translateY(${virtualItem.start}px)`}} | ||
| > | ||
| <Flex align="center" gap="sm" minWidth="0"> | ||
| <Text> | ||
| {nameMatch | ||
| ? highlightFuseMatches(nameMatch, HighlightMark) | ||
| : repo.name} | ||
| </Text> | ||
| <LinkButton | ||
| className="hover-reveal" | ||
| href={repo.url ?? ''} | ||
| external | ||
| size="zero" | ||
| variant="transparent" | ||
| icon={<IconOpen variant="muted" />} | ||
| aria-label={t('View repository on %s', providerName)} | ||
| tooltipProps={{ | ||
| title: t('View repository on %s', providerName), | ||
| }} | ||
| /> | ||
| </Flex> | ||
| {mappedProjectSlugsByRepoId && ( | ||
| <RepoMappings | ||
| slugs={mappedProjectSlugsByRepoId[repo.id] ?? []} | ||
| mappingsLoading={mappingsLoading} | ||
| action={installation.repoActions?.(repo)} | ||
| /> | ||
| )} | ||
| </RepoRow> | ||
| </Fragment> | ||
| ); | ||
| })} | ||
| </Grid> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The empty/loading/no-match states used to sit inside the role="list" wrapper. This early return is a bare <Flex> outside any list semantics, so "Loading repositories" and "No repositories" aren't announced as part of a list anymore. Could keep the list role on the outer container.
| <Flex column={contentColumn} padding="md xl" justify="center"> | ||
| if (visibleRepos.length === 0) { | ||
| return ( | ||
| <Flex padding="xl xl" justify="center" borderTop="primary"> |
2 participants
Making a few adjustments to the repositories list view :)
Changelog:
Layout
SingleInstallTable/MultiInstallTablesplit and the grouped SCM-title panel)<Panel>with<Stack>(border, radius, overflow:hidden)gap="xl")xs→md)xs→sm)md→xl)Card header
tokens.interactive.link.neutral)warning→dangervariantRepo list
PanelHeadercolumn-heading row ("Repositories" / "Connected Projects"); only renders when there are repos to shownestedmode and subgrid rendering inVirtualizedRepoList