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 |
WalkthroughAggregates analytics by normalized URL (clicks, leads, sales, saleAmount), selects a deterministic winner via a multi-step tie-break cascade, normalizes/synchronizes per-field URLs in forms, removes the old ABTesting modal, adds a new EndABTesting modal using aggregated analytics, and fixes query encoding with URLSearchParams. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as complete-ab-tests
participant DB as Analytics Store
participant Norm as normalizeUrl()
participant Agg as Aggregator
participant Decide as Tie-breaker
participant Persist as Persist/Logger
Handler->>DB: fetch analytics rows for link
DB-->>Handler: rows (raw URL, clicks, leads, sales, saleAmount)
loop per row
Handler->>Norm: normalize row.url
Norm-->>Agg: sum metrics under normalized URL
end
Agg->>Decide: compute conversions & rates per normalized URL
alt any conversions > 0
Decide->>Decide: rank by conversions → conversionRate → clicks
else no conversions
Decide->>Decide: rank by leads → leadRate → clicks
end
Decide->>Persist: single deterministic winner?
alt Winner found
Persist->>Persist: ensure winner != original URL, persist change
else No deterministic winner
Persist->>Persist: log and do not change
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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).
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx
Outdated
Show resolved
Hide resolved
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx
Outdated
Show resolved
Hide resolved
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.
apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx
Outdated
Show resolved
Hide resolved
apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx
Outdated
Show resolved
Hide resolved
Replaces the legacy ab-testing-modal implementation with a refactored version, improving URL validation, input handling, and user experience. Updates related components to use encodeURIComponent for URLs, display pretty URLs, and streamline modal state management. Removes the obsolete ab-testing-modal file and updates references to utility functions and keyboard shortcuts.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/ui/links/link-builder/options-list.tsx (1)
155-163: Harden iframe preview (scheme check, sandbox, a11y).Avoid javascript:/data: in iframes; add sandbox and title.
- <iframe - src={url} - style={{ - zoom: 0.5, - }} - className="h-[500px] w-[888px]" - /> + <iframe + src={/^https?:\/\//i.test(url) ? url : undefined} + title="Link cloaking preview" + sandbox="allow-scripts allow-popups" + referrerPolicy="no-referrer" + style={{ zoom: 0.5 }} + className="h-[500px] w-[888px]" + />Optionally replace non‑standard zoom with CSS transform for Safari.
♻️ Duplicate comments (2)
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (1)
147-156: Fix unstable/invalid key; add radio semantics to item.test.id doesn’t exist on this type; use a stable key (URL). Also add role/aria-checked.
- <button - key={test.id} + <button + key={normalizeUrl(test.url) || test.url} type="button" + role="radio" + aria-checked={selectedUrl === test.url} onClick={() => setSelectedUrl(test.url)}apps/web/ui/modals/link-builder/ab-testing-modal.tsx (1)
411-428: Normalize on blur: trim, strip multi trailing slashes, validate, and two‑way sync index 0.Current code misses trim, doesn’t set shouldValidate/shouldTouch, and doesn’t sync index 0 back to parent on blur.
- onBlur={(e) => { - const url = getUrlFromString(e.target.value); - if (url) { - const normalizedUrl = url.replace(/\/$/, ""); - setValue( - `testVariants.${index}.url`, - normalizedUrl, - { - shouldDirty: true, - }, - ); - // Clear the local input state to show the pretty URL - setUrlInputs((prev) => ({ - ...prev, - [index]: undefined, - })); - } - }} + 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) { + setValueParent("url", normalizedUrl, { shouldDirty: true }); + } + setUrlInputs((prev) => ({ ...prev, [index]: undefined })); + } + }}
🧹 Nitpick comments (8)
apps/web/ui/links/link-builder/options-list.tsx (2)
124-129: Good fix; encode domain too (or use URLSearchParams).encodeURIComponent(debouncedUrl) is correct. Also encode domain to avoid edge cases. Prefer URLSearchParams for both.
- ? `/api/links/iframeable?domain=${domain}&url=${encodeURIComponent(debouncedUrl)}` + ? `/api/links/iframeable?${new URLSearchParams({ + domain, + url: debouncedUrl, + }).toString()}`
131-148: Memo deps include props for safety.badge useMemo depends on toggle/onRemove; add to deps to avoid stale closures.
- }, [data, isLoading]); + }, [data, isLoading, toggle, onRemove]);apps/web/ui/links/link-analytics-badge.tsx (1)
149-156: Nice: URL param is now encoded. Consider URLSearchParams for all query parts.This prevents breakage when url has &/? etc. For full safety and readability, build the href with URLSearchParams (encodes domain/key/interval too).
- href={`/${slug}/analytics?domain=${domain}&key=${key}${url ? `&url=${encodeURIComponent(url)}` : ""}&interval=${plan === "free" ? "30d" : plan === "pro" ? "1y" : "all"}`} + {(() => { + const q = new URLSearchParams({ + domain, + key, + interval: plan === "free" ? "30d" : plan === "pro" ? "1y" : "all", + }); + if (url) q.set("url", url); + return `/${slug}/analytics?${q.toString()}`; + })()}apps/web/ui/links/link-tests.tsx (1)
103-151: Use a stable key per test (avoid index).Index keys cause reconciliation issues on re‑order/delete. Use normalized URL.
- {testVariants.map((test, idx) => { + {testVariants.map((test, idx) => { const normalizedTestUrl = normalizeUrl(test.url); const analytics = analyticsByNormalizedUrl?.get(normalizedTestUrl); - - return ( - <li - key={idx} + return ( + <li + key={normalizedTestUrl || idx} className="flex items-center justify-between rounded-md border border-neutral-300 bg-white p-2.5" >apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (3)
18-49: Overflow detection won’t update on resize.Add a ResizeObserver to recompute isOverflowing on container resize.
- 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]);
142-151: Add radiogroup semantics for accessibility.Mark the list as a radio group.
- <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-188: Hoist getValuesParent() out of map.Avoid re-reading form values per row.
- {testVariants?.map((test, index) => { - const normalized = normalizeUrl(test.url); - const analytics = analyticsByNormalizedUrl?.get(normalized); - const link = getValuesParent(); + const parentLink = getValuesParent(); + {testVariants?.map((test, index) => { + const normalized = normalizeUrl(test.url); + const analytics = analyticsByNormalizedUrl?.get(normalized); return ( <button ... - <LinkAnalyticsBadge + <LinkAnalyticsBadge link={{ - ...(link as any), + ...(parentLink as any),apps/web/ui/modals/link-builder/ab-testing-modal.tsx (1)
383-456: Input type vs. displayed value.You render pretty URLs without scheme into type="url", which can trigger native invalid state. Either always show full URL while focused (you do) or switch to type="text" with inputMode="url".
- <input - type="url" + <input + type="text" + inputMode="url"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/ui/links/link-analytics-badge.tsx(1 hunks)apps/web/ui/links/link-builder/options-list.tsx(1 hunks)apps/web/ui/links/link-tests.tsx(5 hunks)apps/web/ui/modals/link-builder/ab-testing-modal.tsx(14 hunks)apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx(0 hunks)apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- apps/web/ui/modals/link-builder/ab-testing/ab-testing-modal.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (2)
packages/utils/src/functions/urls.ts (2)
getPrettyUrl(130-138)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 (1)
packages/utils/src/functions/urls.ts (3)
isValidUrl(1-8)getPrettyUrl(130-138)getUrlFromString(10-18)
apps/web/ui/links/link-tests.tsx (1)
packages/utils/src/functions/urls.ts (2)
normalizeUrl(173-180)getPrettyUrl(130-138)
⏰ 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 (3)
apps/web/ui/links/link-tests.tsx (1)
57-92: Aggregation by normalized URL looks solid.Clean summation with null guards; Map avoids N^2 scans. LGTM.
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (2)
565-575: 6‑week visual cue looks correct.Clear condition and text. LGTM.
785-788: Keyboard shortcut wired to modal context.Good use of modal=true to avoid global conflicts. LGTM.
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx
Outdated
Show resolved
Hide resolved
Refactors URL normalization logic in A/B testing modals to trim whitespace, ensure protocol, and remove trailing slashes for consistency with analytics. Updates key props in test lists for stability, enhances accessibility in radio group selection, and improves overflow detection in tooltips. Also updates query parameter construction to use URLSearchParams for better encoding and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/ui/links/tests-badge.tsx (1)
16-29: Six‑week warning implemented as specified.Date derivation and conditional coloring look good; resolves the earlier review concern.
Also applies to: 37-60
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (1)
188-194: Good fix for nested interactive controls.Capturing click to prevent inner Link navigation addresses the invalid nested interactive pattern.
🧹 Nitpick comments (9)
apps/web/ui/links/link-analytics-badge.tsx (1)
87-94: Unify href construction and guard undefined plan.Reuse the same URLSearchParams block for mobile and default interval when plan is not yet loaded to avoid briefly linking to “all”.
- <Link - href={`/${slug}/analytics?domain=${domain}&key=${key}`} + <Link + href={`/${slug}/analytics?${(() => { + const interval = + plan ? (plan === "free" ? "30d" : plan === "pro" ? "1y" : "all") : "30d"; + const params = new URLSearchParams({ domain, key, interval }); + if (url) params.set("url", url); + return params.toString(); + })()}`} className="flex items-center gap-1 rounded-md border border-neutral-200 bg-neutral-50 px-2 py-0.5 text-sm text-neutral-800" >Also applies to: 150-158
apps/web/ui/links/link-tests.tsx (2)
31-55: Use null for disabled SWR keys.Passing false is accepted but not idiomatic; prefer null for clarity and typings.
- >( - Boolean(testVariants && testVariants.length) && - showTests && - `/api/analytics?${new URLSearchParams({ + >( + testVariants?.length && showTests + ? `/api/analytics?${new URLSearchParams({ event: "composite", groupBy: "top_urls", linkId: link.id, workspaceId: workspaceId!, ...(link.testStartedAt && { start: new Date(link.testStartedAt).toISOString(), }), - }).toString()}`, + }).toString()}` + : null, fetcher, { revalidateOnFocus: false, }, );
104-110: Pass normalized URL to analytics and keep badge on errors.
- Use normalizedTestUrl for the url param to align with analytics normalization.
- Instead of hiding the badge on error, render it with zeroed metrics for consistent UI.
- const normalizedTestUrl = normalizeUrl(test.url); + const normalizedTestUrl = normalizeUrl(test.url); const analytics = analyticsByNormalizedUrl?.get(normalizedTestUrl); ... - <div className="flex justify-end sm:min-w-48"> - {isLoading ? ( + <div className="flex justify-end sm:min-w-48"> + {isLoading ? ( <div className="h-7 w-32 animate-pulse rounded-md bg-neutral-100" /> - ) : error ? null : ( + ) : ( <LinkAnalyticsBadge link={{ ...link, clicks: analytics?.clicks ?? 0, leads: analytics?.leads ?? 0, sales: analytics?.sales ?? 0, saleAmount: analytics?.saleAmount ?? 0, }} - url={test.url} + url={normalizedTestUrl} sharingEnabled={false} /> )} </div>Also applies to: 126-151
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (3)
96-108: Prefer null over false for disabled SWR keys.Clearer, typed, and matches SWR docs.
- >( - Boolean(testVariants && testVariants.length && linkId && workspaceId) && - `/api/analytics?${new URLSearchParams({ + >( + 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()}`, + }).toString()}` + : null, fetcher, { revalidateOnFocus: false }, );
164-165: Reuse normalized value and ensure unique keys.Avoid recomputing normalizeUrl and guard against duplicate normalized URLs.
- <button - key={normalizeUrl(test.url) || test.url} + <button + key={normalized || `${test.url}-${index}`}
158-161: Avoid name shadowing and any; clarify intent.Rename local form value to prevent confusion with the LinkAnalyticsBadge prop and add a minimal type.
- const link = getValuesParent(); + const parentLink = getValuesParent() as Partial<ResponseLink>; ... - link={{ - ...(link as any), + link={{ + ...(parentLink as any), clicks: analytics?.clicks ?? 0, leads: analytics?.leads ?? 0, sales: analytics?.sales ?? 0, saleAmount: analytics?.saleAmount ?? 0, }}Also applies to: 199-205
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (3)
148-195: Guard add-split when no variant ≥ 2× min.findLastIndex can return -1; fall back to the last item to avoid undefined access.
- const toSplitIndex = testVariants.findLastIndex( - ({ percentage }) => percentage >= MIN_TEST_PERCENTAGE * 2, - ); - const toSplit = testVariants[toSplitIndex]; + const toSplitIndexRaw = testVariants.findLastIndex( + ({ percentage }) => percentage >= MIN_TEST_PERCENTAGE * 2, + ); + const toSplitIndex = + toSplitIndexRaw === -1 ? testVariants.length - 1 : toSplitIndexRaw; + const toSplit = testVariants[toSplitIndex];
385-455: Deduplicate normalization into a helper.Define a small normalizeForForm helper and reuse in onBlur and submit/save to keep behavior identical.
+// top-level (near other utils) +const normalizeForForm = (raw: string) => + getUrlFromString(raw.trim()).replace(/\/+$/, ""); ... - const raw = e.target.value.trim(); - const url = getUrlFromString(raw); - if (url) { - const normalizedUrl = url.replace(/\/+$/, ""); + const normalizedUrl = normalizeForForm(e.target.value); + if (normalizedUrl) { ... - const normalizedTests = currentTests.map((test) => { - const trimmedUrl = test.url.trim(); - const urlWithProtocol = getUrlFromString(trimmedUrl); - const normalizedUrl = urlWithProtocol.replace(/\/+$/, ""); + const normalizedTests = currentTests.map((test) => { + const normalizedUrl = normalizeForForm(test.url || ""); return { ...test, url: normalizedUrl, }; });Also applies to: 291-309, 689-699
854-961: Add keyboard accessibility to the slider.Current slider is mouse-only. Provide arrow key support and ARIA (role="slider", aria-valuemin/max/now). Improves a11y without changing visuals.
I can draft a minimal accessible handler (Left/Right adjust ±1%, honoring MIN_TEST_PERCENTAGE).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/ui/links/link-analytics-badge.tsx(1 hunks)apps/web/ui/links/link-builder/options-list.tsx(2 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(14 hunks)apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/ui/links/link-builder/options-list.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (2)
packages/utils/src/functions/urls.ts (2)
getPrettyUrl(130-138)normalizeUrl(173-180)apps/web/ui/links/link-analytics-badge.tsx (1)
LinkAnalyticsBadge(28-200)
apps/web/ui/links/link-tests.tsx (1)
packages/utils/src/functions/urls.ts (2)
normalizeUrl(173-180)getPrettyUrl(130-138)
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (1)
packages/utils/src/functions/urls.ts (3)
isValidUrl(1-8)getUrlFromString(10-18)getPrettyUrl(130-138)
⏰ 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 (5)
apps/web/ui/links/link-analytics-badge.tsx (1)
150-158: Correct: safe query construction with URLSearchParams.Fixes encoding pitfalls for url/domain/key/interval. Good change.
apps/web/ui/links/link-tests.tsx (1)
57-92: Aggregation by normalized URL is solid.Handles duplicates, nullish values, and consolidates metrics correctly.
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (1)
110-136: Aggregation looks correct.Normalization + summation handles duplicates and nullish metrics well.
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (2)
132-146: Cleaner URL validation separation.Per-form validateUrls isolates cross-field rules; good structure.
299-317: Normalization + two‑way sync achieved.Trim → getUrlFromString → strip trailing slashes, then sync index 0 to parent url; meets PR objectives.
Also applies to: 310-316
Refactored URL normalization in AB testing modals for consistency and accessibility. Enhanced TrafficSplitSlider with keyboard support and improved focus handling. Updated LinkAnalyticsBadge and related components to ensure correct query parameter construction and key usage.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/web/ui/links/link-tests.tsx (1)
103-111: Use a stable, non-colliding key.Two tests can normalize to the same URL; keys may collide. Include the index.
- key={normalizedTestUrl || `${test.url}-${idx}`} + key={`${normalizedTestUrl}-${idx}`}apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (2)
190-211: Avoid nested interactive controls; prefer non-interactive badge variant.A Link (inside LinkAnalyticsBadge) within a button is invalid. Stopping propagation prevents navigation but not semantics. Expose a non-interactive mode in LinkAnalyticsBadge (render a
/) and use it here.
165-171: Strengthen list keys.Keys may collide if two URLs normalize the same. Include the index for stability.
- key={normalized || `${test.url}-${index}`} + key={`${normalized}-${index}`}apps/web/ui/modals/link-builder/ab-testing-modal.tsx (3)
381-388: Avoid index keys in dynamic lists.Index keys can mis-associate inputs after add/remove. Prefer a stable key.
-<div key={index} className="flex items-center gap-2"> +<div key={`${test.url}-${index}`} className="flex items-center gap-2">If feasible, use a generated id per item instead of URL.
403-417: Trim in onChange to reduce transient invalid state.Pre-trim user input before adding scheme.
-const inputValue = e.target.value; +const inputValue = e.target.value.trim();
271-324: Deduplicate submit logic.Form onSubmit and primary button onClick duplicate the same validations and updates. Extract a shared handler to reduce drift.
Example (outline):
function applyAbTestChanges(currentTests, completedAt) { // validations... const normalizedTests = currentTests.map(t => ({ ...t, url: normalizeForForm(t.url || "") })); setValueParent("url", normalizedTests[0].url, { shouldDirty: true }); setValueParent("trackConversion", true); setValueParent("testVariants", normalizedTests, { shouldDirty: true }); setValueParent("testCompletedAt", completedAt, { shouldDirty: true }); setShowABTestingModal(false); }Call it from both places.
Also applies to: 655-711
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/ui/links/link-analytics-badge.tsx(2 hunks)apps/web/ui/links/link-tests.tsx(5 hunks)apps/web/ui/modals/link-builder/ab-testing-modal.tsx(18 hunks)apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/ui/links/link-analytics-badge.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/ui/links/link-tests.tsx (1)
packages/utils/src/functions/urls.ts (2)
normalizeUrl(173-180)getPrettyUrl(130-138)
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (3)
packages/utils/src/functions/urls.ts (2)
getPrettyUrl(130-138)normalizeUrl(173-180)apps/web/ui/links/links-container.tsx (1)
ResponseLink(20-22)apps/web/ui/links/link-analytics-badge.tsx (1)
LinkAnalyticsBadge(28-208)
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (2)
packages/utils/src/functions/urls.ts (3)
getUrlFromString(10-18)isValidUrl(1-8)getPrettyUrl(130-138)apps/web/lib/zod/schemas/links.ts (1)
MIN_TEST_PERCENTAGE(29-29)
⏰ 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 (3)
apps/web/ui/links/link-tests.tsx (1)
57-92: Confirm URL normalization level matches backend grouping.You aggregate by normalizeUrl(host+path), dropping query/hash. If variants differ only by query, analytics collapse. Confirm this matches /api/analytics?groupBy=top_urls semantics and product intent.
Also applies to: 103-106
apps/web/ui/modals/link-builder/ab-testing/end-ab-testing-modal.tsx (1)
149-158: Nice accessibility upgrades.Radiogroup, aria-checked, and focus styles are solid; UrlWithTooltip improves long-URL readability.
Also applies to: 165-213
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (1)
904-937: Great keyboard accessibility on the traffic slider.Arrow key support, roles, and focus styles look good.
Also applies to: 978-999
Adds workspaceId check to analytics API call in LinkTests and trims input before normalizing URLs in ABTestingModal to prevent issues with trailing spaces.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (1)
52-54: normalizeForForm now trims before normalization — resolved.This fixes the space-related normalization gap flagged earlier.
🧹 Nitpick comments (5)
apps/web/ui/links/link-tests.tsx (2)
57-92: Align trailing‑slash normalization to avoid split analytics buckets.Analytics aggregation keys currently differ for /path vs /path/. Strip trailing slashes both when building the map and when deriving normalizedTestUrl so lookups match.
- const key = normalizeUrl(row.url); + const key = normalizeUrl(row.url).replace(/\/+$/, ""); @@ - const normalizedTestUrl = normalizeUrl(test.url); + const normalizedTestUrl = normalizeUrl(test.url).replace(/\/+$/, "");Also applies to: 104-106, 140-141
109-110: Make list item keys unambiguous even if URLs duplicate.Two variants could point to the same URL; include index in the key to avoid React key collisions.
- key={normalizedTestUrl || `${test.url}-${idx}`} + key={`${idx}-${normalizedTestUrl || test.url}`}apps/web/ui/modals/link-builder/ab-testing-modal.tsx (3)
172-177: Array.findLastIndex browser support.findLastIndex is relatively new; confirm your target browsers/runtime support it or replace with a small backwards-compatible loop.
- const toSplitIndexRaw = testVariants.findLastIndex( - ({ percentage }) => percentage >= MIN_TEST_PERCENTAGE * 2, - ); + const toSplitIndexRaw = (() => { + for (let i = testVariants.length - 1; i >= 0; i--) { + if (testVariants[i].percentage >= MIN_TEST_PERCENTAGE * 2) return i; + } + return -1; + })();
870-879: Clamp mouse percentage to [0, 100] for smoother dragging.Out-of-bounds cursor positions can produce negative or >100 calculations; clamp before applying.
- const mouseX = e.clientX - containerRect.x; - const mousePercentage = Math.round((mouseX / containerWidth) * 100); + const mouseX = e.clientX - containerRect.x; + const raw = (mouseX / containerWidth) * 100; + const mousePercentage = Math.max(0, Math.min(100, Math.round(raw)));
806-807: Remove unused variable.complete is defined but never used.
- const complete = enabled && new Date() > new Date(testCompletedAt!);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/ui/links/link-tests.tsx(5 hunks)apps/web/ui/modals/link-builder/ab-testing-modal.tsx(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/ui/links/link-tests.tsx (1)
packages/utils/src/functions/urls.ts (2)
normalizeUrl(173-180)getPrettyUrl(130-138)
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (1)
packages/utils/src/functions/urls.ts (3)
getUrlFromString(10-18)isValidUrl(1-8)getPrettyUrl(130-138)
⏰ 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 (1)
apps/web/ui/links/link-tests.tsx (1)
40-51: SWR key gating now includes workspaceId — LGTM.This prevents requests like workspaceId=undefined. Good guard.
URLs are now validated after normalization in AB test editing, displaying an error if any are invalid. Traffic split slider logic is updated to check minimum percentage constraints before applying changes, preventing invalid states.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (2)
135-149: Consider aligning validation with normalization logic.
validateUrlsonly adds the protocol but doesn't trim or remove trailing slashes before validation, whereas the submit handlers usenormalizeForForm(which does both). While this works in practice—invalid URLs after normalization would still fail—it creates inconsistency.For cleaner validation, consider:
const validateUrls = () => { if (!testVariants || testVariants.length <= 1) return false; return testVariants.every((test) => { if (!test.url || test.url.trim() === "") return false; - - // Check if it's a valid URL (with or without protocol) - const urlToValidate = test.url.startsWith("http") - ? test.url - : `https://${test.url}`; - - return isValidUrl(urlToValidate); + + // Use the same normalization as submit-time + const normalizedUrl = normalizeForForm(test.url); + return isValidUrl(normalizedUrl); }); };This ensures the button is only enabled when URLs will pass post-normalization validation.
661-723: Extract duplicated validation logic into a shared function.The onClick handler duplicates the normalization and validation logic from the form's
onSubmithandler (lines 271-331). Both paths:
- Validate empty URLs and total percentage
- Normalize with
normalizeForForm- Validate normalized URLs with
isValidUrl- Sync to parent form
Extract to a shared function to reduce duplication and improve maintainability:
const validateAndNormalizeTests = (currentTests: typeof testVariants) => { if (!currentTests || currentTests.length <= 1) return null; const totalPercentage = currentTests.reduce( (sum, test) => sum + test.percentage, 0, ); if (totalPercentage !== 100) { toast.error("Total percentage must equal 100%"); return null; } if (currentTests.some((test) => !test.url || test.url.trim() === "")) { toast.error("All test URLs must be filled"); return null; } const normalizedTests = currentTests.map((test) => ({ ...test, url: normalizeForForm(test.url || ""), })); if (normalizedTests.some((test) => !isValidUrl(test.url))) { toast.error("Please enter valid URLs"); return null; } return normalizedTests; };Then use it in both handlers:
const normalizedTests = validateAndNormalizeTests(currentTests); if (!normalizedTests) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/ui/modals/link-builder/ab-testing-modal.tsx(18 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (2)
packages/utils/src/functions/urls.ts (3)
getUrlFromString(10-18)isValidUrl(1-8)getPrettyUrl(130-138)apps/web/lib/zod/schemas/links.ts (1)
MIN_TEST_PERCENTAGE(29-29)
⏰ 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 (6)
apps/web/ui/modals/link-builder/ab-testing-modal.tsx (6)
52-54: LGTM: URL normalization is now robust.The
normalizeForFormhelper correctly trims whitespace before URL parsing and strips trailing slashes, addressing the previous review concerns.
73-79: Clarify the purpose of this component.
ABTestingModalInneris a pass-through that simply rendersABTestingEdit. SinceABTestingComplete(defined at lines 732-786) is never rendered in the modal flow, this suggests either:
- Incomplete implementation:
ABTestingModalInnershould route between Edit/Complete views based on test status- Unnecessary abstraction: This component can be removed and
ABTestingEditrendered directly inABTestingModalWhich was intended? If routing logic is planned but not yet implemented, consider adding a conditional to show
ABTestingCompletewhen the test is finished.
424-449: Excellent: All previous onBlur issues resolved.The blur handler now correctly:
- Trims whitespace before parsing
- Normalizes using
normalizeForForm- Passes
shouldValidate,shouldTouch, andshouldDirtytosetValue- Syncs changes to parent URL when
index === 0This comprehensively addresses all four concerns from the previous review.
732-786: Clarify usage of ABTestingComplete component.This component is fully implemented but never rendered in the modal flow. Given the PR objective "display of stats when ending a test early and selecting a winner," this appears to be an incomplete integration.
Should
ABTestingModalInnerconditionally renderABTestingCompletewhen the test has finished? For example:function ABTestingModalInner({ setShowABTestingModal, }: { setShowABTestingModal: Dispatch<SetStateAction<boolean>>; }) { const { watch } = useFormContext<LinkFormData>(); const testCompletedAt = watch("testCompletedAt"); const isComplete = testCompletedAt && new Date() > new Date(testCompletedAt); return isComplete ? ( <ABTestingComplete setShowABTestingModal={setShowABTestingModal} /> ) : ( <ABTestingEdit setShowABTestingModal={setShowABTestingModal} /> ); }If this component is used elsewhere or the routing is intentionally deferred, clarify with a comment.
916-950: Excellent fix: Keyboard slider bounds checking is now correct.The handler now computes proposed values (
nextLeft,nextRight) before checking bounds, preventing arrow keys from pushing percentages belowMIN_TEST_PERCENTAGE. This resolves the issue flagged in the previous review.
993-1001: Well-implemented accessibility for traffic split slider.The slider includes comprehensive keyboard navigation (arrow keys) and ARIA attributes (
role="slider",aria-valuenow,aria-valuemin,aria-valuemax,aria-label), making it fully accessible to screen reader users and keyboard-only navigation.
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
Removed