Open
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Cancel button in integration picker is non-functional
- The integration-picker Cancel button now only renders when mappings exist, removing the no-op action in the forced-add state.
- ✅ Fixed: Manual disclosure pattern instead of Disclosure component
- IntegrationCard now uses the core
Disclosurecomponent for expand/collapse behavior instead of localuseStateand manual chevron toggling.
- IntegrationCard now uses the core
- ✅ Fixed: New styled() calls replaceable by Container component
- Both
MappingCardandCardContainerstyled divs were replaced withContainerprimitives using equivalent border, radius, padding, and overflow props.
- Both
Or push these changes by commenting:
@cursor push 16d766d9e5
Preview (16d766d9e5)
diff --git a/static/app/views/settings/organizationRepositories/allCodeMappings.tsx b/static/app/views/settings/organizationRepositories/allCodeMappings.tsx
--- a/static/app/views/settings/organizationRepositories/allCodeMappings.tsx
+++ b/static/app/views/settings/organizationRepositories/allCodeMappings.tsx
@@ -7,7 +7,7 @@
import {Badge} from '@sentry/scraps/badge';
import {Button} from '@sentry/scraps/button';
import {defaultFormOptions, useScrapsForm} from '@sentry/scraps/form';
-import {Flex, Stack} from '@sentry/scraps/layout';
+import {Container, Flex, Stack} from '@sentry/scraps/layout';
import {Text} from '@sentry/scraps/text';
import {addErrorMessage, addSuccessMessage} from 'sentry/actionCreators/indicator';
@@ -324,7 +324,7 @@
<Stack gap="lg">
{/* Existing mappings */}
{mappings.map(mapping => (
- <MappingCard key={mapping.id}>
+ <Container key={mapping.id} padding="lg" border="neutral.muted" radius="md">
<Flex align="center" justify="between">
<Stack gap="xs" flex={1}>
<Text bold size="sm">
@@ -370,12 +370,12 @@
/>
</FormContainer>
)}
- </MappingCard>
+ </Container>
))}
{/* Add form — shown when no mappings exist or toggled */}
{(showAddForm || mappings.length === 0) && addFormIntegration ? (
- <MappingCard>
+ <Container padding="lg" border="neutral.muted" radius="md">
<InlineMappingForm
projectId={project.id}
integration={addFormIntegration}
@@ -397,7 +397,7 @@
);
}}
/>
- </MappingCard>
+ </Container>
) : (showAddForm || mappings.length === 0) && !addFormIntegration ? (
// Multiple integrations — pick which one
<Flex gap="sm" align="center">
@@ -409,13 +409,15 @@
{i.name}
</Button>
))}
- <Button
- size="xs"
- priority="transparent"
- onClick={() => setShowAddForm(false)}
- >
- {t('Cancel')}
- </Button>
+ {mappings.length > 0 && (
+ <Button
+ size="xs"
+ priority="transparent"
+ onClick={() => setShowAddForm(false)}
+ >
+ {t('Cancel')}
+ </Button>
+ )}
</Flex>
) : (
<Button
@@ -625,12 +627,6 @@
border-bottom: 1px solid ${p => p.theme.tokens.border.neutral.muted};
`;
-const MappingCard = styled('div')`
- padding: ${p => p.theme.space.lg};
- border: 1px solid ${p => p.theme.tokens.border.neutral.muted};
- border-radius: ${p => p.theme.radius.md};
-`;
-
const FormContainer = styled('div')`
margin-top: ${p => p.theme.space.lg};
padding-top: ${p => p.theme.space.lg};
diff --git a/static/app/views/settings/organizationRepositories/scmConnectionsView.tsx b/static/app/views/settings/organizationRepositories/scmConnectionsView.tsx
--- a/static/app/views/settings/organizationRepositories/scmConnectionsView.tsx
+++ b/static/app/views/settings/organizationRepositories/scmConnectionsView.tsx
@@ -4,8 +4,9 @@
import {z} from 'zod';
import {Button} from '@sentry/scraps/button';
+import {Disclosure} from '@sentry/scraps/disclosure';
import {AutoSaveForm, FieldGroup} from '@sentry/scraps/form';
-import {Flex, Stack} from '@sentry/scraps/layout';
+import {Container, Flex, Stack} from '@sentry/scraps/layout';
import {ExternalLink} from '@sentry/scraps/link';
import {Text} from '@sentry/scraps/text';
@@ -25,13 +26,7 @@
import {RepoProviderIcon} from 'sentry/components/repositories/repoProviderIcon';
import {ProviderConfigLink} from 'sentry/components/repositories/scmIntegrationTree/providerConfigLink';
import {useScmIntegrationTreeData} from 'sentry/components/repositories/scmIntegrationTree/useScmIntegrationTreeData';
-import {
- IconChevron,
- IconEllipsis,
- IconOpen,
- IconSettings,
- IconSubtract,
-} from 'sentry/icons';
+import {IconEllipsis, IconOpen, IconSettings, IconSubtract} from 'sentry/icons';
import {t} from 'sentry/locale';
import type {
IntegrationProvider,
@@ -240,7 +235,6 @@
const api = useApi();
const organization = useOrganization();
const queryClient = useQueryClient();
- const [expanded, setExpanded] = useState(false);
const [repoSearchError, setRepoSearchError] = useState<number | null | undefined>(null);
const canAccess =
@@ -302,79 +296,83 @@
}, [integration]);
return (
- <CardContainer>
- <CardHeader>
- <RowToggle onClick={() => setExpanded(!expanded)}>
- <IconChevron direction={expanded ? 'down' : 'right'} size="xs" />
- <RepoProviderIcon provider={`integrations:${provider.key}`} size="sm" />
- <Text bold>{integration.name}</Text>
- {integration.domainName && (
- <Text variant="muted" size="sm">
- {integration.domainName}
- </Text>
- )}
- </RowToggle>
-
- <Flex align="center" gap="sm">
- <Text size="sm" variant="muted">
- {t('%s repos connected', connectedRepos.length)}
- </Text>
- <IntegrationReposAddRepository
- integration={integration}
- currentRepositories={connectedRepos}
- onSearchError={setRepoSearchError}
- onAddRepository={() => invalidateRepos()}
- />
- <DropdownMenu
- trigger={triggerProps => (
- <Button
- size="xs"
- priority="transparent"
- aria-label={t('Actions')}
- icon={<IconEllipsis />}
- {...triggerProps}
+ <Container border="primary" radius="md" overflow="hidden">
+ <Disclosure size="xs">
+ <CardHeader>
+ <Disclosure.Title
+ trailingItems={
+ <Flex align="center" gap="sm">
+ <Text size="sm" variant="muted">
+ {t('%s repos connected', connectedRepos.length)}
+ </Text>
+ <IntegrationReposAddRepository
+ integration={integration}
+ currentRepositories={connectedRepos}
+ onSearchError={setRepoSearchError}
+ onAddRepository={() => invalidateRepos()}
+ />
+ <DropdownMenu
+ trigger={triggerProps => (
+ <Button
+ size="xs"
+ priority="transparent"
+ aria-label={t('Actions')}
+ icon={<IconEllipsis />}
+ {...triggerProps}
+ />
+ )}
+ position="bottom-end"
+ items={[
+ {
+ key: 'settings',
+ label: t('Settings'),
+ onAction: openSettingsModal,
+ },
+ {
+ key: 'disconnect',
+ label: t('Disconnect'),
+ priority: 'danger',
+ disabled: !canAccess,
+ onAction: confirmDisconnect,
+ },
+ ]}
+ />
+ </Flex>
+ }
+ >
+ <Flex align="center" gap="md">
+ <RepoProviderIcon provider={`integrations:${provider.key}`} size="sm" />
+ <Text bold>{integration.name}</Text>
+ {integration.domainName && (
+ <Text variant="muted" size="sm">
+ {integration.domainName}
+ </Text>
+ )}
+ </Flex>
+ </Disclosure.Title>
+ </CardHeader>
+ <Disclosure.Content>
+ <CardBody>
+ {connectedRepos.map(repo => (
+ <RepoRow
+ key={repo.id}
+ repo={repo}
+ domainName={integration.domainName}
+ canAccess={canAccess}
+ onRemove={() => handleRemoveRepo(repo)}
/>
+ ))}
+ {repoSearchError === 400 && (
+ <Flex padding="sm lg">
+ <Text size="xs" variant="muted">
+ {t('Unable to fetch repos. Try reconnecting.')}
+ </Text>
+ </Flex>
)}
- position="bottom-end"
- items={[
- {
- key: 'settings',
- label: t('Settings'),
- onAction: openSettingsModal,
- },
- {
- key: 'disconnect',
- label: t('Disconnect'),
- priority: 'danger',
- disabled: !canAccess,
- onAction: confirmDisconnect,
- },
- ]}
- />
- </Flex>
- </CardHeader>
-
- {expanded && (
- <CardBody>
- {connectedRepos.map(repo => (
- <RepoRow
- key={repo.id}
- repo={repo}
- domainName={integration.domainName}
- canAccess={canAccess}
- onRemove={() => handleRemoveRepo(repo)}
- />
- ))}
- {repoSearchError === 400 && (
- <Flex padding="sm lg">
- <Text size="xs" variant="muted">
- {t('Unable to fetch repos. Try reconnecting.')}
- </Text>
- </Flex>
- )}
- </CardBody>
- )}
- </CardContainer>
+ </CardBody>
+ </Disclosure.Content>
+ </Disclosure>
+ </Container>
);
}
@@ -722,12 +720,6 @@
gap: ${p => p.theme.space.xl};
`;
-const CardContainer = styled('div')`
- border: 1px solid ${p => p.theme.tokens.border.primary};
- border-radius: ${p => p.theme.radius.md};
- overflow: hidden;
-`;
-
const CardHeader = styled('div')`
display: flex;
align-items: center;
@@ -740,18 +732,6 @@
border-top: 1px solid ${p => p.theme.tokens.border.primary};
`;
-const RowToggle = styled('button')`
- display: flex;
- align-items: center;
- gap: ${p => p.theme.space.md};
- background: none;
- border: none;
- padding: 0;
- cursor: pointer;
- color: inherit;
- font: inherit;
-`;
-
const RepoRowContainer = styled('div')`
display: flex;
align-items: center;This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| > | ||
| {t('Cancel')} | ||
| </Button> | ||
| </Flex> |
Contributor
There was a problem hiding this comment.
Cancel button in integration picker is non-functional
Medium Severity
When there are no existing mappings (mappings.length === 0), the integration picker's Cancel button calls setShowAddForm(false), but the ternary condition governing this branch is (showAddForm || mappings.length === 0). Since mappings.length === 0 remains true, this condition always evaluates to true regardless of showAddForm, making the Cancel button completely non-functional. A clickable Cancel button that does nothing is confusing.
| )} | ||
| </CardBody> | ||
| )} | ||
| </CardContainer> |
Contributor
There was a problem hiding this comment.
Manual disclosure pattern instead of Disclosure component
Low Severity
The IntegrationCard manually implements a disclosure/expand-collapse pattern using useState(false), IconChevron direction toggling, and conditional rendering — this is the exact anti-pattern called out in the local AGENTS.md, which says to use the core Disclosure component instead of reimplementing it manually.
Triggered by project rule: Frontend guidelines
| padding: ${p => p.theme.space.lg}; | ||
| border: 1px solid ${p => p.theme.tokens.border.neutral.muted}; | ||
| border-radius: ${p => p.theme.radius.md}; | ||
| `; |
Contributor
There was a problem hiding this comment.
New styled() calls replaceable by Container component
Low Severity
MappingCard and CardContainer are newly added styled('div') calls that only set padding, border, and border-radius — exactly the use case the AGENTS.md says to handle with <Container> from @sentry/scraps/layout. The Container component supports padding, border (accepting theme border variants like "neutral.muted" or "primary"), radius, and overflow props directly.
Additional Locations (1)
Triggered by project rule: Frontend guidelines
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 6 total unresolved issues (including 3 from previous reviews).
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: New styled components where Container primitive suffices
- Replaced the new
CardContainerandMappingCardstyled div wrappers withContainerprimitives using equivalent border, radius, overflow, and padding props.
- Replaced the new
- ✅ Fixed: Duplicate hook calls fetch same data twice
- Removed the duplicate
useScmConnectionsData()call by computing it once inOrganizationRepositoriesand passing the result intoScmConnectionsViewas props.
- Removed the duplicate
- ✅ Fixed: Mutable render-time side effect in dialogRefs pattern
- Converted
dialogRefsfrom a local mutable object to a stableuseRefstore so dialog callbacks persist across renders without recreating mutable render-local state.
- Converted
Or push these changes by commenting:
@cursor push 0019d30c53
Preview (0019d30c53)
diff --git a/static/app/views/settings/organizationRepositories/allCodeMappings.tsx b/static/app/views/settings/organizationRepositories/allCodeMappings.tsx
--- a/static/app/views/settings/organizationRepositories/allCodeMappings.tsx
+++ b/static/app/views/settings/organizationRepositories/allCodeMappings.tsx
@@ -7,7 +7,7 @@
import {Badge} from '@sentry/scraps/badge';
import {Button} from '@sentry/scraps/button';
import {defaultFormOptions, useScrapsForm} from '@sentry/scraps/form';
-import {Flex, Stack} from '@sentry/scraps/layout';
+import {Container, Flex, Stack} from '@sentry/scraps/layout';
import {Text} from '@sentry/scraps/text';
import {addErrorMessage, addSuccessMessage} from 'sentry/actionCreators/indicator';
@@ -324,7 +324,7 @@
<Stack gap="lg">
{/* Existing mappings */}
{mappings.map(mapping => (
- <MappingCard key={mapping.id}>
+ <Container key={mapping.id} padding="lg" border="muted" radius="md">
<Flex align="center" justify="between">
<Stack gap="xs" flex={1}>
<Text bold size="sm">
@@ -370,12 +370,12 @@
/>
</FormContainer>
)}
- </MappingCard>
+ </Container>
))}
{/* Add form — shown when no mappings exist or toggled */}
{(showAddForm || mappings.length === 0) && addFormIntegration ? (
- <MappingCard>
+ <Container padding="lg" border="muted" radius="md">
<InlineMappingForm
projectId={project.id}
integration={addFormIntegration}
@@ -397,7 +397,7 @@
);
}}
/>
- </MappingCard>
+ </Container>
) : (showAddForm || mappings.length === 0) && !addFormIntegration ? (
// Multiple integrations — pick which one
<Flex gap="sm" align="center">
@@ -625,12 +625,6 @@
border-bottom: 1px solid ${p => p.theme.tokens.border.neutral.muted};
`;
-const MappingCard = styled('div')`
- padding: ${p => p.theme.space.lg};
- border: 1px solid ${p => p.theme.tokens.border.neutral.muted};
- border-radius: ${p => p.theme.radius.md};
-`;
-
const FormContainer = styled('div')`
margin-top: ${p => p.theme.space.lg};
padding-top: ${p => p.theme.space.lg};
diff --git a/static/app/views/settings/organizationRepositories/index.tsx b/static/app/views/settings/organizationRepositories/index.tsx
--- a/static/app/views/settings/organizationRepositories/index.tsx
+++ b/static/app/views/settings/organizationRepositories/index.tsx
@@ -15,7 +15,7 @@
export default function OrganizationRepositories() {
const organization = useOrganization();
- const {hasConnections, scmProviders, refetchIntegrations} = useScmConnectionsData();
+ const scmConnectionsData = useScmConnectionsData();
return (
<AnalyticsArea name="source-code-management">
@@ -23,10 +23,10 @@
<SettingsPageHeader
title={t('Source Code')}
action={
- hasConnections ? (
+ scmConnectionsData.hasConnections ? (
<ProviderDropdown
- providers={scmProviders}
- onAddIntegration={refetchIntegrations}
+ providers={scmConnectionsData.scmProviders}
+ onAddIntegration={scmConnectionsData.refetchIntegrations}
buttonText={t('Connect Source Code')}
size="sm"
/>
@@ -35,7 +35,7 @@
/>
<Stack gap="3xl">
- <ScmConnectionsView />
+ <ScmConnectionsView data={scmConnectionsData} />
<AllCodeMappings />
</Stack>
</AnalyticsArea>
diff --git a/static/app/views/settings/organizationRepositories/scmConnectionsView.tsx b/static/app/views/settings/organizationRepositories/scmConnectionsView.tsx
--- a/static/app/views/settings/organizationRepositories/scmConnectionsView.tsx
+++ b/static/app/views/settings/organizationRepositories/scmConnectionsView.tsx
@@ -1,11 +1,11 @@
-import {Fragment, useCallback, useState} from 'react';
+import {Fragment, useCallback, useRef, useState} from 'react';
import styled from '@emotion/styled';
import {mutationOptions} from '@tanstack/react-query';
import {z} from 'zod';
import {Button} from '@sentry/scraps/button';
import {AutoSaveForm, FieldGroup} from '@sentry/scraps/form';
-import {Flex, Stack} from '@sentry/scraps/layout';
+import {Container, Flex, Stack} from '@sentry/scraps/layout';
import {ExternalLink} from '@sentry/scraps/link';
import {Text} from '@sentry/scraps/text';
@@ -77,9 +77,11 @@
// Main component
// ─────────────────────────────────────────────────────────────
-export function ScmConnectionsView() {
- const data = useScmConnectionsData();
-
+export function ScmConnectionsView({
+ data,
+}: {
+ data: ReturnType<typeof useScmConnectionsData>;
+}) {
if (data.isPending) {
return (
<Flex justify="center" padding="xl" minHeight={200}>
@@ -302,7 +304,7 @@
}, [integration]);
return (
- <CardContainer>
+ <Container border="primary" radius="md" overflow="hidden">
<CardHeader>
<RowToggle onClick={() => setExpanded(!expanded)}>
<IconChevron direction={expanded ? 'down' : 'right'} size="xs" />
@@ -374,7 +376,7 @@
)}
</CardBody>
)}
- </CardContainer>
+ </Container>
);
}
@@ -653,6 +655,7 @@
const organization = useOrganization();
const canAccess =
hasEveryAccess(['org:integrations'], {organization}) || isActiveSuperuser();
+ const dialogRefsRef = useRef<Record<string, (() => void) | undefined>>({});
const singleProvider = providers.length === 1 ? providers[0] : undefined;
if (singleProvider) {
@@ -671,8 +674,6 @@
// Render a hidden AddIntegration for each provider so we can grab their openDialog refs.
// When a user picks from the dropdown, we immediately trigger the OAuth flow.
- const dialogRefs: Record<string, (() => void) | undefined> = {};
-
return (
<Fragment>
{/* Hidden AddIntegration instances to capture openDialog callbacks */}
@@ -684,7 +685,7 @@
onInstall={() => onAddIntegration()}
>
{openDialog => {
- dialogRefs[p.key] = openDialog;
+ dialogRefsRef.current[p.key] = openDialog;
return null;
}}
</AddIntegration>
@@ -701,7 +702,7 @@
label: p.name,
leadingItems: <RepoProviderIcon provider={`integrations:${p.key}`} size="xs" />,
disabled: !p.canAdd,
- onAction: () => dialogRefs[p.key]?.(),
+ onAction: () => dialogRefsRef.current[p.key]?.(),
}))}
/>
</Fragment>
@@ -722,12 +723,6 @@
gap: ${p => p.theme.space.xl};
`;
-const CardContainer = styled('div')`
- border: 1px solid ${p => p.theme.tokens.border.primary};
- border-radius: ${p => p.theme.radius.md};
- overflow: hidden;
-`;
-
const CardHeader = styled('div')`
display: flex;
align-items: center;This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 38c183b. Configure here.
| border: 1px solid ${p => p.theme.tokens.border.primary}; | ||
| border-radius: ${p => p.theme.radius.md}; | ||
| overflow: hidden; | ||
| `; |
Contributor
There was a problem hiding this comment.
New styled components where Container primitive suffices
Low Severity
Several newly added styled() calls (e.g. CardContainer, MappingCard) apply only border, border-radius, padding, and overflow — all of which are supported props on the Container core component from @sentry/scraps/layout. Per the frontend guidelines in static/AGENTS.md, Container is preferred over styled('div') for elements that require a border or border radius.
Additional Locations (1)
Triggered by project rule: Frontend guidelines
Reviewed by Cursor Bugbot for commit 38c183b. Configure here.
| return null; | ||
| }} | ||
| </AddIntegration> | ||
| ))} |
Contributor
There was a problem hiding this comment.
Mutable render-time side effect in dialogRefs pattern
Low Severity
dialogRefs is a plain local object mutated during render via AddIntegration children callbacks. Mutating non-ref variables during render is a React anti-pattern — in Strict Mode and concurrent rendering, React may invoke render functions multiple times. A useRef would be the correct way to hold mutable values across the render lifecycle.
Reviewed by Cursor Bugbot for commit 38c183b. Configure here.
| export default function OrganizationRepositories() { | ||
| const organization = useOrganization(); | ||
| const {repoFilter, setRepoFilter, searchTerm, setSearchTerm} = useScmTreeFilters(); | ||
| const {hasConnections, scmProviders, refetchIntegrations} = useScmConnectionsData(); |
Contributor
There was a problem hiding this comment.
Duplicate hook calls fetch same data twice
Low Severity
useScmConnectionsData() is called in the parent OrganizationRepositories component and again inside the child ScmConnectionsView. Both internally call useScmIntegrationTreeData(), which runs multiple useQuery, useInfiniteQuery, useQueries hooks plus useMemo computations and an auto-pagination useEffect. While React Query deduplicates actual API fetches, the duplicated hook invocations are unnecessary overhead and can be avoided by passing the data as props.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 38c183b. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an experimental PR, exploring how we can refactor the SCM settings and configuration