Skip to content

fix: post-merge settings bugs — SCIM duplication, crash, error boundary #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TerrifiedBug merged 2 commits into from
Mar 9, 2026
Merged

fix: post-merge settings bugs — SCIM duplication, crash, error boundary #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41219bb
fix: resolve post-merge settings bugs — SCIM duplication, team settin…
TerrifiedBug Mar 9, 2026
6606bfe
fix: alert UI polish — hide threshold for binary metrics, hide pipeli…
TerrifiedBug Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 69 additions & 52 deletions src/app/(dashboard)/alerts/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,17 @@ const CONDITION_LABELS: Record<string, string> = {

const BINARY_METRICS = new Set(["node_unreachable", "pipeline_crashed"]);

/** Metrics that cannot be scoped to a specific pipeline. */
const GLOBAL_METRICS = new Set([
"node_unreachable",
"new_version_available",
"scim_sync_failed",
"backup_failed",
"certificate_expiring",
"node_joined",
"node_left",
]);

const CHANNEL_TYPE_LABELS: Record<string, string> = {
slack: "Slack",
email: "Email",
Expand Down Expand Up @@ -234,14 +245,14 @@ function AlertRulesSection({ environmentId }: { environmentId: string }) {

const openEdit = (rule: (typeof rules)[0]) => {
setEditingRuleId(rule.id);
const isEvent = isEventMetric(rule.metric);
const skipThreshold = isEventMetric(rule.metric) || BINARY_METRICS.has(rule.metric);
setForm({
name: rule.name,
pipelineId: rule.pipelineId ?? "",
metric: rule.metric,
condition: isEvent ? "" : (rule.condition ?? "gt"),
threshold: isEvent ? "" : String(rule.threshold ?? ""),
durationSeconds: isEvent ? "" : String(rule.durationSeconds ?? ""),
condition: skipThreshold ? "" : (rule.condition ?? "gt"),
threshold: skipThreshold ? "" : String(rule.threshold ?? ""),
durationSeconds: skipThreshold ? "" : String(rule.durationSeconds ?? ""),
channelIds: rule.channels?.map((c) => c.channelId) ?? [],
});
setDialogOpen(true);
Expand All @@ -264,11 +275,13 @@ function AlertRulesSection({ environmentId }: { environmentId: string }) {
return;
}

const skipThreshold = isEvent || isBinary;

if (editingRuleId) {
updateMutation.mutate({
id: editingRuleId,
name: form.name,
...(isEvent
...(skipThreshold
? {}
: {
threshold: parseFloat(form.threshold),
Expand All @@ -282,9 +295,9 @@ function AlertRulesSection({ environmentId }: { environmentId: string }) {
environmentId,
pipelineId: form.pipelineId || undefined,
metric: form.metric as AlertMetric,
condition: isEvent ? null : (form.condition as AlertCondition),
threshold: isEvent ? null : parseFloat(form.threshold),
durationSeconds: isEvent ? null : (parseInt(form.durationSeconds, 10) || 60),
condition: skipThreshold ? null : (form.condition as AlertCondition),
threshold: skipThreshold ? null : parseFloat(form.threshold),
durationSeconds: skipThreshold ? null : (parseInt(form.durationSeconds, 10) || 60),
teamId: selectedTeamId!,
channelIds: form.channelIds.length > 0 ? form.channelIds : undefined,
});
Expand Down Expand Up @@ -342,14 +355,18 @@ function AlertRulesSection({ environmentId }: { environmentId: string }) {
</Badge>
</TableCell>
<TableCell className="font-mono">
{rule.condition ? (CONDITION_LABELS[rule.condition] ?? rule.condition) : "—"}
{BINARY_METRICS.has(rule.metric) || !rule.condition ? "—" : (CONDITION_LABELS[rule.condition] ?? rule.condition)}
</TableCell>
<TableCell className="font-mono">
{BINARY_METRICS.has(rule.metric) ? "—" : (rule.threshold ?? "—")}
</TableCell>
<TableCell className="font-mono">{rule.threshold ?? "—"}</TableCell>
<TableCell className="text-muted-foreground">
{rule.durationSeconds != null ? `${rule.durationSeconds}s` : "—"}
{BINARY_METRICS.has(rule.metric) || rule.durationSeconds == null ? "—" : `${rule.durationSeconds}s`}
</TableCell>
<TableCell>
{rule.pipeline ? (
{GLOBAL_METRICS.has(rule.metric) ? (
<span className="text-muted-foreground">—</span>
) : rule.pipeline ? (
<Badge variant="outline">{rule.pipeline.name}</Badge>
) : (
<span className="text-muted-foreground">All</span>
Expand Down Expand Up @@ -429,31 +446,6 @@ function AlertRulesSection({ environmentId }: { environmentId: string }) {

{!editingRuleId && (
<>
<div className="space-y-2">
<Label htmlFor="rule-pipeline">Pipeline (optional)</Label>
<Select
value={form.pipelineId}
onValueChange={(v) =>
setForm((f) => ({
...f,
pipelineId: v === "__none__" ? "" : v,
}))
}
>
<SelectTrigger id="rule-pipeline">
<SelectValue placeholder="All pipelines" />
</SelectTrigger>
<SelectContent>
<SelectItem value="__none__">All pipelines</SelectItem>
{pipelines.map((p) => (
<SelectItem key={p.id} value={p.id}>
{p.name}
</SelectItem>
))}
</SelectContent>
</Select>
</div>

<div className="space-y-2">
<Label htmlFor="rule-metric">Metric</Label>
<Select
Expand All @@ -468,6 +460,7 @@ function AlertRulesSection({ environmentId }: { environmentId: string }) {
: isEventMetric(v)
? { threshold: "", durationSeconds: "" }
: {}),
...(GLOBAL_METRICS.has(v) ? { pipelineId: "" } : {}),
}))
}
>
Expand Down Expand Up @@ -502,29 +495,53 @@ function AlertRulesSection({ environmentId }: { environmentId: string }) {
</Select>
</div>

{!GLOBAL_METRICS.has(form.metric) && (
<div className="space-y-2">
<Label htmlFor="rule-pipeline">Pipeline (optional)</Label>
<Select
value={form.pipelineId}
onValueChange={(v) =>
setForm((f) => ({
...f,
pipelineId: v === "__none__" ? "" : v,
}))
}
>
<SelectTrigger id="rule-pipeline">
<SelectValue placeholder="All pipelines" />
</SelectTrigger>
<SelectContent>
<SelectItem value="__none__">All pipelines</SelectItem>
{pipelines.map((p) => (
<SelectItem key={p.id} value={p.id}>
{p.name}
</SelectItem>
))}
</SelectContent>
</Select>
</div>
)}
</>
)}

{isEventMetric(form.metric) ? (
{isEventMetric(form.metric) || BINARY_METRICS.has(form.metric) ? (
<p className="text-sm text-muted-foreground py-2">
Notifications will be sent when this event occurs.
</p>
) : (
<>
{!BINARY_METRICS.has(form.metric) && (
<div className="space-y-2">
<Label htmlFor="rule-threshold">Threshold</Label>
<Input
id="rule-threshold"
type="number"
placeholder="80"
value={form.threshold}
onChange={(e) =>
setForm((f) => ({ ...f, threshold: e.target.value }))
}
/>
</div>
)}
<div className="space-y-2">
<Label htmlFor="rule-threshold">Threshold</Label>
<Input
id="rule-threshold"
type="number"
placeholder="80"
value={form.threshold}
onChange={(e) =>
setForm((f) => ({ ...f, threshold: e.target.value }))
}
/>
</div>

<div className="space-y-2">
<Label htmlFor="rule-duration">Duration (seconds)</Label>
Expand Down
3 changes: 0 additions & 3 deletions src/app/(dashboard)/settings/_components/auth-settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ import {
PopoverContent,
PopoverTrigger,
} from "@/components/ui/popover";
import { ScimSettings } from "./scim-settings";

// ─── Auth Tab ──────────────────────────────────────────────────────────────────

Expand Down Expand Up @@ -603,8 +602,6 @@ export function AuthSettings() {
</form>
</CardContent>
</Card>
<Separator className="my-8" />
<ScimSettings />
</div>
);
}
7 changes: 6 additions & 1 deletion src/app/(dashboard)/settings/_components/team-settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,12 @@ export function TeamSettings() {
})
);

const settingsQuery = useQuery(trpc.settings.get.queryOptions());
// settings.get requires super-admin — silently degrade for team admins
const settingsQuery = useQuery({
...trpc.settings.get.queryOptions(),
retry: false,
throwOnError: false,
});
const oidcConfigured = !!(settingsQuery.data?.oidcIssuer && settingsQuery.data?.oidcClientId);

const [resetPasswordOpen, setResetPasswordOpen] = useState(false);
Expand Down
7 changes: 7 additions & 0 deletions src/components/error-boundary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ export class ErrorBoundary extends Component<Props, State> {
return { hasError: true, error };
}

componentDidUpdate(prevProps: Props) {
// Reset error state when children change (e.g. route navigation)
if (this.state.hasError && prevProps.children !== this.props.children) {
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 9, 2026

Choose a reason for hiding this comment

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

Children comparison triggers on every parent re-render, not just navigation

prevProps.children !== this.props.children compares JSX element object identity. Because JSX creates a new object on every render, this condition will be true on any parent re-render — not only on route navigation.

If a parent component re-renders while the error fallback is visible (e.g. a polling query fires, or any context update occurs above the boundary), the sequence becomes:

  1. Error thrown → fallback shown
  2. Parent re-renders (new children object reference) → componentDidUpdate resets error state
  3. Crashed children re-render → same error thrown again → getDerivedStateFromError sets hasError: true
  4. Back to showing the fallback

This produces a visible flash/retry loop whenever a parent re-renders during an error state.

The canonical fix is to drive resets via a stable identity such as the current route pathname. The simplest approach is for callers to pass the current route as a key to the boundary — when the key changes on navigation, React unmounts and remounts the component, clearing error state without any componentDidUpdate logic. Alternatively, accept an explicit resetKey prop and compare that instead of children.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/components/error-boundary.tsx
Line: 29

Comment:
**Children comparison triggers on every parent re-render, not just navigation**

`prevProps.children !== this.props.children` compares JSX element object identity. Because JSX creates a **new object on every render**, this condition will be `true` on any parent re-render — not only on route navigation. 

If a parent component re-renders while the error fallback is visible (e.g. a polling query fires, or any context update occurs above the boundary), the sequence becomes:

1. Error thrown → fallback shown
2. Parent re-renders (new children object reference) → `componentDidUpdate` resets error state
3. Crashed children re-render → same error thrown again → `getDerivedStateFromError` sets `hasError: true`
4. Back to showing the fallback

This produces a visible flash/retry loop whenever a parent re-renders during an error state.

The canonical fix is to drive resets via a stable identity such as the current route pathname. The simplest approach is for callers to pass the current route as a `key` to the boundary — when the key changes on navigation, React unmounts and remounts the component, clearing error state without any `componentDidUpdate` logic. Alternatively, accept an explicit `resetKey` prop and compare that instead of `children`.

How can I resolve this? If you propose a fix, please make it concise.

this.setState({ hasError: false, error: undefined });
}
}

render() {
if (this.state.hasError) {
return (
Expand Down
Loading