Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Prevent task duplication when 'Save all tasks' is clicked multip… #998

Merged
merged 7 commits into from
Dec 22, 2024

Conversation

AnoukRImola
Copy link
Contributor

@AnoukRImola AnoukRImola commented Dec 20, 2024

…le times

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

x Bugfix

  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Testing
  • Other (please describe):

Resolves: #985

Other information

Description

This PR fixes the task duplication issue when clicking "Save all tasks" multiple times without refreshing the page.

Changes Made

  1. Modified handleCreateTask in app/admin/quests/create/page.tsx to:

    • Filter unsaved steps using step.data.id
    • Update step IDs after successful creation
    • Prevent duplicate creation of already saved tasks
  2. Used existing checkStepChanges mechanism in app/admin/quests/dashboard/[questId]/page.tsx which properly:

    • Separates tasks into updated, removed, and added categories
    • Prevents duplicate task creation during quest editing
    • Maintains proper state between saves

Testing

  • Created new quests with multiple tasks
  • Verified tasks are saved correctly on first save
  • Confirmed clicking "Save all tasks" multiple times doesn't create duplicates
  • Tested task editing functionality remains intact

Video

video.mp4

Close #985

Summary by CodeRabbit

  • New Features

    • Enhanced task creation process for quests, allowing multiple tasks to be created efficiently.
    • Improved error handling to ensure all valid tasks are processed, even if some are invalid.
    • Updated state management to reflect the latest changes in task data.
    • Implemented filtering for unsaved steps to streamline task processing.
    • Added prevention for concurrent task creation to improve reliability.
  • Bug Fixes

    • Resolved issues with retaining task IDs for future reference after creation.

Copy link

vercel bot commented Dec 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
starknet-quest ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 22, 2024 9:17am

Copy link

vercel bot commented Dec 20, 2024

@AnoukRImola is attempting to deploy a commit to the LFG Labs Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

coderabbitai bot commented Dec 20, 2024

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 eslint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

app/admin/quests/create/page.tsx

Oops! Something went wrong! :(

ESLint: 9.17.0

ESLint couldn't find an eslint.config.(js|mjs|cjs) file.

From ESLint v9.0.0, the default configuration file is now eslint.config.js.
If you are using a .eslintrc.* file, please follow the migration guide
to update your configuration file to the new format:

https://eslint.org/docs/latest/use/configure/migration-guide

If you still have problems after following the migration guide, please stop by
https://eslint.org/chat/help to chat with the team.

Walkthrough

The changes enhance the task creation process in the admin quest creation page by modifying the handleCreateTask function. Key updates include the introduction of concurrency control to prevent multiple task creation attempts, filtering of unsaved steps to focus on tasks needing creation, improved error handling to allow processing of all valid tasks, refined task creation logic to ensure task IDs are stored correctly, and updated state management to reflect changes accurately.

Changes

File Change Summary
app/admin/quests/create/page.tsx - Added useRef variable to prevent concurrent task creation
- Implemented filtering for unsaved steps
- Improved error handling with continue instead of early return
- Updated task creation logic to store task IDs
- Enhanced state management with setSteps([...steps])

Assessment against linked issues

Objective Addressed Explanation
Prevent task duplication [#985]
Filter unsaved steps before processing [#985]
Add mechanism to prevent re-saving tasks [#985]

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🔭 Outside diff range comments (1)
app/admin/quests/create/page.tsx (1)

Line range hint 255-413: Standardize error handling across all task types

The error handling is inconsistent across different task types. Some use try-catch blocks while others don't, which could lead to uncaught errors and incomplete task creation.

  1. Wrap each task creation in try-catch:
 } else if (step.type === "Quiz") {
+  try {
     if (
       step.data.quiz_name?.length === 0 ||
       // ... other validations
     ) {
       showNotification("Please fill all fields for Quiz", "info");
       continue;
     }
     const response = await AdminService.createQuiz({
       // ... quiz creation
     });
     if (response) {
       step.data.id = response.id;
     }
+  } catch (error) {
+    console.error("Error while creating quiz task:", error);
+    showNotification(`Error adding Quiz task: ${error}`, "error");
+  }
 }
  1. Add JSON validation for Contract task:
 } else if (step.type === "Contract") {
   try {
+    let parsedCalls;
+    try {
+      parsedCalls = JSON.parse(step.data.contract_calls);
+    } catch (parseError) {
+      showNotification("Invalid JSON format in contract calls", "error");
+      continue;
+    }
     const response = await AdminService.createContract({
       quest_id: questId,
       name: step.data.contract_name,
       desc: step.data.contract_desc,
       href: step.data.contract_href,
       cta: step.data.contract_cta,
-      calls: JSON.parse(step.data.contract_calls),
+      calls: parsedCalls,
     });
🧹 Nitpick comments (1)
app/admin/quests/create/page.tsx (1)

251-254: Good addition of task filtering to prevent duplicates!

The filtering of unsaved steps is an effective solution to prevent task duplication. However, consider removing or conditionally enabling the console.log statement before deployment.

-    console.log("Steps to save:", unsavedSteps);
+    if (process.env.NODE_ENV === 'development') {
+      console.log("Steps to save:", unsavedSteps);
+    }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f6ccbf5 and 7c4d26d.

📒 Files selected for processing (1)
  • app/admin/quests/create/page.tsx (6 hunks)

app/admin/quests/create/page.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
app/admin/quests/create/page.tsx (1)

258-419: Consider adding network failure recovery

The task creation process could be improved to handle network failures more gracefully:

  1. Add retry logic for transient failures
  2. Implement rollback for partially completed task groups
  3. Save progress locally to allow recovery

Would you like me to provide a detailed implementation for any of these improvements?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7c4d26d and 1701cdf.

📒 Files selected for processing (1)
  • app/admin/quests/create/page.tsx (3 hunks)
🔇 Additional comments (2)
app/admin/quests/create/page.tsx (2)

3-3: LGTM: Good addition of isSaving ref for preventing concurrent saves

The addition of useRef and initialization of isSaving is a good approach to prevent race conditions during task creation.

Also applies to: 26-26


252-256: LGTM: Effective race condition prevention

The implementation correctly prevents concurrent task creation attempts by using both the isSaving ref and button loading state.

app/admin/quests/create/page.tsx Outdated Show resolved Hide resolved
app/admin/quests/create/page.tsx Outdated Show resolved Hide resolved
app/admin/quests/create/page.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
app/admin/quests/create/page.tsx (1)

283-294: 🛠️ Refactor suggestion

Improve quiz questions error handling

The quiz questions creation silently catches errors for individual questions without proper error aggregation or user feedback.

As suggested in a previous review, implement error aggregation and user feedback:

+ let failedQuestions = [];
  for (const question of step.data.questions) {
    try {
      await AdminService.createQuizQuestion({
        quiz_id: response.quiz_id,
        question: question.question,
        options: question.options,
        correct_answers: question.correct_answers,
      });
    } catch (error) {
      console.error("Error executing promise:", error);
+     failedQuestions.push(question.question);
    }
  }
+ if (failedQuestions.length > 0) {
+   showNotification(
+     `Failed to create ${failedQuestions.length} questions. Please review and try again.`,
+     "warning"
+   );
+ }
🧹 Nitpick comments (1)
app/admin/quests/create/page.tsx (1)

427-429: Consider partial success handling

The state is updated unconditionally after processing all steps, without considering partial failures.

Consider implementing a mechanism to track successful and failed tasks, and provide appropriate feedback to users:

+ const successfulSteps = [];
+ const failedSteps = [];
  for (const step of unsavedSteps) {
    try {
      // ... task creation logic ...
+     successfulSteps.push(step);
    } catch (error) {
+     failedSteps.push(step);
    }
  }
+ if (failedSteps.length > 0) {
+   showNotification(`${failedSteps.length} tasks failed to save. Please try again.`, "warning");
+ }
+ if (successfulSteps.length > 0) {
+   setSteps([...steps]);
+   setCurrentPage((prev) => prev + 1);
+ }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1701cdf and c8a7858.

📒 Files selected for processing (1)
  • app/admin/quests/create/page.tsx (3 hunks)
🔇 Additional comments (3)
app/admin/quests/create/page.tsx (3)

26-26: LGTM! Good addition of concurrent operation protection

The isSaving ref effectively prevents race conditions from multiple rapid clicks, complementing the existing buttonLoading state.


258-259: LGTM! Good optimization to filter unsaved steps

The filtering of unsaved steps prevents unnecessary API calls for already saved tasks, effectively addressing the task duplication issue.


431-434: LGTM! Proper cleanup in finally block

The cleanup in the finally block ensures that the saving state is properly reset regardless of success or failure.

app/admin/quests/create/page.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@Marchand-Nicolas Marchand-Nicolas left a comment

Choose a reason for hiding this comment

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

Really good job, appreciate the video!
Could you fix the conflicts so that I can merge please ?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
app/admin/quests/create/page.tsx (2)

437-443: Optimize state updates

The state updates and page navigation could be more efficient.

Consider this optimization:

- setSteps([...steps]);
- setCurrentPage((prev) => prev + 1);
+ const updatedSteps = [...steps];
+ setSteps(updatedSteps);
+ setCurrentPage((prev) => prev + 1);

536-536: Remove unnecessary eslint disable comment

Instead of using an empty function with an eslint disable comment, consider using null or removing the prop if it's optional.

- // eslint-disable-next-line @typescript-eslint/no-empty-function
- setShowDomainPopup={() => {}}
+ setShowDomainPopup={null}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c8a7858 and b678074.

📒 Files selected for processing (1)
  • app/admin/quests/create/page.tsx (4 hunks)
🔇 Additional comments (3)
app/admin/quests/create/page.tsx (3)

3-9: LGTM: Good implementation of concurrent save prevention

The addition of useRef and isSaving flag effectively prevents task duplication by blocking concurrent save operations.

Also applies to: 32-32


257-261: LGTM: Effective prevention of concurrent saves

The combination of isSaving check and setButtonLoading provides robust protection against multiple simultaneous saves.


284-297: 🛠️ Refactor suggestion

Enhance quiz question creation error handling

The current implementation silently catches errors for individual questions without proper error aggregation or user feedback.

Consider implementing this approach:

+ let failedQuestions = [];
  for (const question of step.data.questions) {
    try {
      await AdminService.createQuizQuestion({
        quiz_id: response.quiz_id,
        question: question.question,
        options: question.options,
        correct_answers: question.correct_answers,
      });
    } catch (error) {
      console.error("Error executing promise:", error);
+     failedQuestions.push(question.question);
+     showNotification(`Failed to create question: ${error}`, "error");
    }
  }
+ if (failedQuestions.length > 0) {
+   throw new Error(`Failed to create ${failedQuestions.length} questions`);
+ }

Likely invalid or redundant comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (5)
app/admin/quests/create/page.tsx (5)

263-263: Clarify the log message
The console log "Steps to save:" provides useful debug info. Consider enhancing these logs with more context if needed for debugging.


280-311: Validate potential empty quiz questions array
You gracefully handle empty quiz field data but also consider if step.data.questions might be undefined or an empty array. Skipping iteration when questions are missing may avoid runtime errors.


312-330: Consistent notification style across tasks
The user notifications for missing fields in “TwitterFw” (lines 312-322) and other tasks vary slightly. For completeness, ensure all tasks maintain consistent messages in form and style.


395-408: Ensure coverage and thorough error reporting
You’ve wrapped “createBalance” in a try/catch, but the catch block logs the error and notifies only in the console. Consider using showNotification to elevate user awareness in case of failure, matching the other tasks.


549-549: Future-proof your domain popup
This no-op domain popup handler might be extended or removed if domain management is out of scope. Keeping it is fine, but consider clarifying it with a comment or removing unused code.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b678074 and 384f1f1.

📒 Files selected for processing (1)
  • app/admin/quests/create/page.tsx (4 hunks)
🔇 Additional comments (5)
app/admin/quests/create/page.tsx (5)

32-32: Good addition to prevent concurrent saves
Using a ref-based guard variable is an effective solution to prevent multiple concurrent "Save all tasks" actions. This aligns well with the PR objective of avoiding task duplication.


409-423: Good job handling CustomApi tasks
This block consistently handles errors by logging and not interrupting the loop. Nicely done!


424-447: Robust error handling for contract tasks
The inline JSON.parse try-catch effectively prevents user input from crashing the entire flow. Good approach!


451-453: Check side effects from setSteps and setCurrentPage
The function updates the steps array and immediately increments the currentPage. Verify that subsequent re-renders after setSteps don’t introduce timing issues.

✅ Verification successful

Let me gather more context about the steps array and its usage.


Let me search for the complete context around where these state updates are happening.


Based on the context gathered, I can now provide the final response:

No timing issues found with state updates
The state updates are safe as they occur sequentially within the same synchronous block after all async operations are complete. The spread operator creates a new array reference, ensuring proper re-render, and the currentPage update using the functional form prevents any race conditions. Both updates are wrapped in a try-finally block that properly manages loading states.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Attempt to locate possible double increments or immediate re-calls of handleCreateTask
rg -A 3 "setCurrentPage"

Length of output: 4600


Script:

#!/bin/bash
# Search for steps state and its usage
rg -A 5 "const \[steps, setSteps\]"

# Search for handleCreateTask function implementation
ast-grep --pattern 'const handleCreateTask = $_ => {
  $$$
}'

Length of output: 788


Script:

#!/bin/bash
# Search for the context around where setSteps and setCurrentPage are used together
rg -B 10 -A 10 "setSteps\(\[\.\.\.steps\]\);"

Length of output: 1267


257-258: Avoid potential race condition on re-render
Since this ref is only used to block duplicate saves, ensure that any other re-render triggers or side effects cannot reset or circumvent this ref in exceptional cases.

Run the following script to confirm there are no hidden set/reset calls to “isSaving.current” beyond these lines:

✅ Verification successful

Let me check the full context around these usages to ensure proper handling.


Let me check how isSaving is initialized and if there are any other effects that might interfere with it.


Let me try a broader search to find the initialization and any effects.


Race condition protection is properly implemented
The isSaving ref is correctly initialized at component mount and only modified within the handleCreateTask callback. The ref's value is set to true before the async operation and properly reset to false in the finally block. None of the useEffect hooks in the component interact with this ref, ensuring its state cannot be accidentally reset during re-renders or side effects.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching for any references to isSaving besides lines 257-258, 454
rg "isSaving\.current"

Length of output: 220


Script:

#!/bin/bash
# Get more context around the isSaving.current usages
rg "isSaving\.current" -B 3 -A 3

Length of output: 1008


Script:

#!/bin/bash
# Get the initialization of isSaving ref and any useEffect blocks
ast-grep --pattern 'const isSaving = useRef' -A 3
rg "useEffect.*\{.*isSaving" -A 5

Length of output: 87


Script:

#!/bin/bash
# Search for useRef initialization with more context
rg "const.*isSaving.*useRef" -B 2 -A 2
# Get a broader view of the file structure
rg "useRef|useEffect" app/admin/quests/create/page.tsx -B 1 -A 1

Length of output: 887

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
app/admin/quests/create/page.tsx (3)

32-32: Use a more descriptive comment for clarity.
The comment at line 257 is in Spanish (“Prevenir múltiples guardados”). Consider adding or switching to an English explanation of why and how we guard against multiple saves for consistency with the rest of the code.


262-265: Avoid leaving console.log statements in production code.
While it’s helpful for debugging, consider removing or converting this log to a user-facing notification in production to avoid cluttering logs.

- console.log("Steps to save:", unsavedSteps);
+ // console.debug("Steps to save:", unsavedSteps);

442-443: Separate state update from navigation for clarity.
Calling both setSteps([...steps]) and setCurrentPage(...) in rapid succession may cause potential confusion when debugging. Consider batching or sequentially clarifying these calls if more complexity is added later.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 384f1f1 and 340daab.

📒 Files selected for processing (1)
  • app/admin/quests/create/page.tsx (4 hunks)
🔇 Additional comments (2)
app/admin/quests/create/page.tsx (2)

257-259: Good job preventing repeated submissions.
The early return if isSaving.current is true effectively stops multiple simultaneous submissions, aligning with the new concurrency controls. This addresses the duplication bug by blocking multiple attempts to create tasks in quick succession.


541-541: Confirm domain popup logic.
This line sets setShowDomainPopup to a no-op lambda. If domain settings are needed later, ensure this is replaced with a real handler or removed to prevent confusion.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
app/admin/quests/create/page.tsx (1)

427-433: Optimize state updates and navigation

The state updates and navigation could be improved to prevent potential race conditions.

Consider this optimization:

- setSteps([...steps]); 
  
  } finally {
    isSaving.current = false;
    setButtonLoading(false);
-   setCurrentPage((prev) => prev + 1);
  }
+ 
+ // Move state updates and navigation outside finally block
+ setSteps([...steps]);
+ setCurrentPage((prev) => prev + 1);

This ensures that state updates and navigation occur only after all cleanup is complete.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 340daab and 7b9b57d.

📒 Files selected for processing (1)
  • app/admin/quests/create/page.tsx (4 hunks)
🔇 Additional comments (6)
app/admin/quests/create/page.tsx (6)

3-9: LGTM: Good addition of concurrency control

The addition of useRef and isSaving ref provides a reliable way to prevent concurrent task creation attempts.

Also applies to: 32-32


257-261: LGTM: Effective concurrency control implementation

The implementation properly prevents concurrent task creation attempts by using both the ref check and button loading state.


263-264: LGTM: Efficient filtering of unsaved steps

The filtering of unsaved steps ensures that only new tasks are processed, preventing duplication.


417-417: ⚠️ Potential issue

Add error handling for JSON parsing

The JSON.parse(step.data.contract_calls) could throw if the input is invalid JSON.

Add proper error handling:

- calls: JSON.parse(step.data.contract_calls),
+ calls: (() => {
+   try {
+     return JSON.parse(step.data.contract_calls);
+   } catch (error) {
+     showNotification("Invalid contract calls format", "error");
+     throw error;
+   }
+ })(),

Likely invalid or redundant comment.


303-423: 🛠️ Refactor suggestion

Standardize error handling across all task types

The error handling is inconsistent across different task types:

  • Contract and CustomApi tasks have detailed error handling with notifications
  • Balance task has basic error logging
  • Other tasks (Quiz, TwitterFw, TwitterRw, Discord, Custom, Domain) lack try-catch blocks

Wrap each task creation in a try-catch block with proper error notifications:

 } else if (step.type === "TwitterFw") {
+  try {
     if (
       step.data.twfw_name?.length === 0 ||
       step.data.twfw_desc?.length === 0 ||
       step.data.twfw_username?.length === 0
     ) {
       showNotification("Please fill all fields for Twitter Follow", "info");
       continue;
     }
     const response = await AdminService.createTwitterFw({
       quest_id: questId,
       name: step.data.twfw_name,
       desc: step.data.twfw_desc,
       username: step.data.twfw_username,
     });
     if (response) step.data.id = response.id;
+  } catch (error) {
+    console.error("Error while creating Twitter Follow task:", error);
+    showNotification(`Error adding Twitter Follow task: ${error}`, "error");
+  }
 }

Likely invalid or redundant comment.


288-299: 🛠️ Refactor suggestion

Improve error handling for quiz questions

The current implementation silently catches errors for individual questions without proper error aggregation or user feedback.

Consider collecting failed questions and showing a summary:

+ let failedQuestions = [];
  for (const question of step.data.questions) {
    try {
      await AdminService.createQuizQuestion({
        quiz_id: response.quiz_id,
        question: question.question,
        options: question.options,
        correct_answers: question.correct_answers,
      });
    } catch (error) {
      console.error("Error executing promise:", error);
+     failedQuestions.push(question.question);
    }
  }
+ if (failedQuestions.length > 0) {
+   showNotification(
+     `Failed to create ${failedQuestions.length} questions. Please review and try again.`,
+     "warning"
+   );
+ }

Likely invalid or redundant comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
app/admin/quests/create/page.tsx (2)

249-253: Guard against double-clicks by disabling the button earlier.

You already check isSaving.current to prevent multiple calls. Consider disabling the button as soon as the function is triggered and re-enabling it once completed – you have partially done so with setButtonLoading, but confirm that the button is effectively disabled at all times while isSaving.current is true.


307-447: Consolidate repeated logic for each step type creation.

Many task creations reuse a similar pattern: checking required fields, calling AdminService, and assigning step.data.id. Consolidating or standardizing this pattern (possibly via a factory or a single shared function receiving step.data) will reduce code duplication and maintenance overhead.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ec0a61 and ad23990.

📒 Files selected for processing (1)
  • app/admin/quests/create/page.tsx (3 hunks)
🔇 Additional comments (3)
app/admin/quests/create/page.tsx (3)

26-26: Prevent repeated saves: nice usage of isSaving.

Declaring a ref to track saving state addresses concurrency issues. This straightforward approach helps ensure that clicking the save button multiple times won’t duplicate tasks.


275-306: Quiz creation partially fails scenario.

When failing to create some quiz questions, you proceed with the rest. This is suitable if partial success is acceptable. Otherwise, consider bundling them into a single atomic operation if partial success is undesirable.


3-3: Ensure minimal re-renders by verifying dependencies in the useCallback.

You have introduced the import of useRef alongside other hooks. Double-check that all callbacks using state or refs include the correct dependencies. For instance, calls to “steps” or “questId” in useCallback should have them in dependency arrays (unless they are updated externally).

Comment on lines 254 to 267
try {
isSaving.current = true;
setButtonLoading(true);

const unsavedSteps = steps.filter(step => !step.data.id);
console.log("Steps to save:", unsavedSteps);

let failedQuestions = [];

for (const step of unsavedSteps) {
if (step.type === "Quiz") {
if (
step.data.quiz_name?.length === 0 ||
step.data.quiz_desc?.length === 0 ||
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor for consistent field validations.

You validate fields for each task type individually (e.g., Quiz fields). Consider extracting a shared validation helper or typed definition to streamline code and maintain consistent notifications.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ad23990 and 798cc9d.

📒 Files selected for processing (1)
  • app/admin/quests/create/page.tsx (4 hunks)
🔇 Additional comments (3)
app/admin/quests/create/page.tsx (3)

3-9: LGTM: Proper concurrency control implementation

The addition of useRef for tracking the saving state is a good approach to prevent multiple simultaneous task creation attempts.

Also applies to: 32-32


261-261: LGTM: Effective filtering of unsaved steps

The filtering of unsaved steps using step.data.id is a good approach to prevent task duplication.


447-450: LGTM: Proper cleanup in finally block

The cleanup of loading and saving states in the finally block ensures they're always reset, even if an error occurs.

Comment on lines +389 to +403
} else if (step.type === "Balance") {
try {
const response = await AdminService.createBalance({
quest_id: questId,
name: step.data.balance_name,
desc: step.data.balance_desc,
contracts: step.data.balance_contracts,
cta: step.data.balance_cta,
href: step.data.balance_href,
});
if (response) step.data.id = response.id;
} catch (error) {
console.error("Error while creating balance task:", error);
}
} else if (step.type === "CustomApi") {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Standardize error handling for Balance and CustomApi tasks

The error handling for Balance and CustomApi tasks only logs to console, while Contract tasks show notifications. This inconsistency could lead to poor user experience.

Add user notifications:

 } catch (error) {
   console.error("Error while creating balance task:", error);
+  showNotification(
+    `Error adding Balance task: ${error}`,
+    "error"
+  );
 }
 } catch (error) {
   console.error("Error while creating CustomApi task:", error);
+  showNotification(
+    `Error adding CustomApi task: ${error}`,
+    "error"
+  );
 }

Also applies to: 403-418

Comment on lines +382 to +389
} else if (step.type === "Domain") {
const response = await AdminService.createDomain({
quest_id: questId,
name: step.data.domain_name,
desc: step.data.domain_desc,
});
if (response) step.data.id = response.id;
} else if (step.type === "Balance") {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add validation for Domain task type

The Domain task type lacks field validation unlike other task types. This could lead to creation of incomplete tasks.

Add validation similar to other task types:

 } else if (step.type === "Domain") {
+  if (
+    step.data.domain_name?.length === 0 ||
+    step.data.domain_desc?.length === 0
+  ) {
+    showNotification("Please fill all fields for Domain", "info");
+    continue;
+  }
   const response = await AdminService.createDomain({
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} else if (step.type === "Domain") {
const response = await AdminService.createDomain({
quest_id: questId,
name: step.data.domain_name,
desc: step.data.domain_desc,
});
if (response) step.data.id = response.id;
} else if (step.type === "Balance") {
} else if (step.type === "Domain") {
if (
step.data.domain_name?.length === 0 ||
step.data.domain_desc?.length === 0
) {
showNotification("Please fill all fields for Domain", "info");
continue;
}
const response = await AdminService.createDomain({
quest_id: questId,
name: step.data.domain_name,
desc: step.data.domain_desc,
});
if (response) step.data.id = response.id;
} else if (step.type === "Balance") {

@Marchand-Nicolas Marchand-Nicolas merged commit d158e4d into lfglabs-dev:testnet Dec 22, 2024
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix duplicate tasks in the admin dashboard
2 participants