Skip to content

Night Shift: fix timeout memory leaks in dashboard#15

Open
EtanHey wants to merge 1 commit intomasterfrom
nightshift/2026-02-17-7524
Open

Night Shift: fix timeout memory leaks in dashboard#15
EtanHey wants to merge 1 commit intomasterfrom
nightshift/2026-02-17-7524

Conversation

@EtanHey
Copy link
Owner

@EtanHey EtanHey commented Feb 17, 2026

User description

Automated improvement by GolemsZikaron Night Shift.

fix timeout memory leaks in dashboard


PR Type

Bug fix


Description

  • Fix uncleaned setTimeout in migration modal causing memory leaks

  • Add proper cleanup for goals initialization retry logic

  • Cap goals retry attempts at 3 with exponential backoff

  • Prevent state updates on unmounted components


Diagram Walkthrough

flowchart LR
  A["Migration Modal<br/>setTimeout"] -->|cleanup added| B["clearTimeout<br/>on unmount"]
  C["Goals Retry<br/>unbounded"] -->|capped at 3| D["Exponential<br/>backoff"]
  D -->|tracked via ref| E["Proper cleanup<br/>on unmount"]
Loading

File Walkthrough

Relevant files
Bug fix
dashboard.tsx
Fix memory leaks from uncleaned timeouts                                 

src/routes/_authed/dashboard.tsx

  • Added useRef import for tracking retry state
  • Wrapped migration modal setTimeout with cleanup function
  • Introduced goalsRetryRef to track retry count and timer ID
  • Added useEffect hook to clean up pending retry timers on unmount
  • Modified goals initialization error handler to cap retries at 3
    attempts
  • Implemented exponential backoff delay (2000ms * retry count)
  • Reset retry counter on successful goals initialization
+24/-7   

Fix two uncleaned setTimeout calls in the dashboard that could cause
state updates on unmounted components:

1. Migration modal timeout (500ms) now has proper cleanup
2. Goals initialization retry now uses a ref for cleanup on unmount,
   caps retries at 3 with increasing backoff (was unbounded/recursive)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@vercel
Copy link

vercel bot commented Feb 17, 2026

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

Project Deployment Actions Updated (UTC)
songscript Ready Ready Preview, Comment Feb 17, 2026 2:10am

@coderabbitai
Copy link

coderabbitai bot commented Feb 17, 2026

Warning

Rate limit exceeded

@EtanHey has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 13 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 9a964b0 and 46d89c9.

📒 Files selected for processing (1)
  • src/routes/_authed/dashboard.tsx
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch nightshift/2026-02-17-7524

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Brittle error retry: The retry logic relies on substring-matching error.message for Authentication required and
otherwise only warns, which may miss other failure modes or non-string error shapes
without providing a robust fallback.

Referred Code
onError: (error) => {
  console.warn('Initialize goals failed:', error);
  // Retry up to 3 times with increasing delay if authentication error
  if (error.message?.includes('Authentication required') && goalsRetryRef.current.count < 3) {
    goalsRetryRef.current.count++;
    const delay = 2000 * goalsRetryRef.current.count;
    goalsRetryRef.current.timerId = setTimeout(() => {
      goalsRetryRef.current.timerId = null;
      initializeGoals({});
    }, delay);
  }

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Raw error logging: console.warn('Initialize goals failed:', error) logs the full error object
unstructured, which could inadvertently include sensitive details depending on what the
thrown error contains.

Referred Code
onError: (error) => {
  console.warn('Initialize goals failed:', error);
  // Retry up to 3 times with increasing delay if authentication error

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

const timer = setTimeout(() => {
setIsMigrationModalOpen(true);
}, 500);
return () => clearTimeout(timer);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migration timer cleared before it can fire

Medium Severity

The cleanup returned from the migration useEffect clears the setTimeout created for opening the modal, but the same effect also calls setMigrationChecked(true). That state change re-runs the effect immediately, so the previous cleanup runs and cancels the timer in dashboard.tsx before setIsMigrationModalOpen(true) executes.

Additional Locations (2)

Fix in Cursor Fix in Web

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Keep modal timer alive until fire

Refactor the useEffect for the migration modal by moving
setMigrationChecked(true) into the setTimeout callback and removing
migrationChecked from the dependency array to ensure the timer fires correctly.

src/routes/_authed/dashboard.tsx [127-138]

 useEffect(() => {
-  if (session?.user?.id && !migrationChecked) {
-    setMigrationChecked(true);
-    if (shouldShowMigrationModal(session.user.id)) {
-      const timer = setTimeout(() => {
-        setIsMigrationModalOpen(true);
-      }, 500);
-      return () => clearTimeout(timer);
-    }
+  if (session?.user?.id && shouldShowMigrationModal(session.user.id)) {
+    const timer = setTimeout(() => {
+      setMigrationChecked(true);
+      setIsMigrationModalOpen(true);
+    }, 500);
+    return () => clearTimeout(timer);
   }
-}, [session?.user?.id, migrationChecked]);
+}, [session?.user?.id]);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a critical bug in the useEffect hook's logic. The current implementation would cause the timeout to be cleared immediately, preventing the migration modal from ever appearing. The proposed fix is correct and necessary for the feature to work.

High
Prevent multiple concurrent retry timeouts

In the onError handler for initializeGoals, clear any existing retry timeout
before scheduling a new one to prevent multiple concurrent retries and potential
race conditions.

src/routes/_authed/dashboard.tsx [205-216]

 onError: (error) => {
   console.warn('Initialize goals failed:', error);
   // Retry up to 3 times with increasing delay if authentication error
   if (error.message?.includes('Authentication required') && goalsRetryRef.current.count < 3) {
+    if (goalsRetryRef.current.timerId) {
+      clearTimeout(goalsRetryRef.current.timerId);
+    }
     goalsRetryRef.current.count++;
     const delay = 2000 * goalsRetryRef.current.count;
     goalsRetryRef.current.timerId = setTimeout(() => {
       goalsRetryRef.current.timerId = null;
       initializeGoals({});
     }, delay);
   }
 },
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a race condition in the new retry logic where multiple timeouts could be scheduled concurrently. Clearing the previous timer before setting a new one is the correct fix and improves the robustness of the error handling.

Medium
High-level
Simplify mutation hook usage

The implementation unnecessarily wraps a useConvexMutation hook within a
useMutation hook. It is recommended to remove the redundant useMutation wrapper
and pass the configuration options directly to useConvexMutation.

Examples:

src/routes/_authed/dashboard.tsx [199-217]
  const { mutate: initializeGoals } = useMutation({
    mutationFn: (args: any) => initializeDefaultGoalsMutation(args),
    onSuccess: () => {
      goalsRetryRef.current.count = 0;
      refetchGoals();
    },
    onError: (error) => {
      console.warn('Initialize goals failed:', error);
      // Retry up to 3 times with increasing delay if authentication error
      if (error.message?.includes('Authentication required') && goalsRetryRef.current.count < 3) {

 ... (clipped 9 lines)

Solution Walkthrough:

Before:

const initializeDefaultGoalsMutation = useConvexMutation(api.goals.initializeDefaultGoals);

// ...

const { mutate: initializeGoals } = useMutation({
  mutationFn: (args: any) => initializeDefaultGoalsMutation(args),
  onSuccess: () => {
    // ...
  },
  onError: (error) => {
    // ... retry logic
  },
});

After:

const { mutate: initializeGoals } = useConvexMutation(api.goals.initializeDefaultGoals, {
  onSuccess: () => {
    // ...
  },
  onError: (error) => {
    // ... retry logic
  },
});
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a redundant wrapper (useMutation around useConvexMutation), and the proposed refactoring significantly simplifies the code and aligns it with the library's intended usage.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant