feat: add community promo toast and rename Forum to Community

[sonyccd]

Summary

• Community Promo Toast: New toast notification encouraging users to visit the community forum

  • Appears 3 days after first login for users without a linked Discourse account (forum_username is null)
  • Shows only once, persisted via localStorage
  • Stays open until user dismisses (X) or clicks "Visit Community"
  • Avoids overlap with PWA install banner
  • Includes test mode via localStorage.setItem('community-toast-test-mode', 'true') for easier testing

• Rename "Forum" → "Community": Updated navigation sidebar, profile modal, and associated tests

• Website Favicon: Added favicon.svg to all website HTML pages to match the app

Test plan

• Verify navigation shows "Community" instead of "Forum"
• Verify Profile modal shows "Community Username"
• Test toast appearance (use test mode: localStorage.setItem('community-toast-test-mode', 'true'))
• Verify toast persists until dismissed
• Verify PWA banner and community toast don't overlap
• Verify website pages show favicon
• Run full test suite: npm run test:run

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Review	Updated (UTC)
openhamprep	Ready	Preview, Comment	Jan 14, 2026 6:28pm

[supabase]

Updates to Preview Branch (feature/community-promo-toast) ↗︎

Deployments	Status	Updated
Database	✅	Wed, 14 Jan 2026 18:28:17 UTC
Services	✅	Wed, 14 Jan 2026 18:28:17 UTC
APIs	✅	Wed, 14 Jan 2026 18:28:17 UTC

Tasks are run on every commit but only new migration files are pushed.
Close and reopen this PR if you want to apply changes from existing seed or migration files.

Tasks	Status	Updated
Configurations	✅	Wed, 14 Jan 2026 18:28:17 UTC
Migrations	✅	Wed, 14 Jan 2026 18:28:18 UTC
Seeding	✅	Wed, 14 Jan 2026 18:28:19 UTC
Edge Functions	✅	Wed, 14 Jan 2026 18:28:21 UTC

---

View logs for this Workflow Run ↗︎.
Learn more about Supabase for Git ↗︎.

[sentry]

Codecov Report

❌ Patch coverage is 96.72131% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.91%. Comparing base (d890153) to head (df68641).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/hooks/use-toast.ts	0.00%	3 Missing ⚠️
src/hooks/useCommunityPromoToast.ts	99.08%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
+ Coverage   86.83%   86.91%   +0.07%     
==========================================
  Files         116      117       +1     
  Lines       14565    14684     +119     
  Branches     2238     2262      +24     
==========================================
+ Hits        12647    12762     +115     
- Misses       1918     1922       +4     

Flag	Coverage Δ
unittests	86.91% <96.72%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/components/AppLayout.tsx	98.26% <100.00%> (+0.11%)	⬆️
src/components/DashboardSidebar.tsx	97.15% <100.00%> (ø)
src/components/ProfileModal.tsx	72.48% <100.00%> (ø)
src/hooks/useCommunityPromoToast.ts	99.08% <99.08%> (ø)
src/hooks/use-toast.ts	91.40% <0.00%> (-2.20%)	⬇️

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Pull Request Review

Summary

This PR adds a community promotion toast notification, renames "Forum" to "Community" throughout the app, and adds favicons to website pages. Overall, the implementation is well-structured with excellent test coverage. I have some recommendations to improve code quality and address potential issues.

---

✅ Strengths

1. Excellent Test Coverage

• Comprehensive test suite (248 lines) covering all guard conditions
• Tests edge cases like PWA banner overlap, test mode, and re-render behavior
• Proper use of vitest fake timers for time-based logic
• Tests verify localStorage behavior correctly

2. Good Architecture

• Custom hook pattern keeps logic isolated and reusable
• Multiple guard clauses make conditions explicit and easy to understand
• Integration with existing toast system maintains consistency
• PWA banner awareness prevents UI conflicts

3. User Experience

• Toast persists until dismissed (duration: Infinity) - good UX choice
• 1.5s delay ensures page is loaded before showing toast
• Test mode makes development/QA easier
• Clean animations with proper semantic tokens

---

⚠️ Issues & Recommendations

Critical Issues

1. Memory Leak Risk in useEffect (useCommunityPromoToast.ts:110)

The useEffect has dependencies that will cause it to re-run, but hasScheduledRef prevents re-scheduling. However, each re-run creates a new timeout without clearing the old one.

Problem:

useEffect(() => {
  // ... guards ...
  timeoutRef.current = setTimeout(() => { /* ... */ }, 1500);
}, [userCreatedAt, forumUsername, isAuthenticated, isPWABannerShowing]);
// ⚠️ If deps change, this runs again, creating multiple timeouts

Solution:

useEffect(() => {
  // ... guards ...
  
  // Clear any existing timeout before scheduling a new one
  if (timeoutRef.current) {
    clearTimeout(timeoutRef.current);
  }
  
  hasScheduledRef.current = true;
  timeoutRef.current = setTimeout(() => { /* ... */ }, 1500);
}, [userCreatedAt, forumUsername, isAuthenticated, isPWABannerShowing]);

2. React createElement Usage is Unnecessarily Complex (useCommunityPromoToast.ts:77-101)

Using createElement with inline classNames and attributes makes the code hard to read and maintain. This goes against React best practices and the project's component patterns.

Problem:

• Hard to read nested createElement calls
• Inline styling logic mixed with structure
• Difficult to modify or extend
• Not following the project's pattern of extracting components

Solution:
Extract the toast content into a separate component:

// CommunityPromoToastContent.tsx
export function CommunityPromoToastContent({ onVisit }: { onVisit: () => void }) {
  return (
    <div className="flex items-center gap-3" role="status" aria-live="polite">
      <div className="p-2 rounded-lg bg-primary/10 shrink-0">
        <Users className="w-5 h-5 text-primary" aria-hidden />
      </div>
      <span className="font-semibold text-foreground">Join the Community!</span>
    </div>
  );
}

// In hook:
toast({
  title: <CommunityPromoToastContent onVisit={handleVisit} />,
  // ...
});

This aligns with the project's style and CLAUDE.md guidance to extract reusable pieces.

---

Code Quality Issues

3. Inconsistent Type Safety (use-toast.ts:153-157)

The custom onOpenChange handler in the toast hook has a subtle issue:

onOpenChange: (open) => {
  if (!open) dismiss();
  // Call the custom onOpenChange if provided
  props.onOpenChange?.(open);
}

When a user provides their own onOpenChange, both the built-in dismiss logic AND the custom handler run. This is the intended behavior here, but the order matters:

Issue: If props.onOpenChange throws an error or calls dismiss() again, it could cause unexpected behavior.

Recommendation: Add a comment explaining this is intentional, or consider calling the custom handler first:

onOpenChange: (open) => {
  // Always call custom handler first
  props.onOpenChange?.(open);
  // Then handle built-in dismiss
  if (!open) dismiss();
}

4. Duplicate localStorage Write (useCommunityPromoToast.ts:94, 105)

The localStorage key is set in TWO places:

1. When action button is clicked (line 94)
2. When toast is dismissed via X button (line 105)

While this works, clicking the action button will write to localStorage twice (onClick writes it, then onOpenChange writes it again when toast closes).

Recommendation: Remove line 94 and rely solely on onOpenChange:

action: createElement(ToastAction, {
  altText: 'Visit the Open Ham Prep community forum',
  className: '...',
  onClick: () => {
    window.open(COMMUNITY_URL, '_blank', 'noopener,noreferrer');
    // Remove this line - onOpenChange will handle it
    // localStorage.setItem(STORAGE_KEY, 'true');
  },
}, /* ... */)

---

Performance & Best Practices

5. Potential Performance Issue with Profile Query (AppLayout.tsx:33-46)

The profile query runs on every render when user exists. While TanStack Query caches this, it's queried in AppLayout which wraps the entire app.

Recommendation: Consider moving this query to a context provider or ensure it's properly memoized. The current implementation is acceptable but could be optimized if profile updates are infrequent.

6. Magic Numbers Should Be Constants (use-toast.ts:6)

const TOAST_REMOVE_DELAY = 1000000;

1 million milliseconds (~16.7 minutes) seems like it might be a placeholder? If this is intentional, add a comment explaining why. If it should actually be Infinity or a shorter duration, update it.

Current behavior: Toasts auto-remove after 16+ minutes even if user hasn't interacted.

7. Missing Error Handling for window.open (useCommunityPromoToast.ts:93)

window.open can be blocked by popup blockers or fail in certain environments. Consider handling this:

onClick: () => {
  const newWindow = window.open(COMMUNITY_URL, '_blank', 'noopener,noreferrer');
  if (!newWindow) {
    // Popup blocked - could show a toast or fallback
    toast({
      title: 'Popup Blocked',
      description: 'Please allow popups to visit the community.',
      variant: 'destructive',
    });
  }
  localStorage.setItem(STORAGE_KEY, 'true');
}

---

Testing Improvements

8. Test Coverage Gap: Action Button Click

Tests verify onOpenChange is called but don't test clicking the action button directly. Add a test:

it('sets localStorage when action button is clicked', () => {
  renderHook(() =>
    useCommunityPromoToast({
      userCreatedAt: getDateDaysAgo(5),
      forumUsername: null,
      isAuthenticated: true,
    })
  );

  vi.advanceTimersByTime(2000);
  
  const toastCall = mockToast.mock.calls[0][0];
  const actionElement = toastCall.action;
  
  // Simulate clicking the action button
  actionElement.props.onClick();
  
  expect(localStorage.getItem('community-toast-shown')).toBe('true');
});

9. Missing Test: Cleanup on Unmount

While there's a cleanup effect, there's no test verifying the timeout is cleared on unmount:

it('clears timeout on unmount', () => {
  const { unmount } = renderHook(() =>
    useCommunityPromoToast({
      userCreatedAt: getDateDaysAgo(5),
      forumUsername: null,
      isAuthenticated: true,
    })
  );

  unmount();
  vi.advanceTimersByTime(2000);
  
  // Toast should not be called after unmount
  expect(mockToast).not.toHaveBeenCalled();
});

---

Minor Issues

10. Toast Styling Changes May Affect Existing Toasts (toast.tsx:26)

The PR modifies the base toast styling (rounded-xl, padding, animation). This affects all toasts in the app, not just the community promo toast.

Changes:

• rounded-md → rounded-xl
• p-6 pr-8 → p-4
• Different animation (slide-in-from-bottom-5 vs slide-in-from-top-full)
• bg-background → bg-card

Recommendation: Document these as intentional design improvements OR revert and style only the community toast specifically. If these are intentional improvements, they should be mentioned in the PR description.

11. Toaster Layout Change (toaster.tsx:12-20)

The restructured layout moves the close button outside the title/description wrapper. This changes the layout for all toasts. Verify this doesn't break any existing toast usage in the app.

12. Website Favicon Accessibility (favicon.svg)

The favicon uses hardcoded #000000 (black) fill/stroke, which won't adapt to dark mode. Consider using currentColor or providing both light/dark variants.

---

🔒 Security Review

✅ No security concerns identified:

• window.open uses proper noopener,noreferrer
• localStorage usage is safe (client-side only, no sensitive data)
• No XSS risks (React handles escaping)
• External URL is a constant, not user-provided
• No authentication bypasses or privilege escalation

---

📊 Test Coverage Assessment

Current: Excellent (95%+ estimated)

• ✅ All guard conditions covered
• ✅ Edge cases tested
• ✅ Mock dependencies properly
• ⚠️ Missing action button click test
• ⚠️ Missing unmount cleanup test

---

🎯 Recommendations Priority

Must Fix Before Merge:

1. ❗ Fix timeout memory leak (Issue Bump glob from 10.4.5 to 10.5.0 in the npm_and_yarn group across 1 directory #1)
2. ⚠️ Decide on toast styling changes (Issue Fix weak questions not being removed after correct answers #10) - document or revert

Should Fix:

3. Refactor createElement to component (Issue Bump esbuild from 0.21.5 to 0.25.12 in the npm_and_yarn group across 1 directory #2)
4. Remove duplicate localStorage write (Issue Bump vite from 5.4.19 to 5.4.21 in the npm_and_yarn group across 1 directory #4)

Nice to Have:

5. Add missing tests (Issues Fix all ESLint errors to pass CI workflow #8, Remove stale unfurl-url function config #9)
6. Handle window.open failure (Issue Install and Configure Vercel Web Analytics #7)
7. Clarify TOAST_REMOVE_DELAY constant (Issue Install Vercel Speed Insights integration #6)

---

✨ Final Thoughts

This is a solid PR with excellent testing and user experience considerations. The main concerns are:

1. The timeout memory leak which should be fixed
2. The complex createElement usage that violates React best practices
3. Clarifying whether toast styling changes are intentional global improvements

The "Forum" → "Community" rename is clean and comprehensive. The website favicon addition is a nice touch, though consider making it theme-aware.

Overall Assessment: Approve with Changes ✅🔧

Great work on the comprehensive test coverage and thoughtful UX design! The PWA banner awareness and test mode features are particularly well done. 🎉

[claude]

Pull Request Review

This PR adds a community promo toast notification and renames "Forum" to "Community" throughout the app. Overall, this is a well-implemented feature with excellent test coverage and thoughtful UX considerations. Here's my detailed feedback:

✅ Strengths

1. Excellent Test Coverage (309 lines!)

The useCommunityPromoToast.test.ts file is exemplary:

• Covers all edge cases (authentication, timing, PWA overlap, localStorage, re-renders)
• Tests cleanup behavior (unmount handling)
• Includes popup blocker handling
• Test mode for easier development
• Uses fake timers properly

2. Thoughtful UX Design

• Avoids overlap with PWA install banner (src/hooks/useCommunityPromoToast.ts:46-49)
• 3-day delay prevents annoying new users immediately
• duration: Infinity ensures users see it (not auto-dismissed)
• Only shows once per user via localStorage
• Test mode for development

3. Clean Hook Implementation

• Proper use of refs to prevent re-render issues (hasScheduledRef, timeoutRef)
• Good guard clauses for readability
• Cleanup on unmount prevents memory leaks (src/hooks/useCommunityPromoToast.ts:123-129)

4. Accessibility Considerations

• role="status" and aria-live="polite" (src/hooks/useCommunityPromoToast.ts:84-85)
• aria-hidden="true" on decorative icons
• altText on action button (src/hooks/useCommunityPromoToast.ts:95)

5. Improved Toast UI

• Better visual design (rounded-xl, better spacing)
• Close button always visible (moved from hover-only to always-visible)
• Better layout with flex positioning

🔍 Issues & Suggestions

1. Critical: hasScheduledRef Never Resets ⚠️

Location: src/hooks/useCommunityPromoToast.ts:52-53, 72

Problem: Once hasScheduledRef.current = true is set, it never resets. This could cause issues:

• If the component unmounts and remounts (navigation, React Strict Mode), the toast won't show even if conditions change
• The ref persists across effect re-runs

Reproduction scenario:

1. User logs in → toast scheduled → hasScheduledRef = true
2. User navigates away → component unmounts → timeout cleared
3. User navigates back → component remounts → ref still true → toast never shows

Fix: Reset the ref when localStorage is set:

onOpenChange: (open: boolean) => {
  if (!open) {
    localStorage.setItem(STORAGE_KEY, 'true');
    hasScheduledRef.current = false; // Reset for future mounts
  }
},

Or check localStorage before the ref check:

// Guard: Toast already shown (check localStorage)
if (localStorage.getItem(STORAGE_KEY) === 'true') {
  hasScheduledRef.current = false; // Reset if already shown
  return;
}

// Guard: Already scheduled (prevents re-scheduling on re-renders)
if (hasScheduledRef.current) {
  return;
}

2. Toast UI: Close Button Positioning Issue

Location: src/components/ui/toast.tsx:70

The close button style was changed from absolute right-2 top-2 opacity-0 group-hover:opacity-100 to always visible. However:

• The new layout in toaster.tsx already positions the close button with flexbox
• Removing absolute positioning is good
• But you removed the opacity transition entirely

Consider: Users might prefer the hover-reveal pattern for less visual clutter. The current always-visible approach is fine but differs from common toast patterns.

3. Minor: Dependency Array Completeness

Location: src/hooks/useCommunityPromoToast.ts:120

The effect depends on computed values (like isTestMode) but only lists props. While this works, it could be clearer. Consider:

• Adding a comment explaining why only props are in the array
• Or accepting testMode as a prop for testability

4. Race Condition: Multiple Auth State Changes

Scenario: User profile loads after the component mounts:

1. Component mounts with user but profile = undefined
2. Effect runs, forumUsername = undefined, toast scheduled
3. Profile loads with forum_username = 'existing'
4. Effect re-runs but hasScheduledRef.current = true, so it doesn't cancel

Impact: Low - the guard checks at line 37 happen before scheduling, so this is unlikely. But worth noting.

5. Popup Blocker Handling Could Be More User-Friendly

Location: src/hooks/useCommunityPromoToast.ts:97-103

Currently logs to console. Consider:

• Showing a fallback toast: "Please allow popups" with a retry button
• Or copying the URL to clipboard as a fallback
• Or showing the URL in the toast so users can manually navigate

6. Code Style: createElement vs JSX

You're using createElement to avoid JSX in the hook. This is valid but less readable than:

• Moving the toast content to a separate component
• Using JSX (React allows JSX in non-component functions)

Example:

const CommunityToastContent = () => (
  <div className="flex items-center gap-3" role="status" aria-live="polite">
    <div className="p-2 rounded-lg bg-primary/10 shrink-0">
      <Users className="w-5 h-5 text-primary" aria-hidden="true" />
    </div>
    <span className="font-semibold text-foreground">Join the Community!</span>
  </div>
);

// Then in hook:
title: <CommunityToastContent />

7. Favicon: Missing viewBox Optimization

Location: website/favicon.svg:1

The SVG has width="24px" height="24px" which should be viewBox="0 0 24 24" only for scalability. Minor issue.

🎨 Design System Compliance

✅ Good: Uses semantic tokens:

• bg-primary/10, text-primary, bg-primary, text-primary-foreground
• text-foreground, text-muted-foreground
• bg-card, text-card-foreground, border-border

✅ Good: Toast styling uses design system tokens consistently

🔒 Security

✅ Good: Uses noopener,noreferrer when opening external links (src/hooks/useCommunityPromoToast.ts:98)

✅ Good: No XSS risks (all content is static strings)

✅ Good: localStorage keys are namespaced ("community-toast-shown")

📊 Performance

✅ Good: Uses refs to prevent unnecessary re-renders

✅ Good: Guards prevent expensive operations

✅ Good: Timeout cleanup prevents memory leaks

⚠️ Minor: The usePWAInstall hook is called on every render. Consider memoization if that hook is expensive.

🧪 Testing

✅ Excellent: Comprehensive test coverage

✅ Good: Uses fake timers properly

✅ Good: Tests cleanup behavior

✅ Good: Mocks external dependencies

Suggestion: Add a test for the scenario where profile loads after initial mount (async profile fetch):

it('does not show toast if forumUsername appears after mount', () => {
  const { rerender } = renderHook(
    ({ forumUsername }) => useCommunityPromoToast({ 
      userCreatedAt: getDateDaysAgo(5), 
      forumUsername, 
      isAuthenticated: true 
    }),
    { initialProps: { forumUsername: null } }
  );
  
  vi.advanceTimersByTime(500); // Partial delay
  
  // Profile loads with forum_username before timeout fires
  rerender({ forumUsername: 'user123' });
  
  vi.advanceTimersByTime(2000); // Complete delay
  expect(mockToast).not.toHaveBeenCalled();
});

📝 Naming Changes (Forum → Community)

✅ Good: Comprehensive renaming across all relevant files

✅ Good: Tests updated to match

✅ Good: Consistent terminology

🎯 Summary

Overall Assessment: High Quality (8/10)

This PR is well-implemented with excellent tests and thoughtful UX. The main issue is the hasScheduledRef never resetting, which could cause edge case bugs. The other issues are minor polish items.

Recommendation: Approve with minor fixes

Required Before Merge:

1. Fix hasScheduledRef reset issue

Optional Improvements:

2. Consider better popup blocker UX
3. Add test for async profile loading
4. Consider moving toast content to JSX component for readability

Great work overall! The test coverage is particularly impressive. 🎉

[claude]

Pull Request Review - PR #160

Summary

This PR adds a community promo toast notification system and renames "Forum" to "Community" throughout the app. The implementation is well-thought-out with excellent test coverage and attention to UX details.

---

✅ Strengths

1. Excellent Code Quality

• Clean separation of concerns: The useCommunityPromoToast hook is well-isolated and reusable (src/hooks/useCommunityPromoToast.ts:18-135)
• Comprehensive guard clauses: Multiple early returns prevent unnecessary logic execution (lines 31-54)
• Well-documented code: Clear comments explain the rationale behind design decisions
• TypeScript usage: Proper typing with explicit interfaces

2. Outstanding Test Coverage

The test file (src/hooks/useCommunityPromoToast.test.ts) is exceptional:

• 309 lines of comprehensive tests covering all edge cases
• Tests for authentication state, localStorage persistence, PWA banner collision avoidance
• Tests for popup blockers and cleanup on unmount
• Uses fake timers correctly for testing async behavior
• Achieves excellent coverage of the feature

3. Smart UX Considerations

• No overlap with PWA banner: Checks isPWABannerShowing to avoid competing UI elements (line 24, 46-49)
• Test mode for easier QA: community-toast-test-mode localStorage flag bypasses the 3-day wait (line 57)
• Persistent until dismissed: duration: Infinity ensures users see the message (line 93)
• Graceful popup blocker handling: Warns user but still marks as shown (lines 98-103)

4. Performance Optimizations

• Prevents duplicate renders: Uses useRef to track if toast is already scheduled (line 27, 52-53)
• Proper cleanup: Clears timeout on unmount to prevent memory leaks (lines 125-134)
• Efficient localStorage checks: Early exit if already shown (line 42-44)

5. Accessibility

• Proper ARIA attributes: role="status", aria-live="polite", aria-hidden="true" on icons (lines 84-85, 88, 108)
• altText on action button for screen readers (line 95)

6. Design System Compliance

• Follows project guidelines by using semantic tokens: bg-primary, text-foreground, bg-card, etc.
• Consistent with CLAUDE.md design system requirements

---

🔍 Observations & Minor Suggestions

1. Toast UI Refinements (src/components/ui/toast.tsx)

The toast styling updates are good, but consider:

• Line 26: The animation classes string is very long. Consider extracting to a variable for readability:

const baseClasses = "group pointer-events-auto relative flex...";
const animationClasses = "data-[state=open]:animate-in data-[state=open]:fade-in-0...";
const toastVariants = cva([baseClasses, animationClasses].join(' '), {

2. ToastClose Styling Change (src/components/ui/toast.tsx:70)

• Previous: Close button was subtle (opacity-0) and appeared on hover
• New: Close button is always visible with shrink-0
• This is actually an improvement for mobile UX (no hover state), but it's a behavioral change that might affect existing toasts. Consider documenting this in the PR description.

3. Toaster Layout Restructure (src/components/ui/toaster.tsx)

The new layout is cleaner, but the action button placement changed:

• Before: Action was a sibling to content
• After: Action is in flex justify-end container
  This could affect action button widths in other toasts. Verify all existing toast usages still look correct.

4. Hook Dependency Array (useCommunityPromoToast.ts:122)

The comment at lines 120-122 mentions isTestMode doesn't need to be in deps, which is correct since it's read from localStorage. However, consider if this could cause issues if someone changes test mode while the component is mounted. Current behavior is acceptable but worth noting.

5. Cleanup Effect Dependency Array (useCommunityPromoToast.ts:125)

The cleanup effect has an empty dependency array, which means it only runs on unmount. The comment suggests remounting allows re-scheduling, but hasScheduledRef.current = false in the cleanup means the ref is reset on unmount, not on re-render. This is correct but the comment could be clearer:

// Reset on unmount so the hook can schedule again if component remounts
// (only if localStorage wasn't set, preventing duplicate toasts)

---

🐛 Potential Issues

1. Race Condition with Profile Data (AppLayout.tsx:84-88)

The hook is called with profile?.forum_username, but profile data might load asynchronously. If profile loads after the initial render:

• First render: forumUsername = undefined → toast might be scheduled
• Second render (profile loaded): forumUsername = 'actual_value' → toast should be suppressed

Current behavior: The hasScheduledRef.current prevents re-scheduling on the second render, so if profile loads slowly, a user WITH a forum username might still see the toast.

Suggested fix: Add profile loading state to the conditions or ensure profile loads before AppLayout mounts. Check if useProfile() hook handles this correctly.

2. localStorage Timing Issue (useCommunityPromoToast.ts:113-116)

The onOpenChange callback sets localStorage when open = false. If a user refreshes the page before dismissing the toast, they'll see it again on next visit. This might be intentional, but consider if you want to set localStorage when the toast is shown rather than dismissed:

Current: Toast persists across page loads until user dismisses
Alternative: Toast shows once, even if page refreshed before dismissal

If current behavior is intended, it's fine. Just flagging for consideration.

3. Window.open Popup Blocker (useCommunityPromoToast.ts:98-103)

The popup blocker handling is good, but the toast stays open after onClick. The user might expect the toast to auto-close after clicking "Visit Community". Consider adding:

onClick: () => {
  const newWindow = window.open(COMMUNITY_URL, '_blank', 'noopener,noreferrer');
  if (!newWindow) {
    console.warn('Popup blocked. Please allow popups to visit the community forum.');
  }
  // Auto-dismiss toast after click (will trigger onOpenChange)
  return true; // or call dismiss() if available
}

Currently, clicking the button opens the link but leaves the toast visible. User must manually click X.

---

🔒 Security Review

✅ No Security Concerns Identified

• window.open uses proper noopener,noreferrer attributes (line 98)
• localStorage usage is safe (no sensitive data stored)
• External URL is hard-coded constant, no user input injection risk
• No XSS vectors: All content is static or from trusted sources

---

🎨 Design System Compliance

✅ Follows CLAUDE.md Guidelines

• Uses semantic tokens throughout (bg-primary, text-foreground, etc.)
• No direct color values
• Follows existing patterns for hooks and components
• TypeScript configuration matches project standards

Minor Note: Toast Styling

The toast now uses rounded-xl instead of rounded-md and different animations. Verify this matches your design system's intent for toast appearance.

---

🧪 Testing Assessment

Excellent Test Coverage ✅

The test suite is exemplary:

• 11 test cases covering all scenarios
• Tests guard clauses, localStorage, PWA interaction, popup blocking
• Uses proper mocking and fake timers
• Tests cleanup and memory leaks

Suggested Additional Test

Consider adding a test for the profile loading race condition mentioned above:

it('does not show toast if profile loads late with forum_username', () => {
  const { rerender } = renderHook(
    ({ forumUsername }) => useCommunityPromoToast({ 
      userCreatedAt: getDateDaysAgo(5), 
      forumUsername, 
      isAuthenticated: true 
    }),
    { initialProps: { forumUsername: null } }
  );
  
  vi.advanceTimersByTime(1000); // Before toast shows
  
  rerender({ forumUsername: 'late_loaded_user' });
  vi.advanceTimersByTime(1000); // After profile loads
  
  expect(mockToast).not.toHaveBeenCalled();
});

---

📝 Naming Changes Review

"Forum" → "Community" Rename ✅

• Consistent across all components (DashboardSidebar, ProfileModal)
• All tests updated correctly
• URL still points to forum.openhamprep.com (which is fine, URL doesn't need to match)

---

🌐 Website Favicon Addition

✅ Good Addition

• Favicon added to all HTML pages (about.html, faq.html, features.html, index.html)
• Uses SVG format (scalable, modern)
• Matches app branding

Note

Verify the SVG renders correctly across all browsers. Some older browsers might need a PNG fallback.

---

📊 Performance Impact

Minimal Performance Impact ✅

• Hook only runs checks, no expensive operations
• Single timeout scheduled per mount
• localStorage reads are fast
• No unnecessary re-renders due to useRef usage

---

🎯 Recommendations Priority

High Priority

1. Test profile loading race condition - Ensure users with forum_username never see the toast even if profile loads asynchronously

Medium Priority

2. Auto-dismiss toast after clicking action button - Better UX
3. Consider localStorage timing - Clarify if toast should persist across page refreshes

Low Priority (Optional)

4. Refactor long className strings - Improve readability
5. Add PNG favicon fallback - For older browsers

---

✅ Final Verdict

APPROVE with minor suggestions

This is high-quality work with exceptional attention to detail:

• ✅ Excellent test coverage (309 lines of tests!)
• ✅ Well-documented code
• ✅ Smart UX decisions (PWA banner avoidance, test mode)
• ✅ Follows project conventions
• ✅ No security issues
• ⚠️ One potential race condition to investigate (profile loading)
• 💡 Minor UX suggestion (auto-dismiss on action click)

The code is production-ready. The suggestions above are minor improvements that can be addressed in a follow-up PR if desired.

Great work! 🎉

---

Reviewed with Claude Code