Skip to content

fix: eliminate setState-in-effect anti-patterns (Category D, issue #200) #256

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

Closed
R-Hart80 wants to merge 4 commits into from
Closed

fix: eliminate setState-in-effect anti-patterns (Category D, issue #200) #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d7ba03a
fix: dim labels and reposition note for webhook-driven readonly fields
R-Hart80 Apr 13, 2026
e6fdc28
fix: highlight ancestor nodes whose labels match the search query
R-Hart80 May 11, 2026
99689cd
fix: eliminate setState-in-effect anti-patterns (Category D, issue #200)
R-Hart80 May 14, 2026
4efc369
fix: add searchQuery to useFilteredTree effect dependency array
R-Hart80 May 23, 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
66 changes: 66 additions & 0 deletions __tests__/lib/hooks/useFilteredTree.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,72 @@ describe("mergePathsIntoTree", () => {
expect(tree).toHaveLength(0);
});

it("highlights an ancestor whose label matches the query, even though it is not a backend result", () => {
const paths: AncestorPath[] = [
{
matchIri: "urn:leaf",
matchLabel: "Document Collection Event",
ancestors: [
{ iri: "urn:root", label: "Event", child_count: 1 },
{ iri: "urn:ancestor", label: "Document Collection and Production Events", child_count: 1 },
],
},
];

const tree = mergePathsIntoTree(paths, "document");

// Root ("Event") does not match
expect(tree[0].isSearchMatch).toBeFalsy();
// Ancestor ("Document Collection and Production Events") matches the query
expect(tree[0].children[0].isSearchMatch).toBe(true);
// Leaf ("Document Collection Event") is a backend match AND label matches
expect(tree[0].children[0].children[0].isSearchMatch).toBe(true);
});

it("is case-insensitive when matching ancestor labels against the query", () => {
const paths: AncestorPath[] = [
{
matchIri: "urn:leaf",
matchLabel: "Leaf",
ancestors: [{ iri: "urn:ancestor", label: "Document Archive", child_count: 1 }],
},
];

const tree = mergePathsIntoTree(paths, "DOCUMENT");

expect(tree[0].isSearchMatch).toBe(true);
});

it("does not highlight ancestors when query is empty", () => {
const paths: AncestorPath[] = [
{
matchIri: "urn:leaf",
matchLabel: "Leaf",
ancestors: [{ iri: "urn:ancestor", label: "Some Ancestor", child_count: 1 }],
},
];

const tree = mergePathsIntoTree(paths, "");

expect(tree[0].isSearchMatch).toBeFalsy();
expect(tree[0].children[0].isSearchMatch).toBe(true);
});

it("does not highlight ancestors whose labels do not contain the query", () => {
const paths: AncestorPath[] = [
{
matchIri: "urn:leaf",
matchLabel: "Document Event",
ancestors: [{ iri: "urn:ancestor", label: "Unrelated Category", child_count: 1 }],
},
];

const tree = mergePathsIntoTree(paths, "document");

expect(tree[0].isSearchMatch).toBeFalsy();
expect(tree[0].children[0].isSearchMatch).toBe(true);
});

it("assigns entityType 'class' to all nodes", () => {
const paths: AncestorPath[] = [
{
Expand Down
5 changes: 2 additions & 3 deletions app/auth/error/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,8 @@ function ErrorContent() {
useEffect(() => {
if (!isTransient || retryCount >= MAX_RETRIES || retrying) return;
if (countdown <= 0) {
setRetryCount((c) => c + 1);
retry();
return;
const id = setTimeout(() => retry(), 0);
return () => clearTimeout(id);
}
const timer = setTimeout(() => setCountdown((c) => c - 1), 1000);
return () => clearTimeout(timer);
Expand Down
13 changes: 3 additions & 10 deletions app/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,13 @@ type FilterType = "public" | "private" | "mine" | "all";

export default function HomePage() {
const { data: session, status } = useSession();
const [filter, setFilter] = useState<FilterType>("public");
const [userFilter, setUserFilter] = useState<FilterType | null>(null);
const [searchQuery, setSearchQuery] = useState("");
const [debouncedSearch, setDebouncedSearch] = useState("");

const isAuthenticated = status === "authenticated";

// Default authenticated users to "mine" tab
const authDefaultApplied = useRef(false);
useEffect(() => {
if (!authDefaultApplied.current && status !== "loading") {
authDefaultApplied.current = true;
if (isAuthenticated) setFilter("mine");
}
}, [status, isAuthenticated]);
const filter = userFilter ?? (status !== "loading" && isAuthenticated ? "mine" : "public");
const setFilter = useCallback((f: FilterType) => setUserFilter(f), []);

// Debounce search input
const debounceRef = useRef<ReturnType<typeof setTimeout> | null>(null);
Expand Down
32 changes: 22 additions & 10 deletions app/projects/[id]/settings/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2883,7 +2883,10 @@ function RemoteSyncSection({
{/* Repository */}
<div className="grid grid-cols-2 gap-3">
<div>
<label className="mb-1 block text-sm font-medium text-slate-700 dark:text-slate-300">
<label className={cn(
"mb-1 block text-sm font-medium text-slate-700 dark:text-slate-300",
isWebhookDriven && "opacity-60"
)}>
Comment on lines +2886 to +2889

@coderabbitai coderabbitai Bot May 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check WCAG contrast ratio for dimmed labels
# The script calculates the effective color after applying opacity-60 to slate-700

python3 << 'EOF'
def hex_to_rgb(hex_color):
    hex_color = hex_color.lstrip('#')
    return tuple(int(hex_color[i:i+2], 16) for i in (0, 2, 4))

def apply_opacity(fg_rgb, bg_rgb, opacity):
    """Blend foreground with background at given opacity"""
    return tuple(
        int(fg * opacity + bg * (1 - opacity))
        for fg, bg in zip(fg_rgb, bg_rgb)
    )

def relative_luminance(rgb):
    """Calculate relative luminance per WCAG formula"""
    r, g, b = [x / 255.0 for x in rgb]
    r = r / 12.92 if r <= 0.03928 else ((r + 0.055) / 1.055) ** 2.4
    g = g / 12.92 if g <= 0.03928 else ((g + 0.055) / 1.055) ** 2.4
    b = b / 12.92 if b <= 0.03928 else ((b + 0.055) / 1.055) ** 2.4
    return 0.2126 * r + 0.7152 * g + 0.0722 * b

def contrast_ratio(l1, l2):
    """Calculate contrast ratio between two luminance values"""
    lighter = max(l1, l2)
    darker = min(l1, l2)
    return (lighter + 0.05) / (darker + 0.05)

# Tailwind slate-700 (approximate)
slate_700 = (51, 65, 85)
white = (255, 255, 255)

# Apply 60% opacity
effective_color = apply_opacity(slate_700, white, 0.6)
print(f"Slate-700 at 60% opacity: rgb{effective_color}")

# Calculate contrast
lum_effective = relative_luminance(effective_color)
lum_white = relative_luminance(white)
ratio = contrast_ratio(lum_effective, lum_white)

print(f"Contrast ratio: {ratio:.2f}:1")
print(f"WCAG AA normal text (4.5:1): {'PASS' if ratio >= 4.5 else 'FAIL'}")
print(f"WCAG AA large text (3:1): {'PASS' if ratio >= 3.0 else 'FAIL'}")
print(f"WCAG AAA normal text (7:1): {'PASS' if ratio >= 7.0 else 'FAIL'}")
EOF

Repository: CatholicOS/ontokit-web

Length of output: 232

🏁 Script executed:

sed -n '2880,2895p' app/projects/[id]/settings/page.tsx | head -20

Repository: CatholicOS/ontokit-web

Length of output: 653

🏁 Script executed:

python3 << 'EOF'
# Check dark mode contrast as well
def apply_opacity(fg_rgb, bg_rgb, opacity):
    """Blend foreground with background at given opacity"""
    return tuple(
        int(fg * opacity + bg * (1 - opacity))
        for fg, bg in zip(fg_rgb, bg_rgb)
    )

def relative_luminance(rgb):
    """Calculate relative luminance per WCAG formula"""
    r, g, b = [x / 255.0 for x in rgb]
    r = r / 12.92 if r <= 0.03928 else ((r + 0.055) / 1.055) ** 2.4
    g = g / 12.92 if g <= 0.03928 else ((g + 0.055) / 1.055) ** 2.4
    b = b / 12.92 if b <= 0.03928 else ((b + 0.055) / 1.055) ** 2.4
    return 0.2126 * r + 0.7152 * g + 0.0722 * b

def contrast_ratio(l1, l2):
    """Calculate contrast ratio between two luminance values"""
    lighter = max(l1, l2)
    darker = min(l1, l2)
    return (lighter + 0.05) / (darker + 0.05)

# Dark mode: slate-300 on dark background (approximate dark background: `#1e293b` or rgb(30, 41, 59))
slate_300 = (203, 213, 225)
dark_bg = (30, 41, 59)

# Apply 60% opacity on dark background
effective_color_dark = apply_opacity(slate_300, dark_bg, 0.6)
print(f"Dark mode: Slate-300 at 60% opacity on dark background: rgb{effective_color_dark}")

# Calculate contrast
lum_effective_dark = relative_luminance(effective_color_dark)
lum_dark_bg = relative_luminance(dark_bg)
ratio_dark = contrast_ratio(lum_effective_dark, lum_dark_bg)

print(f"Dark mode contrast ratio: {ratio_dark:.2f}:1")
print(f"WCAG AA normal text (4.5:1): {'PASS' if ratio_dark >= 4.5 else 'FAIL'}")

# Also check if text-slate-500 would be better (light mode)
slate_500 = (100, 116, 139)
effective_500 = apply_opacity(slate_500, (255, 255, 255), 0.6)
print(f"\nLight mode: Slate-500 at 60% opacity: rgb{effective_500}")
lum_500 = relative_luminance(effective_500)
lum_white = relative_luminance((255, 255, 255))
ratio_500 = contrast_ratio(lum_500, lum_white)
print(f"Slate-500 contrast ratio: {ratio_500:.2f}:1")
print(f"WCAG AA normal text (4.5:1): {'PASS' if ratio_500 >= 4.5 else 'FAIL'}")
EOF

Repository: CatholicOS/ontokit-web

Length of output: 333

Address WCAG AA contrast issue in light mode for dimmed labels.

The opacity-60 styling on labels with text-slate-700 results in a 3.36:1 contrast ratio in light mode, which fails the WCAG AA requirement of 4.5:1 for normal text (the labels use text-sm). Dark mode with text-slate-300 passes at 4.51:1, so the issue is light-mode only.

The suggested alternative of using text-slate-500 would worsen the issue to 2.32:1. Instead, consider:

  • Using text-slate-600 for better contrast while maintaining the dimmed visual
  • Increasing opacity (e.g., opacity-70 or opacity-75)
  • Applying opacity-60 only in dark mode where it still passes

This affects the labels for Repository owner, Repository name, Branch, and File path (lines 2886–2889, 2907–2910, 2942–2945, 2963–2966).

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/projects/`[id]/settings/page.tsx around lines 2886 - 2889, The label
rendering using cn(...) with isWebhookDriven currently applies "opacity-60"
which drops the light-mode contrast below WCAG AA; update the class logic in the
label elements (the <label> usages that call cn with isWebhookDriven — e.g., the
Repository owner/name, Branch, and File path labels) so that when
isWebhookDriven is true you either use a higher-contrast text class (e.g.,
"text-slate-600") in light mode or increase opacity (e.g., "opacity-75"), or
apply "opacity-60" only for dark mode; implement this by changing the
conditional passed to cn(...) (reference the isWebhookDriven conditional in
page.tsx) to choose between "text-slate-600" or "opacity-75" for light mode and
keep "opacity-60" for dark mode via a dark: utility or two-branch conditional.

Repository owner
</label>
<input
Expand All @@ -2901,7 +2904,10 @@ function RemoteSyncSection({
/>
</div>
<div>
<label className="mb-1 block text-sm font-medium text-slate-700 dark:text-slate-300">
<label className={cn(
"mb-1 block text-sm font-medium text-slate-700 dark:text-slate-300",
isWebhookDriven && "opacity-60"
)}>
Repository name
</label>
<input
Expand All @@ -2920,12 +2926,6 @@ function RemoteSyncSection({
</div>
</div>

{isWebhookDriven && (
<p className="text-xs text-indigo-500 dark:text-indigo-400">
Repository fields are managed by the GitHub integration.
</p>
)}

{/* Same-repo info when editing form matches GitHub integration */}
{!isWebhookDriven && githubIntegration &&
repoOwner === githubIntegration.repo_owner &&
Expand All @@ -2939,7 +2939,10 @@ function RemoteSyncSection({
{/* Branch + File path */}
<div className="grid grid-cols-2 gap-3">
<div>
<label className="mb-1 block text-sm font-medium text-slate-700 dark:text-slate-300">
<label className={cn(
"mb-1 block text-sm font-medium text-slate-700 dark:text-slate-300",
isWebhookDriven && "opacity-60"
)}>
Branch
</label>
<input
Expand All @@ -2957,7 +2960,10 @@ function RemoteSyncSection({
/>
</div>
<div>
<label className="mb-1 block text-sm font-medium text-slate-700 dark:text-slate-300">
<label className={cn(
"mb-1 block text-sm font-medium text-slate-700 dark:text-slate-300",
isWebhookDriven && "opacity-60"
)}>
File path
</label>
<input
Expand All @@ -2976,6 +2982,12 @@ function RemoteSyncSection({
</div>
</div>

{isWebhookDriven && (
<p className="text-xs text-indigo-500 dark:text-indigo-400">
Repository fields are managed by the GitHub integration.
</p>
)}

{/* Frequency + Update mode */}
<div className="grid grid-cols-2 gap-3">
<div>
Expand Down
11 changes: 6 additions & 5 deletions app/settings/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,16 +330,17 @@ function EditorPreferencesSection() {
const setTheme = useEditorModeStore((s) => s.setTheme);
const preferEditMode = useEditorModeStore((s) => s.preferEditMode);
const setPreferEditMode = useEditorModeStore((s) => s.setPreferEditMode);
const [highlightedSetting, setHighlightedSetting] = useState<string | null>(null);
const [highlightedSetting, setHighlightedSetting] = useState<string | null>(() => {
if (typeof window === "undefined") return null;
return window.location.hash.slice(1) || null;
});

// Highlight and scroll to the setting referenced by the URL hash
// Scroll to the setting referenced by the URL hash and clear the highlight after 2 s
useEffect(() => {
const hash = window.location.hash.slice(1);
if (!hash) return;
const el = document.getElementById(hash);
if (!el) return;
el.scrollIntoView({ behavior: "smooth", block: "center" });
setHighlightedSetting(hash);
if (el) el.scrollIntoView({ behavior: "smooth", block: "center" });
const timer = setTimeout(() => setHighlightedSetting(null), 2000);
return () => clearTimeout(timer);
}, []);
Expand Down
1 change: 1 addition & 0 deletions components/editor/developer/DeveloperEditorLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ export function DeveloperEditorLayout(props: DeveloperEditorLayoutProps) {
projectId,
accessToken,
branch: activeBranch,
searchQuery,
});

const handleSearchSelect = (iri: string) => {
Expand Down
14 changes: 5 additions & 9 deletions components/editor/shared/EntityTreeToolbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"use client";

import { useRef, useCallback, useState, useEffect } from "react";
import { useRef, useCallback, useState } from "react";
import { Search, X, Plus, ChevronDown, ChevronsDown, ChevronRight, ChevronsRight, Loader2 } from "lucide-react";
import { cn } from "@/lib/utils";

Expand Down Expand Up @@ -44,17 +44,13 @@ export function EntityTreeToolbar({
const internalRef = useRef<HTMLInputElement>(null);
const inputRef = searchInputRef || internalRef;

// Dismissible tip
const [showTip, setShowTip] = useState(false);
useEffect(() => {
const [showTip, setShowTip] = useState(() => {
try {
if (!localStorage.getItem(TIP_DISMISSED_KEY)) {
setShowTip(true);
}
return !localStorage.getItem(TIP_DISMISSED_KEY);
} catch {
// localStorage unavailable
return false;
}
}, []);
});

const dismissTip = useCallback(() => {
setShowTip(false);
Expand Down
1 change: 1 addition & 0 deletions components/editor/standard/StandardEditorLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,7 @@ export function StandardEditorLayout(props: StandardEditorLayoutProps) {
projectId,
accessToken,
branch: activeBranch,
searchQuery,
});

const handleSearchSelect = (iri: string) => {
Expand Down
16 changes: 11 additions & 5 deletions lib/hooks/useFilteredTree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ interface UseFilteredTreeOptions {
projectId: string;
accessToken?: string;
branch?: string;
searchQuery?: string;
}

interface UseFilteredTreeReturn {
Expand All @@ -29,6 +30,7 @@ export function useFilteredTree({
projectId,
accessToken,
branch,
searchQuery,
}: UseFilteredTreeOptions): UseFilteredTreeReturn {
const [filteredNodes, setFilteredNodes] = useState<EntityTreeNode[] | null>(null);
const [isBuilding, setIsBuilding] = useState(false);
Expand Down Expand Up @@ -92,7 +94,7 @@ export function useFilteredTree({
if (buildId !== buildIdRef.current) return;

// Merge all ancestor paths into a unified tree
const tree = mergePathsIntoTree(ancestorPaths);
const tree = mergePathsIntoTree(ancestorPaths, searchQuery ?? "");

setFilteredNodes(tree);
setFirstMatchIri(limitedResults[0]?.iri ?? null);
Expand All @@ -107,7 +109,7 @@ export function useFilteredTree({
}
}
})();
}, [searchResults, projectId, accessToken, branch]);
}, [searchResults, searchQuery, projectId, accessToken, branch]);

return { filteredNodes, isBuilding, firstMatchIri, truncated };
}
Expand All @@ -121,8 +123,11 @@ export interface AncestorPath {
/**
* Merge multiple ancestor paths into a unified EntityTreeNode tree.
* Matched nodes get `isSearchMatch: true`, all ancestors are `isExpanded: true`.
* If `query` is provided, any node whose label contains the query (case-insensitive)
* is also marked as a match — not just the leaf IRIs the backend returned.
*/
export function mergePathsIntoTree(paths: AncestorPath[]): EntityTreeNode[] {
export function mergePathsIntoTree(paths: AncestorPath[], query = ""): EntityTreeNode[] {
const q = query.trim().toLowerCase();
// nodeMap: iri -> EntityTreeNode
const nodeMap = new Map<string, EntityTreeNode>();
// childrenMap: parentIri -> Set<childIri>
Expand All @@ -143,6 +148,7 @@ export function mergePathsIntoTree(paths: AncestorPath[]): EntityTreeNode[] {

for (let i = 0; i < fullPath.length; i++) {
const item = fullPath[i];
const labelMatches = q.length > 0 && item.label.toLowerCase().includes(q);

if (!nodeMap.has(item.iri)) {
nodeMap.set(item.iri, {
Expand All @@ -153,12 +159,12 @@ export function mergePathsIntoTree(paths: AncestorPath[]): EntityTreeNode[] {
isLoading: false,
hasChildren: item.hasChildren,
entityType: "class",
isSearchMatch: matchIris.has(item.iri),
isSearchMatch: matchIris.has(item.iri) || labelMatches,
});
} else {
const existing = nodeMap.get(item.iri)!;
existing.hasChildren = existing.hasChildren || item.hasChildren;
if (matchIris.has(item.iri)) {
if (matchIris.has(item.iri) || labelMatches) {
existing.isSearchMatch = true;
}
}
Expand Down
Loading