Larger updates, fixes and changes to the a/b testing #2980
Conversation
Will add this.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
WalkthroughThe PR switches A/B winner selection to use analytics aggregated by normalized URLs and a deterministic tie-breaking cascade (conversions → conversion rate → clicks, fallback to leads → lead rate → clicks). Multiple UI forms and modals now normalize URLs on blur, display normalized analytics, and surface test completion dates. Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger as complete-ab-tests
participant Norm as Normalize URLs
participant Aggregate as Aggregate Metrics
participant Decide as Tie-Breaking Cascade
participant Persist as Persist Winner / No-Op
Trigger->>Norm: Read testVariants, normalize each URL
Norm->>Aggregate: Group metrics by normalized URL (clicks, leads, sales, saleAmount)
Aggregate->>Decide: Compute per-URL conversions & rates
rect rgb(200,220,255)
Note over Decide: Primary path (conversions)
Decide->>Decide: Sort by conversions DESC
Decide->>Decide: Tie → conversion rate DESC
Decide->>Decide: Tie → clicks DESC
end
rect rgb(240,230,200)
Note over Decide: Fallback (no conversions)
Decide->>Decide: Sort by leads DESC
Decide->>Decide: Tie → lead rate DESC
Decide->>Decide: Tie → clicks DESC
end
Decide->>Persist: Single deterministic winner?
alt Winner found
Persist->>Persist: Ensure winner URL ≠ original, mark winner
else No deterministic winner
Persist->>Persist: Log no-deterministic-winner, take no action
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
Changed back to "highest performing" to cover both leads and conversions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx (1)
51-67: Complete the AB testing modal refactor—migrate all imports to the nested path or remove the duplicate.The two ab-testing-modal implementations are diverging: the new nested version (
apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx) has improved component composition (ABTestingModal wrapper + ABTestingModalInner), but all three active consumers still import from the old path (@/ui/modals/link-builder/ab-testing-modal), leaving the new version unused. This suggests an incomplete refactor.Recommended action:
- Migrate imports in
link-feature-buttons.tsx,more-dropdown.tsx, andconstants.tsto the new nested path, or- Remove the new nested file and continue using the old implementation.
Leaving both in place risks maintaining two diverging codebases.
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (1)
230-239: Stale prop risk: include onEndTest in useCallback deps.Without it, updated callbacks won’t propagate.
- }, [showEndABTestingModal, setShowEndABTestingModal]); + }, [showEndABTestingModal, setShowEndABTestingModal, onEndTest]);
🧹 Nitpick comments (16)
apps/web/ui/links/destination-url-input.tsx (1)
115-128: Great: https auto-append + first-variant sync. Two small tweaks.
- Comment says “set the https:// prefix” but code only trims a trailing slash. Adjust comment for accuracy.
- Trim multiple trailing slashes to avoid “//” artifacts.
- // remove trailing slash and set the https:// prefix - const normalizedUrl = url.replace(/\/$/, ""); + // append scheme if missing (handled by getUrlFromString) and remove trailing slashes + const normalizedUrl = url.replace(/\/+$/, "");Optional: only set testVariants.0.url when it actually changes to avoid unnecessary dirtiness.
- if (Array.isArray(testVariants) && testVariants.length > 0) { + if (Array.isArray(testVariants) && testVariants.length > 0) { + const current0 = formContext.getValues("testVariants.0.url" as any); + if (current0 !== normalizedUrl) { formContext.setValue( "testVariants.0.url" as any, normalizedUrl, { shouldDirty: true }, ); + } }apps/web/ui/links/link-tests.tsx (3)
57-92: Solid normalization-based aggregation; confirm intended grouping semantics.Using normalizeUrl collapses protocol/query/hash differences. This merges http/https and strips UTM parameters when aggregating. If that’s the desired behavior for A/B test analytics, this is perfect; otherwise, consider a custom key that preserves protocol or selected query keys.
Would you like me to draft a small helper like normalizeUrlForABTestKey(hostname + pathname only) and wire it here and in complete-ab-tests.ts for consistency?
104-106: Lookup key consistency.normalizeUrl(test.url) matches the map’s key generation above—good. If you ever change the key function, extract it to a shared util to keep UI and server logic in lockstep.
146-151: UI nit.Percentage is rounded; if you later allow fractional splits in the slider, consider toFixed(1) for display while keeping integers in storage.
apps/web/lib/api/links/complete-ab-tests.ts (3)
35-59: Aggregation matches UI logic; be explicit about protocol/query collapsing.normalizeUrl merges http/https and drops queries and fragments. That’s consistent with link-tests.tsx but changes attribution if variants differ only by UTM. If this is intentional, add a brief comment to document the decision.
- // Aggregate analytics by normalized URL for stable matching with variants + // Aggregate by normalized URL (hostname + pathname). Intentionally collapses protocol and query/hash + // so http/https and UTM variants are grouped under the same destination.
79-114: Deterministic tie-breakers look good; consider extracting comparator + optional saleAmount tie.Logic is correct and readable. Two small improvements:
- Extract a compareVariants(a,b) to reduce branching and make this unit-testable.
- Optional: when conversions tie and rates tie, consider saleAmount as an extra tie-breaker before clicks.
I can submit a follow-up refactor + unit tests for these paths if you want.
Also applies to: 115-144
146-154: Non-deterministic case handling: log + early return.Good call to avoid random selection. Prefer structured logging (with link.id, projectId, window) over console.log for observability.
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (2)
299-359: URL onBlur normalization is consistent; small robustness tweak.
- Use //+$/ to trim multiple trailing slashes.
- Consider updating parent destination URL immediately when editing index 0 (today it updates on Save) if you want truly live two-way sync.
- const normalizedUrl = url.replace(/\/$/, ""); + const normalizedUrl = url.replace(/\/+$/, "");If live sync is desired:
+ if (index === 0) { + setValueParent("url", normalizedUrl, { shouldDirty: true }); + }
465-474: 6‑week visual warning works; edge of day rounding.differenceInDays truncates; near the 6‑week boundary, users might see non-red until the next day. Acceptable, but flagging in case you want strict > 42 days using differenceInMilliseconds.
apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx (2)
342-401: Per-field onBlur normalization mirrors Destination input; unify slash trimming.Same suggestion: use //+$/ and optionally live-update parent url when index === 0 for immediate two-way sync.
- const normalizedUrl = url.replace(/\/$/, ""); + const normalizedUrl = url.replace(/\/+$/, "");
457-460: Completion date UX.
- Copy + anchor updates LGTM.
- Red text hint is correct; consider milliseconds for exact >6 weeks if precision matters.
Also applies to: 507-517
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (5)
76-97: SWR key: return null when disabled; avoid building the URL eagerly.Use a memoized key function that returns null until all dependencies are ready. Cleaner and prevents accidental fetches on falsy keys.
- const { data, error, isLoading } = useSWR< + const analyticsKey = useMemo(() => { + if (!(testVariants?.length && linkId && workspaceId)) return null; + const qs = new URLSearchParams({ + event: "composite", + groupBy: "top_urls", + linkId: linkId as string, + workspaceId: workspaceId!, + ...(testStartedAt && { + start: new Date(testStartedAt as Date).toISOString(), + }), + }).toString(); + return `/api/analytics?${qs}`; + }, [testVariants, linkId, workspaceId, testStartedAt]); + + const { data, error, isLoading } = useSWR< { url: string; clicks: number; leads: number; saleAmount: number; sales: number; }[] - >( - Boolean(testVariants && testVariants.length && linkId && workspaceId) && - `/api/analytics?${new URLSearchParams({ - event: "composite", - groupBy: "top_urls", - linkId: linkId as string, - workspaceId: workspaceId!, - ...(testStartedAt && { - start: new Date(testStartedAt as Date).toISOString(), - }), - }).toString()}`, - fetcher, - { revalidateOnFocus: false }, - ); + >(analyticsKey, fetcher, { revalidateOnFocus: false });
22-27: Make overflow detection responsive.Recompute on container resize; current effect runs only on URL change.
- useEffect(() => { - const element = textRef.current; - if (element) { - setIsOverflowing(element.scrollWidth > element.clientWidth); - } - }, [url]); + useEffect(() => { + const el = textRef.current; + if (!el) return; + const compute = () => + setIsOverflowing(el.scrollWidth > el.clientWidth); + compute(); + const ro = new ResizeObserver(compute); + ro.observe(el); + return () => ro.disconnect(); + }, [url]);
141-143: A11y: use radio semantics for the selection list.Declare the group as a radiogroup for screen readers.
- <div className="mt-4 flex flex-col gap-2"> + <div + className="mt-4 flex flex-col gap-2" + role="radiogroup" + aria-label="Select winning URL" + >
142-146: Avoid repeated getValues calls inside the map.Call once per render outside the loop.
- {testVariants?.map((test, index) => { - const normalized = normalizeUrl(test.url); - const analytics = analyticsByNormalizedUrl?.get(normalized); - const link = getValuesParent(); + {/* move this above the map in component body if preferred */} + {testVariants?.map((test, index) => { + const normalized = normalizeUrl(test.url); + const analytics = analyticsByNormalizedUrl?.get(normalized); + const link = getValuesParent();Alternatively, hoist:
@@ - return ( + const link = getValuesParent(); + return (…and remove the inner declaration at Line 145.
206-215: Reset selection on close/confirm for a clean next open.Prevents stale selection persisting across modal sessions.
onClick={() => { if (selectedUrl) { setValueParent("url", selectedUrl, { shouldDirty: true }); setValueParent("testCompletedAt", new Date(), { shouldDirty: true, }); + setSelectedUrl(null); setShowEndABTestingModal(false); onEndTest?.(); } }}Optionally also clear on hide:
@@ line 72 - const [selectedUrl, setSelectedUrl] = useState<string | null>(null); + const [selectedUrl, setSelectedUrl] = useState<string | null>(null); + useEffect(() => { + if (!showEndABTestingModal) setSelectedUrl(null); + }, [showEndABTestingModal]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/lib/api/links/complete-ab-tests.ts(2 hunks)apps/web/ui/links/destination-url-input.tsx(1 hunks)apps/web/ui/links/link-tests.tsx(5 hunks)apps/web/ui/links/tests-badge.tsx(2 hunks)apps/web/ui/modals/link-builder/ab-testing-modal.tsx(5 hunks)apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx(6 hunks)apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/web/lib/api/links/complete-ab-tests.ts (2)
apps/web/lib/analytics/get-analytics.ts (1)
getAnalytics(20-251)packages/utils/src/functions/urls.ts (1)
normalizeUrl(173-180)
apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx (2)
packages/utils/src/functions/urls.ts (2)
isValidUrl(1-8)getUrlFromString(10-18)apps/web/lib/zod/schemas/links.ts (1)
MAX_TEST_COUNT(30-30)
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (2)
packages/utils/src/functions/urls.ts (1)
normalizeUrl(173-180)apps/web/ui/links/link-analytics-badge.tsx (1)
LinkAnalyticsBadge(28-192)
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (2)
packages/utils/src/functions/urls.ts (2)
isValidUrl(1-8)getUrlFromString(10-18)apps/web/lib/zod/schemas/links.ts (1)
MAX_TEST_COUNT(30-30)
apps/web/ui/links/link-tests.tsx (1)
packages/utils/src/functions/urls.ts (1)
normalizeUrl(173-180)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
apps/web/ui/links/tests-badge.tsx (3)
4-4: LGTM!The
formatDateTimeimport is correctly added and used for formatting the test completion date.
34-34: LGTM! Note on overflow handling.The styling changes look appropriate. Removing
overflow-hiddenaligns with the PR's goal of improving overflow handling for long content. The increased padding (p-3) provides better spacing for the new centered text layout.
16-25: No issues found — formatDateTime already handles invalid dates.The review comment's concern is unfounded. The
formatDateTimefunction already includes a guard:if (datetime.toString() === "Invalid Date") return "";, so any invalid date string passed to it will safely return an empty string rather than cause unexpected behavior. The date formatting logic in tests-badge.tsx (lines 16-25) is correct and properly handles edge cases.Likely an incorrect or invalid review comment.
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (2)
389-389: Spacing/UI.mt-2 matches the reduced padding objective. LGTM.
414-417: Tooltip copy updated.Copy change aligns with new help doc. LGTM.
apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx (2)
330-331: Help link anchor update.Anchor update looks good.
279-283: Start timestamp.Setting testStartedAt on first save is a nice touch for analytics windows. LGTM.
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (1)
99-125: Confirm normalization semantics: query string is dropped.normalizeUrl keeps hostname+pathname only. If variants differ via query params (e.g., utm vs. content changes), analytics will be merged.
Is this intentional for winner selection and display? If not, consider preserving or selectively stripping queries (e.g., drop known tracking params only).
| {formattedDate ? ( | ||
| <div className="text-center"> | ||
| <p className="font-semibold text-neutral-900"> | ||
| A/B test is running | ||
| </p> | ||
| <p className="mt-1 text-neutral-600"> | ||
| Scheduled completion date is{" "} | ||
| <span className="font-medium text-neutral-800"> | ||
| {formattedDate} | ||
| </span> | ||
| </p> | ||
| </div> | ||
| ) : ( | ||
| <p className="text-center">A/B tests</p> | ||
| )} |
There was a problem hiding this comment.
Missing feature: 6-week completion date warning.
The conditional rendering correctly shows the test status and completion date. However, according to the PR objectives, "Completion date visual warning: if completion date is set beyond six weeks, the note turns red." This logic is not implemented.
Apply this diff to add the 6-week warning:
const formattedDate = completedAtDate
? formatDateTime(completedAtDate, {
month: "short",
day: "numeric",
year: "numeric",
})
: undefined;
+ const isBeyondSixWeeks = completedAtDate
+ ? completedAtDate.getTime() > Date.now() + 6 * 7 * 24 * 60 * 60 * 1000
+ : false;
return (
<div className="hidden sm:block">
<HoverCard.Root openDelay={100}>
<HoverCard.Portal>
<HoverCard.Content
side="bottom"
sideOffset={8}
className="animate-slide-up-fade z-[99] overflow-hidden rounded-xl border border-neutral-200 bg-white p-3 text-sm text-neutral-700 shadow-sm"
>
{formattedDate ? (
<div className="text-center">
<p className="font-semibold text-neutral-900">
A/B test is running
</p>
- <p className="mt-1 text-neutral-600">
+ <p className={cn("mt-1", isBeyondSixWeeks ? "text-red-600" : "text-neutral-600")}>
Scheduled completion date is{" "}
- <span className="font-medium text-neutral-800">
+ <span className={cn("font-medium", isBeyondSixWeeks ? "text-red-800" : "text-neutral-800")}>
{formattedDate}
</span>
</p>
</div>
) : (
<p className="text-center">A/B tests</p>
)}
</HoverCard.Content>Committable suggestion skipped: line range outside the PR's diff.
| <button | ||
| key={index} | ||
| onClick={() => setSelectedUrl(test.url)} | ||
| className={`relative flex w-full items-center justify-between rounded-lg border bg-white p-0 text-left ring-0 ring-black transition-all duration-100 hover:bg-neutral-50 ${ | ||
| selectedUrl === test.url | ||
| ? "border-black ring-1" | ||
| : "border-neutral-200" | ||
| }`} | ||
| > |
There was a problem hiding this comment.
Prevent unintended form submit; add type="button" and stable keys.
Inside forms, button defaults to submit. Clicks may submit the parent form. Also, prefer a stable key over the index.
Apply:
- <button
- key={index}
+ <button
+ key={test.url}
+ type="button"
+ role="radio"
+ aria-checked={selectedUrl === test.url}
onClick={() => setSelectedUrl(test.url)}
className={`relative flex w-full items-center justify-between rounded-lg border bg-white p-0 text-left ring-0 ring-black transition-all duration-100 hover:bg-neutral-50 ${
selectedUrl === test.url
? "border-black ring-1"
: "border-neutral-200"
}`}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| key={index} | |
| onClick={() => setSelectedUrl(test.url)} | |
| className={`relative flex w-full items-center justify-between rounded-lg border bg-white p-0 text-left ring-0 ring-black transition-all duration-100 hover:bg-neutral-50 ${ | |
| selectedUrl === test.url | |
| ? "border-black ring-1" | |
| : "border-neutral-200" | |
| }`} | |
| > | |
| <button | |
| key={test.url} | |
| type="button" | |
| role="radio" | |
| aria-checked={selectedUrl === test.url} | |
| onClick={() => setSelectedUrl(test.url)} | |
| className={`relative flex w-full items-center justify-between rounded-lg border bg-white p-0 text-left ring-0 ring-black transition-all duration-100 hover:bg-neutral-50 ${ | |
| selectedUrl === test.url | |
| ? "border-black ring-1" | |
| : "border-neutral-200" | |
| }`} | |
| > |
🤖 Prompt for AI Agents
In apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx around
lines 147-155, the button is missing an explicit type which causes it to act as
a submit inside a form and it uses the array index as the key which is unstable;
change the button to include type="button" to prevent form submission and
replace key={index} with a stable unique value (e.g., key={test.id} or
key={test.url}) to avoid reconciliation bugs.
| <LinkAnalyticsBadge | ||
| link={{ | ||
| ...(link as any), | ||
| clicks: analytics?.clicks ?? 0, | ||
| leads: analytics?.leads ?? 0, | ||
| sales: analytics?.sales ?? 0, | ||
| saleAmount: analytics?.saleAmount ?? 0, | ||
| }} | ||
| url={test.url} | ||
| sharingEnabled={false} |
There was a problem hiding this comment.
🧩 Analysis chain
Encode URL passed into analytics link (cross-file).
LinkAnalyticsBadge builds href with &url=${url} without encoding (apps/web/ui/links/link-analytics-badge.tsx). Raw ?/& in a URL breaks the querystring.
Proposed change in apps/web/ui/links/link-analytics-badge.tsx (reference snippet):
- href={`/${slug}/analytics?domain=${domain}&key=${key}${url ? `&url=${url}` : ""}&interval=${plan === "free" ? "30d" : plan === "pro" ? "1y" : "all"}`}
+ href={`/${slug}/analytics?domain=${domain}&key=${key}${url ? `&url=${encodeURIComponent(url)}` : ""}&interval=${plan === "free" ? "30d" : plan === "pro" ? "1y" : "all"}`}Quick grep to confirm other sites:
🏁 Script executed:
#!/bin/bash
rg -nP --type=tsx -C2 '\&url=\$\{(?!encodeURIComponent)' apps/web | sed -n '1,120p'Length of output: 27
I'll re-run the search with corrected syntax and verify the LinkAnalyticsBadge implementation.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for unencoded url parameters using glob pattern
rg -nP -C2 '&url=\$\{(?!encodeURIComponent)' -g "*.tsx" apps/web
echo "---"
echo "Checking LinkAnalyticsBadge file location and content:"
# Find and examine the LinkAnalyticsBadge file
fd LinkAnalyticsBadge -e tsx apps/web
Length of output: 1085
I found the issue confirmed in the codebase. Let me examine the LinkAnalyticsBadge file more closely and search for all unencoded URL instances.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the LinkAnalyticsBadge file in full
echo "=== LinkAnalyticsBadge.tsx content ==="
cat apps/web/ui/links/link-analytics-badge.tsx
echo ""
echo "=== Search for all &url= patterns (encoded and unencoded) ==="
rg -nP '&url=' -g "*.tsx" -g "*.ts" apps/web | head -30
Length of output: 6880
Encode URL passed into analytics link query string.
The URL parameter at line 150 of apps/web/ui/links/link-analytics-badge.tsx is inserted into the query string without encoding. If url contains &, ?, =, or other reserved characters, it will break query string parsing.
Change:
- href={`/${slug}/analytics?domain=${domain}&key=${key}${url ? `&url=${url}` : ""}&interval=${plan === "free" ? "30d" : plan === "pro" ? "1y" : "all"}`}
+ href={`/${slug}/analytics?domain=${domain}&key=${key}${url ? `&url=${encodeURIComponent(url)}` : ""}&interval=${plan === "free" ? "30d" : plan === "pro" ? "1y" : "all"}`}Note: A similar unencoded url parameter exists at apps/web/ui/links/link-builder/options-list.tsx:126 (/api/links/iframeable?domain=${domain}&url=${debouncedUrl}), which should also be encoded for consistency.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx (1)
371-384: Improve trailing slash removal to handle multiple slashes.The current regex
/\/$/only removes a single trailing slash. URLs with multiple trailing slashes (e.g.,https://example.com///) would not be fully normalized.Apply this diff:
onBlur={(e) => { field.onBlur(e); const url = getUrlFromString(e.target.value); if (url) { - const normalizedUrl = url.replace(/\/$/, ""); + const normalizedUrl = url.replace(/\/+$/, ""); setValue( `testVariants.${index}.url`, normalizedUrl, { shouldDirty: true, }, ); } }}apps/web/ui/modals/link-builder/ab-testing-modal.tsx (7)
23-29: Centralize URL normalization.Good call importing getUrlFromString. Consider introducing/using a single normalizeUrl helper (scheme add, trim, trailing slash policy) across inputs to avoid drift between fields and analytics. Right now we partially normalize (only trailing “/”).
285-288: Make help copy dynamic w.r.t MAX_TEST_COUNT.Avoid hard-coding “3 additional”; derive from MAX_TEST_COUNT to prevent future drift.
- title="Add up to 3 additional destination URLs to test for this short link." + title={`Add up to ${Math.max(0, MAX_TEST_COUNT - 1)} additional destination URLs to test for this short link.`}
229-233: Guard against duplicate URLs.Duplicated variants skew analytics and winner selection. Add a uniqueness check before save.
// Validate all URLs are filled if (currentTests.some((test) => !test.url)) { toast.error("All test URLs must be filled"); return; } + // Validate URLs are unique (case-insensitive) + const urls = currentTests.map((t) => t.url.trim().toLowerCase()); + if (urls.some((u, i) => urls.indexOf(u) !== i)) { + toast.error("Each testing URL must be unique"); + return; + }
464-474: Six‑week warning styling only changes color; consider an accessible status.Optional: add role="status" or aria-live="polite" so screen readers notice when the warning turns red; also include the threshold in text for clarity (e.g., “Set within 6 weeks to enable saving”).
533-537: Minor: typo and small cleanup.
- Fix “competion” → “completion”.
- Compute difference once to avoid repeated calls.
- // Restrict competion date from -1 days to 6 weeks - (differenceInDays(testCompletedAt, new Date()) > 6 * 7 || - differenceInDays(testCompletedAt, new Date()) < -1), + // Restrict completion date from -1 days to 6 weeks + ((d => d > 6 * 7 || d < -1)( + differenceInDays(testCompletedAt, new Date()) + )),
689-723: Optional: stable keys to preserve focus when removing rows.Using index as key may cause focus jumps when deleting in the middle. If feasible, add a stable id to each test variant and use it as key.
657-660: Comment accuracy.Comment says “minimum 10%”, but you enforce MIN_TEST_PERCENTAGE. Update comment to avoid confusion.
- // Ensure minimum 10% for each test + // Ensure minimum MIN_TEST_PERCENTAGE% for each test
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/ui/modals/link-builder/ab-testing-modal.tsx(6 hunks)apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (2)
packages/utils/src/functions/urls.ts (2)
isValidUrl(1-8)getUrlFromString(10-18)apps/web/lib/zod/schemas/links.ts (1)
MAX_TEST_COUNT(30-30)
apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx (2)
packages/utils/src/functions/urls.ts (2)
isValidUrl(1-8)getUrlFromString(10-18)apps/web/lib/zod/schemas/links.ts (1)
MAX_TEST_COUNT(30-30)
🔇 Additional comments (7)
apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx (6)
27-27: LGTM!The import of
getUrlFromStringis necessary for the URL normalization feature implemented in the onBlur handler.
330-330: LGTM!The anchor link provides better navigation to the specific help documentation section.
386-398: LGTM!The Remove button is correctly restricted to URLs after the first one (index > 0), ensuring the first testing URL remains in sync with the destination URL as per the PR objectives.
431-431: LGTM!The reduced spacing aligns with the PR objective to tighten the layout between the traffic split label and input.
457-459: LGTM!The wording change to "Schedule" is clearer, and the updated anchor link provides better navigation to the relevant documentation section.
273-273: Verify two-way sync from destination URL to testVariants[0].Line 273 syncs
testVariants[0].urlto the parent's main URL (destination URL), but the reverse sync (destination URL →testVariants[0].url) is not implemented. The PR objectives mention "two-way sync" between these URLs.Ensure that when the destination URL changes,
testVariants[0].urlis updated accordingly. This may require adding a watch or useEffect hook in the parent form component or the main form controller to propagate destination URL changes back to the first test variant.apps/web/ui/modals/link-builder/ab-testing-modal.tsx (1)
413-418: Verify help anchor.Confirm the ab-testing#completion-date-has-passed anchor exists and matches the doc section title to avoid 404/scroll misses.
| {testVariants.map((_, index) => { | ||
| const field = register(`testVariants.${index}.url`, { | ||
| validate: (value, { testVariants }) => { | ||
| if (!value) return "URL is required"; | ||
|
|
||
| if (!isValidUrl(value)) return "Invalid URL"; | ||
|
|
||
| return ( | ||
| testVariants.length > 1 && | ||
| testVariants.length <= MAX_TEST_COUNT | ||
| ); | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Field validator mixes value and cross-field rules.
Per-field validate should only check the field’s URL. Using testVariants.length here turns the whole form invalid when there’s 1 URL (and shows no message). Keep length constraints in add/remove logic and submit guard.
- const field = register(`testVariants.${index}.url`, {
- validate: (value, { testVariants }) => {
- if (!value) return "URL is required";
- if (!isValidUrl(value)) return "Invalid URL";
- return (
- testVariants.length > 1 &&
- testVariants.length <= MAX_TEST_COUNT
- );
- },
- });
+ const field = register(`testVariants.${index}.url`, {
+ validate: (value) => {
+ if (!value) return "URL is required";
+ if (!isValidUrl(value)) return "Invalid URL";
+ return true;
+ },
+ });| type="url" | ||
| placeholder={ | ||
| domains?.find(({ slug }) => slug === domain) | ||
| ?.placeholder || | ||
| "https://dub.co/help/article/what-is-dub" | ||
| } | ||
| className="block h-9 grow border-none px-2 text-neutral-900 placeholder-neutral-400 focus:ring-0 sm:text-sm" | ||
| {...field} | ||
| onBlur={(e) => { | ||
| field.onBlur(e); | ||
| const url = getUrlFromString(e.target.value); | ||
| if (url) { | ||
| const normalizedUrl = url.replace(/\/$/, ""); | ||
| setValue( | ||
| `testVariants.${index}.url`, | ||
| normalizedUrl, | ||
| { | ||
| shouldDirty: true, | ||
| }, | ||
| ); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
Trim + validate-after-normalize + immediate two-way sync for index 0.
- Missing trim means “example.com ” won’t get https:// auto-appended (getUrlFromString bails on spaces).
- Calling field.onBlur before setValue validates the pre-normalized value; also you don’t set shouldValidate, so isValid may lag.
- To meet the “two-way sync” objective, reflect index 0 changes into parent url on blur, not only on submit.
- {...field}
- onBlur={(e) => {
- field.onBlur(e);
- const url = getUrlFromString(e.target.value);
- if (url) {
- const normalizedUrl = url.replace(/\/$/, "");
- setValue(
- `testVariants.${index}.url`,
- normalizedUrl,
- {
- shouldDirty: true,
- },
- );
- }
- }}
+ {...field}
+ onBlur={(e) => {
+ const raw = e.target.value.trim();
+ const url = getUrlFromString(raw);
+ if (url) {
+ const normalizedUrl = url.replace(/\/+$/, "");
+ setValue(`testVariants.${index}.url`, normalizedUrl, {
+ shouldDirty: true,
+ shouldValidate: true,
+ shouldTouch: true,
+ });
+ if (index === 0) {
+ // Keep destination URL and position 1 in sync
+ setValueParent("url", normalizedUrl, { shouldDirty: true });
+ }
+ }
+ field.onBlur(e);
+ }}If trailing-slash removal is product-wide policy, confirm with analytics normalization to avoid mismatches.
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect the ab-testing-modal.tsx file to understand form context and structure
fd --type f -name "ab-testing-modal.tsx" --exec head -100 {} \;
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for getUrlFromString implementation and its whitespace handling
rg -n "getUrlFromString" --type=ts --type=tsx -B2 -A10Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for setValueParent or parent form setValue patterns
rg -n "setValueParent|setValue.*url\b" --type=ts --type=tsx -B2 -A2 | head -50Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for form setup and parent context in the component
fd --type f -name "ab-testing-modal.tsx" --exec grep -n "useFormContext\|getValues\|watch\|index === 0" {} \;Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for any trailing slash normalization patterns in the codebase
rg -n "replace.*\/$" --type=ts --type=tsx -B2 -A2 | head -40Length of output: 87
🏁 Script executed:
#!/bin/bash
# Find the ab-testing-modal file
fd "ab-testing-modal" --type fLength of output: 184
🏁 Script executed:
#!/bin/bash
# Search for getUrlFromString without type constraint
rg "getUrlFromString" -B2 -A10Length of output: 23048
🏁 Script executed:
#!/bin/bash
# Search for setValueParent or parent form setValue
rg "setValueParent|setValue.*\(.*url" -B2 -A3Length of output: 32657
🏁 Script executed:
#!/bin/bash
# Find the full ab-testing-modal file and check its structure
fd "ab-testing-modal" --type f -x cat {}Length of output: 55729
🏁 Script executed:
#!/bin/bash
# Search for trailing slash removal patterns more carefully
rg 'replace.*\/\$' -B2 -A2Length of output: 88
Verify and implement the four fixes: trim before URL parsing, reorder blur handler steps, add validation options, and sync index-0 changes to parent.
The issues identified in the review are confirmed:
-
Missing trim(): getUrlFromString checks
!str.includes(" ")and bails if spaces are present, so "example.com " won't receive thehttps://prefix without trimming first. -
Validation timing: field.onBlur is called before setValue normalizes the value, leaving validation state potentially stale. The review correctly identifies shouldValidate and shouldTouch flags are missing.
-
Parent synchronization: setValueParent is available via useFormContext, confirming the parent form function exists and can be called to sync index 0 changes back to the parent link URL.
-
Trailing slash robustness: The suggested change from
/\/$/, ""to/\/+$/, ""handles multiple trailing slashes, a defensive improvement.
The diff provided is structurally sound and aligns with the codebase patterns (e.g., similar sync logic in destination-url-input.tsx). Implement as suggested, and verify with your analytics team that trailing-slash normalization is indeed applied consistently product-wide to avoid data mismatches.
🤖 Prompt for AI Agents
In apps/web/ui/modals/link-builder/ab-testing-modal.tsx around lines 320 to 341,
the onBlur handler must be updated: first trim the input string before calling
getUrlFromString so trailing spaces don't prevent URL normalization, then if a
URL is found replace multiple trailing slashes using .replace(/\/+$/, "") and
call setValue to store the normalized URL, passing { shouldDirty: true,
shouldTouch: true, shouldValidate: true }; after setValue, call field.onBlur(e)
(not before) so validation sees the normalized value; additionally, when index
=== 0 call setValueParent(...) to sync the parent link URL with the same
normalized value.
| const field = register(`testVariants.${index}.url`, { | ||
| validate: (value, { testVariants }) => { | ||
| if (!value) return "URL is required"; | ||
|
|
||
| if (!isValidUrl(value)) return "Invalid URL"; | ||
|
|
||
| return ( | ||
| testVariants.length > 1 && | ||
| testVariants.length <= MAX_TEST_COUNT | ||
| ); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Validation timing may cause UX friction.
The validation runs in onChange mode (line 127), so URLs like "example.com" will show "Invalid URL" errors until the user blurs the field to trigger normalization. This could be confusing for users who expect auto-correction.
Consider either:
- Adding a validation bypass for URLs that would become valid after normalization
- Using
onBlurvalidation mode for URL fields
Apply this diff to improve the validation experience:
- validate: (value, { testVariants }) => {
- if (!value) return "URL is required";
-
- if (!isValidUrl(value)) return "Invalid URL";
-
+ validate: (value, { testVariants }) => {
+ if (!value) return "URL is required";
+
+ // Allow URLs that would be valid after normalization
+ const normalized = getUrlFromString(value);
+ if (!isValidUrl(normalized)) return "Invalid URL";
+
return (
testVariants.length > 1 &&
testVariants.length <= MAX_TEST_COUNT
);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const field = register(`testVariants.${index}.url`, { | |
| validate: (value, { testVariants }) => { | |
| if (!value) return "URL is required"; | |
| if (!isValidUrl(value)) return "Invalid URL"; | |
| return ( | |
| testVariants.length > 1 && | |
| testVariants.length <= MAX_TEST_COUNT | |
| ); | |
| }, | |
| }); | |
| const field = register(`testVariants.${index}.url`, { | |
| validate: (value, { testVariants }) => { | |
| if (!value) return "URL is required"; | |
| // Allow URLs that would be valid after normalization | |
| const normalized = getUrlFromString(value); | |
| if (!isValidUrl(normalized)) return "Invalid URL"; | |
| return ( | |
| testVariants.length > 1 && | |
| testVariants.length <= MAX_TEST_COUNT | |
| ); | |
| }, | |
| }); |
| <p | ||
| className={cn( | ||
| "mt-1 text-xs", | ||
| testCompletedAt && | ||
| differenceInDays(testCompletedAt, new Date()) > 6 * 7 | ||
| ? "text-red-700" | ||
| : "text-neutral-500", | ||
| )} | ||
| > | ||
| 6 weeks maximum | ||
| </p> |
There was a problem hiding this comment.
Add visual feedback for past completion dates.
The warning text turns red only when the completion date exceeds 6 weeks in the future (line 511), but the submit button is also disabled for dates less than -1 days in the past (line 579). This inconsistency means users won't receive visual feedback when selecting invalid past dates.
Apply this diff to provide consistent visual feedback:
<p
className={cn(
"mt-1 text-xs",
testCompletedAt &&
- differenceInDays(testCompletedAt, new Date()) > 6 * 7
+ (differenceInDays(testCompletedAt, new Date()) > 6 * 7 ||
+ differenceInDays(testCompletedAt, new Date()) < -1)
? "text-red-700"
: "text-neutral-500",
)}
>
- 6 weeks maximum
+ {testCompletedAt &&
+ differenceInDays(testCompletedAt, new Date()) < -1
+ ? "Date must be in the future"
+ : "6 weeks maximum"}
</p>🤖 Prompt for AI Agents
In apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx around lines
507 to 517, the warning text only turns red for completion dates more than 6
weeks in the future but not for dates too far in the past (the submit button is
disabled for dates < -1 days), so update the conditional that sets the text
color to also mark the text red when differenceInDays(testCompletedAt, new
Date()) < -1; i.e., change the className condition to red when
differenceInDays(...) > 6*7 OR differenceInDays(...) < -1 so users get
consistent visual feedback for both invalid future and invalid past dates.
1 participant
This is a major push to make the A/B testing really functional and useful for people. Defining the new logic and helping clarify a few unknowns.
2-way sync between the destination url and the url in position 1
It was saving from the destination URL to the testing URL, but not the other way around. This is now synced for two-way.
Reorganization of opened link view
When the A/B test is open in the link view, the traffic percentage has been moved to the right for better alignment with the stats.
A/B test tooltip
When hovering the A/B test icon in the links view, we now show that the A/B test is running and provide the completion date.
Completion date visual warning
If a user selects a completion date further out than six weeks, the note changes to a red color to indicate that it's outside that window.
Appending http to urls
When adding testing urls, if HTTPS:// isn't added when focus isn't on the input, it adds it in, much like the destination URL.
End test selection
If the user is ending the test early, we now show the stats when they're selecting the URL as the winner.
Ending modal overflow
If the link overflows to ellipses, and the user can't see the entire URL that they added.
Winner logic updates
OR - If no conversions are recorded or trackedOther minor updates
Summary by CodeRabbit
New Features
Improvements