Skip to content

feat(repos): Add installation settings button and drawer in repos v2#114792

Merged
evanpurkhiser merged 1 commit intomasterfrom
evanpurkhiser/feat-repos-add-installation-settings-button-and-drawer-in-repos-v2
May 5, 2026
Merged

feat(repos): Add installation settings button and drawer in repos v2#114792
evanpurkhiser merged 1 commit intomasterfrom
evanpurkhiser/feat-repos-add-installation-settings-button-and-drawer-in-repos-v2

Conversation

@evanpurkhiser
Copy link
Copy Markdown
Member

Adds useInstallationSettings to the repos v2 page, opening a per-integration
settings drawer that auto-saves each configOrganization field. The settings
button is always visible but disabled while the per-integration config fetch is
in-flight.

Forwarded via settingsButtonProps on ScmInstallation, keeping the table
component generic.

Adds useInstallationSettings to the repos v2 page, opening a per-integration
settings drawer that auto-saves each configOrganization field. The settings
button is always visible but disabled while the per-integration config fetch is
in-flight.

Forwarded via settingsButtonProps on ScmInstallation, keeping the table
component generic.
@evanpurkhiser evanpurkhiser requested a review from a team as a code owner May 4, 2026 22:29
@evanpurkhiser evanpurkhiser requested review from a team and removed request for a team May 4, 2026 22:29
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 4, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

📊 Type Coverage Diff

Metric Before After Delta
Coverage 93.40% 93.40% ±0%
Typed 134,900 134,928 🟢 +28
Untyped 9,528 9,529 🔴 +1
🔍 1 new type safety issue introduced

Type assertions (as) (1 new)

File Line Detail
static/app/views/settings/organizationRepositoriesV2/useInstallationSettings.tsx 122 `as FieldValue<
                    typeof fieldConfig
                  >` — `resolvedIntegration.configData?.[fieldConfig.name] as FieldValue< typeof fieldC…` |

This is informational only and does not block the PR.

Comment on lines +142 to +144
settingsButtonProps: {
disabled: configByIntegrationId[integration.id] === undefined,
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: If the API call to fetch an integration's configuration fails, the settings button becomes permanently disabled because configByIntegrationId[integration.id] is undefined.
Severity: MEDIUM

Suggested Fix

The UI should differentiate between a loading state and an error state. Instead of just checking if configByIntegrationId[integration.id] is undefined, check the status of the corresponding query. If there's an error, display an error message to the user and potentially allow them to retry the action, rather than permanently disabling the button.

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/organizationRepositoriesV2/index.tsx#L142-L144

Potential issue: The `combine` function in `useInstallationSettings.tsx` returns
`undefined` for an integration if its corresponding API call fails. In `index.tsx`, the
settings button's `disabled` prop is set to `configByIntegrationId[integration.id] ===
undefined`. While this correctly disables the button during the initial loading state,
it also causes the button to become permanently disabled if the API call fails after
retries. The user is not shown any error message and has no way to recover or retry the
action. The `openInstallationSettings` function has a fallback `?? integration` which
suggests an intent to handle this case, but this code is unreachable as the button that
triggers it is disabled.

Did we get this right? 👍 / 👎 to inform future reviews.

: (resolvedIntegration.configOrganization?.map(f => ({
...f,
disabled: true,
disabledReason: NO_ACCESS_REASON,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just double checked and I don’t think we map disabledReason correctly to the new form system. I’ll fix this

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evanpurkhiser evanpurkhiser merged commit 7da3249 into master May 5, 2026
74 checks passed
@evanpurkhiser evanpurkhiser deleted the evanpurkhiser/feat-repos-add-installation-settings-button-and-drawer-in-repos-v2 branch May 5, 2026 15:15
evanpurkhiser added a commit that referenced this pull request May 5, 2026
Follow-up to #114792. adds test for rendering and form submission, and
swaps `getByText` for `getByRole('checkbox', {name: ...})`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants