fix: Add Social Network modal with URL validation#13
Merged
nathanialhenniges merged 2 commits intomainfrom Apr 15, 2026
Merged
Conversation
Replace broken handleActivateNetwork (set isActive with empty URL) with modal flow that requires valid URL before activating — fixes silent failure chain through activeNetworks filter, socialDirty gate, and handleSaveSocial skip. Also removes 20-network hard cap and fixes touch-invisible remove button. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe social tab component was refactored to implement a modal-based flow for adding social networks. Users now enter a URL via dialog before activation, with validation for HTTP/HTTPS format. The 20-network limit was removed, and button styling was updated for improved accessibility and interaction states. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/components/admin/builder/social-tab.tsx (2)
396-413: Consider extracting the IIFE to a variable for cleaner JSX.The immediately invoked function expression inside JSX works but is harder to read. Extract it to a
constbefore the return for clarity.♻️ Proposed refactor
+ const brandIcon = pendingBrand && (() => { + const { bg, fg } = getAdminThemeColors(resolvedTheme); + const fill = getAccessibleIconFill(pendingBrand.hex, bg, fg); + const needsRing = isLowLuminance(pendingBrand.hex); + return ( + <div + className={cn( + "flex h-8 w-8 shrink-0 items-center justify-center rounded-full", + needsRing && "ring-1 ring-border dark:ring-white/20", + )} + style={{ backgroundColor: `${pendingBrand.hex}20` }} + > + <svg viewBox="0 0 24 24" className="h-4 w-4" aria-hidden="true"> + <path d={pendingBrand.svgPath} fill={fill} /> + </svg> + </div> + ); + })(); // Then in JSX: <DialogTitle className="flex items-center gap-3"> - {pendingBrand && (() => { - const { bg, fg } = getAdminThemeColors(resolvedTheme); - const fill = getAccessibleIconFill(pendingBrand.hex, bg, fg); - const needsRing = isLowLuminance(pendingBrand.hex); - return ( - <div - className={cn( - "flex h-8 w-8 shrink-0 items-center justify-center rounded-full", - needsRing && "ring-1 ring-border dark:ring-white/20", - )} - style={{ backgroundColor: `${pendingBrand.hex}20` }} - > - <svg viewBox="0 0 24 24" className="h-4 w-4" aria-hidden="true"> - <path d={pendingBrand.svgPath} fill={fill} /> - </svg> - </div> - ); - })()} + {brandIcon} <span>Add {pendingBrand?.name ?? "Network"}</span> </DialogTitle>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/admin/builder/social-tab.tsx` around lines 396 - 413, The JSX contains an IIFE that renders the pending brand icon; extract that block into a const (e.g., pendingBrandIcon) just above the return so the JSX becomes {pendingBrandIcon} for readability. Compute the const using the same symbols: check pendingBrand, call getAdminThemeColors(resolvedTheme), getAccessibleIconFill(pendingBrand.hex, bg, fg), isLowLuminance(pendingBrand.hex), and cn to build the className and style, and return the same <div>…<svg> structure; ensure the const is null/undefined when pendingBrand is falsy so JSX renders nothing.
431-446: Link the error message to the input for screen reader accessibility.The input has
aria-invalidbut the error message isn't programmatically associated with it. Add anidto the error and reference it viaaria-describedbyfor better screen reader support.♿ Proposed accessibility improvement
<Input id="add-network-url" type="url" autoFocus value={pendingUrl} onChange={(e) => setPendingUrl(e.target.value)} placeholder={ pendingSlug ? URL_PLACEHOLDERS[pendingSlug] ?? "https://" : "https://" } aria-invalid={pendingUrl.length > 0 && !pendingUrlValid} + aria-describedby={pendingUrl.length > 0 && !pendingUrlValid ? "add-network-url-error" : undefined} /> {pendingUrl.length > 0 && !pendingUrlValid && ( - <p className="text-xs text-destructive"> + <p id="add-network-url-error" className="text-xs text-destructive"> Enter a valid URL starting with http:// or https:// </p> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/admin/builder/social-tab.tsx` around lines 431 - 446, The error text under the Input is not associated with the input for screen readers: give the error element a stable id (e.g. "add-network-url-error") and update the Input to set aria-describedby to that id only when the error is shown (i.e., when pendingUrl.length > 0 && !pendingUrlValid); keep aria-invalid as-is and ensure aria-describedby is omitted or null when there is no error so screen readers don't reference a non-existent element. Use the existing Input component and the pendingUrl/pendingUrlValid state to control this.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/components/admin/builder/social-tab.tsx`:
- Around line 396-413: The JSX contains an IIFE that renders the pending brand
icon; extract that block into a const (e.g., pendingBrandIcon) just above the
return so the JSX becomes {pendingBrandIcon} for readability. Compute the const
using the same symbols: check pendingBrand, call
getAdminThemeColors(resolvedTheme), getAccessibleIconFill(pendingBrand.hex, bg,
fg), isLowLuminance(pendingBrand.hex), and cn to build the className and style,
and return the same <div>…<svg> structure; ensure the const is null/undefined
when pendingBrand is falsy so JSX renders nothing.
- Around line 431-446: The error text under the Input is not associated with the
input for screen readers: give the error element a stable id (e.g.
"add-network-url-error") and update the Input to set aria-describedby to that id
only when the error is shown (i.e., when pendingUrl.length > 0 &&
!pendingUrlValid); keep aria-invalid as-is and ensure aria-describedby is
omitted or null when there is no error so screen readers don't reference a
non-existent element. Use the existing Input component and the
pendingUrl/pendingUrlValid state to control this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6fa80ea-617e-4f2a-9130-47e05ef0964c
📒 Files selected for processing (1)
apps/web/src/components/admin/builder/social-tab.tsx
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.
Summary
handleActivateNetwork(setisActive:truewith empty URL) with a modal flow that requires a valid URL before activating — fixes 4-layer silent failure chain:activeNetworksfilter,socialDirtygate,handleSaveSocialskip, and D1url NOT NULLconstrainthidden group-hover:flex, use always-visibleopacity-60with hover/focus rampRoot cause
handleActivateNetworkonly setisActive: true— URL stayed empty. Every downstream check required a truthy URL, so the network silently disappeared from both lists after click.Test plan
bun run check-typespasses🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes