fix: eliminate setState-in-effect anti-patterns (Category D, issue #200)#256
Open
R-Hart80 wants to merge 4 commits into
Open
fix: eliminate setState-in-effect anti-patterns (Category D, issue #200)#256R-Hart80 wants to merge 4 commits into
R-Hart80 wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 58 minutes and 37 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe PR extends search functionality in the filtered-tree hook to mark ancestor nodes matching the query by label (case-insensitive), integrates that query through editor layouts, adds test coverage, and refactors state initialization patterns across several components to use derived values or lazy initializers instead of effects. ChangesFeature and refactor updates
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/hooks/useFilteredTree.ts (1)
112-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
searchQueryto the useEffect dependency array.The effect uses
searchQueryat line 97 (mergePathsIntoTree(ancestorPaths, searchQuery ?? "")), but it is not included in the dependency array at line 112. IfsearchQuerychanges withoutsearchResultschanging, the effect won't rerun and label-based highlighting will become stale.Fix
- }, [searchResults, projectId, accessToken, branch]); + }, [searchResults, projectId, accessToken, branch, searchQuery]);🤖 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 `@lib/hooks/useFilteredTree.ts` at line 112, The useEffect in useFilteredTree.ts is missing searchQuery in its dependency array causing stale highlights when searchQuery changes; update the dependency array (currently [searchResults, projectId, accessToken, branch]) to include searchQuery so the effect that calls mergePathsIntoTree(ancestorPaths, searchQuery ?? "") reruns on searchQuery updates.
🧹 Nitpick comments (1)
app/projects/[id]/settings/page.tsx (1)
2985-2989: 💤 Low valueConsider relocating the informational message before the read-only fields.
The explanatory text "Repository fields are managed by the GitHub integration" appears after all four fields. Users will encounter the dimmed, read-only fields before seeing this explanation.
For optimal UX, consider moving this message to appear immediately before the first read-only field (Repository owner) or as a banner at the top of the grid section, so users understand the constraint before attempting to interact with the fields.
💡 Suggested repositioning
Move the informational message before the repository fields grid:
Enable remote tracking </span> </label> + {isWebhookDriven && ( + <p className="text-xs text-indigo-500 dark:text-indigo-400"> + Repository fields are managed by the GitHub integration. + </p> + )} + {/* Repository */} <div className="grid grid-cols-2 gap-3"> <div> <label className={cn( "mb-1 block text-sm font-medium text-slate-700 dark:text-slate-300", isWebhookDriven && "opacity-60" )}> Repository owner </label> ... </div> </div> ... - {isWebhookDriven && ( - <p className="text-xs text-indigo-500 dark:text-indigo-400"> - Repository fields are managed by the GitHub integration. - </p> - )}🤖 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 2985 - 2989, Move the explanatory text so users see it before encountering read-only fields: when isWebhookDriven is true, render the paragraph with "Repository fields are managed by the GitHub integration" immediately before the repository fields grid (i.e., before the JSX that renders the "Repository owner" input and the other three repository fields) or as a banner at the top of that grid section; update the conditional that uses isWebhookDriven to wrap/precede the repository fields container instead of appearing after the four fields.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@app/projects/`[id]/settings/page.tsx:
- Around line 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.
---
Outside diff comments:
In `@lib/hooks/useFilteredTree.ts`:
- Line 112: The useEffect in useFilteredTree.ts is missing searchQuery in its
dependency array causing stale highlights when searchQuery changes; update the
dependency array (currently [searchResults, projectId, accessToken, branch]) to
include searchQuery so the effect that calls mergePathsIntoTree(ancestorPaths,
searchQuery ?? "") reruns on searchQuery updates.
---
Nitpick comments:
In `@app/projects/`[id]/settings/page.tsx:
- Around line 2985-2989: Move the explanatory text so users see it before
encountering read-only fields: when isWebhookDriven is true, render the
paragraph with "Repository fields are managed by the GitHub integration"
immediately before the repository fields grid (i.e., before the JSX that renders
the "Repository owner" input and the other three repository fields) or as a
banner at the top of that grid section; update the conditional that uses
isWebhookDriven to wrap/precede the repository fields container instead of
appearing after the four fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e34d7c7-e076-41fb-a54a-231fc8dde8b6
📒 Files selected for processing (9)
__tests__/lib/hooks/useFilteredTree.test.tsapp/auth/error/page.tsxapp/page.tsxapp/projects/[id]/settings/page.tsxapp/settings/page.tsxcomponents/editor/developer/DeveloperEditorLayout.tsxcomponents/editor/shared/EntityTreeToolbar.tsxcomponents/editor/standard/StandardEditorLayout.tsxlib/hooks/useFilteredTree.ts
Comment on lines
+2886
to
+2889
| <label className={cn( | ||
| "mb-1 block text-sm font-medium text-slate-700 dark:text-slate-300", | ||
| isWebhookDriven && "opacity-60" | ||
| )}> |
There was a problem hiding this comment.
🧩 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'}")
EOFRepository: CatholicOS/ontokit-web
Length of output: 232
🏁 Script executed:
sed -n '2880,2895p' app/projects/[id]/settings/page.tsx | head -20Repository: 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'}")
EOFRepository: 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-600for better contrast while maintaining the dimmed visual - Increasing opacity (e.g.,
opacity-70oropacity-75) - Applying
opacity-60only 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.
When a project is webhook-driven, the four GitHub integration input
fields (repo owner, repo name, branch, file path) were already marked
readOnly and visually styled as disabled on the inputs themselves.
This commit extends the visual treatment to their labels by applying
opacity-60 when isWebhookDriven is true, and moves the explanatory
note ("Repository fields are managed by the GitHub integration.")
to appear after all four fields so it clearly covers the whole group.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mergePathsIntoTree now accepts a query string and marks any node isSearchMatch when its label contains the query (case-insensitive), not only the leaf IRIs returned by the backend. Plumbed searchQuery from useTreeSearch through UseFilteredTreeOptions into the merge. Four new tests cover: ancestor highlight, case-insensitivity, empty query no-op, and non-matching ancestor stays unhighlighted. Fixes CatholicOS#209. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tholicOS#200) Replace four direct setState calls inside useEffect bodies with lazy initialisers and derived state: - EntityTreeToolbar: useState lazy init reads localStorage instead of setting state in a mount effect; removes the useEffect entirely. - app/auth/error: move auto-retry call into a setTimeout callback so it is asynchronous; removes the redundant setRetryCount that duplicated what retry() already does. - app/settings: useState lazy init reads window.location.hash so the highlight value is available on the first render without a setState-in-effect. - app/page: replace authDefaultApplied ref + useEffect with a derived filter (null-coalesce on userFilter) so authenticated users default to "mine" without an extra render cycle. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Missing dep caused stale tree highlights when the search query changed but searchResults did not (e.g. same result set, different query string). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
R-Hart80
force-pushed
the
fix/setState-in-effect-category-d
branch
from
157b859 to
4efc369
Compare
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.
Fixes part of #200 — Category D: direct
setStatecalls insideuseEffectbodies that can be replaced with lazy initialisers or derived state.Changes
components/editor/shared/EntityTreeToolbar.tsxuseState(false)+ mount effect readslocalStorageand callssetShowTip(true)useStatelazy initialiser readslocalStoragesynchronously;useEffectremoved entirelyapp/auth/error/page.tsxsetRetryCount((c) => c + 1)+retry()called directly in effect body when countdown hits 0setRetryCountremoved;retry()wrapped insetTimeoutcallback so it runs asynchronously (same pattern as the existingsetCountdowntick)app/settings/page.tsxuseState(null)+ mount effect readswindow.location.hashand callssetHighlightedSetting(hash)useStatelazy initialiser reads hash synchronously; effect now only handles scroll + timer to clearapp/page.tsxuseRef(false)+useEffectsetsfilter("mine")once when session resolvesuserFilterstate starts asnull;filteris derived via null-coalesce (userFilter ?? (authenticated ? "mine" : "public")); removes ref and effect entirelyVerification
npx tsc --noEmit— only pre-existingLanguagePicker.tsxerrors (missingcmdk)npm run test:coverage -- --run— 2737 tests pass, 0 regressionsnpx eslinton the 4 changed files — 0 warnings (down from 4)react-hooks/set-state-in-effectwarnings in the codebase are pre-existing Category A/B/C sites (out of scope for this PR)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests