feat(repos): Add installation settings button and drawer in repos v2#114792
Conversation
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.
📊 Type Coverage Diff
🔍 1 new type safety issue introducedType assertions (
This is informational only and does not block the PR. |
| settingsButtonProps: { | ||
| disabled: configByIntegrationId[integration.id] === undefined, | ||
| }, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
I just double checked and I don’t think we map disabledReason correctly to the new form system. I’ll fix this
There was a problem hiding this comment.
Follow-up to #114792. adds test for rendering and form submission, and swaps `getByText` for `getByRole('checkbox', {name: ...})`.
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.