Fix: New banner step when editing a quest #1060
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@sandragcarrillo is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
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
npm warn config production Use WalkthroughThe pull request introduces a new "Banner" step in the quest editing workflow for administrators. This addition allows users to define banner details such as tag, title, description, call-to-action, href, and image before the preview step. The changes include creating a new Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
components/admin/taskSteps/bannerStep.tsx (2)
19-63: Potentially add client-side validation or length constraints.Since users can input text for multiple banner fields, you might want to include basic validation (character limits, required fields, URL validation for
hrefandimage, etc.) to ensure the banner data meets certain standards before being submitted.
67-67: Include unit test coverage for the new component.Consider adding a suite of tests to verify the banner step’s functionality, ensuring that interactions (input changes, etc.) are properly handled and that default/fallback values are correctly displayed.
Would you like me to generate a sample unit testing file for this component?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
components/admin/taskSteps/bannerStep.tsx(1 hunks)
| const BannerStep: FunctionComponent<BannerStepProps> = ({ | ||
| handleTasksInputChange, | ||
| step, | ||
| index, | ||
| }) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider defaulting to a fallback when banner is null or undefined.
If step.data.banner can be missing, this code will throw a runtime error when accessing its properties. Safeguard by providing a default object or verifying if the banner object is defined before rendering.
-const BannerStep: FunctionComponent<BannerStepProps> = ({
- handleTasksInputChange,
- step,
- index,
-}) => {
+const BannerStep: FunctionComponent<BannerStepProps> = (props) => {
+ const { handleTasksInputChange, step, index } = props;
+ const banner = step?.data?.banner || {
+ tag: "",
+ title: "",
+ description: "",
+ cta: "",
+ href: "",
+ image: ""
+ };
return (
<div className="flex flex-col gap-4 pt-2">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const BannerStep: FunctionComponent<BannerStepProps> = ({ | |
| handleTasksInputChange, | |
| step, | |
| index, | |
| }) => { | |
| const BannerStep: FunctionComponent<BannerStepProps> = (props) => { | |
| const { handleTasksInputChange, step, index } = props; | |
| const banner = step?.data?.banner || { | |
| tag: "", | |
| title: "", | |
| description: "", | |
| cta: "", | |
| href: "", | |
| image: "" | |
| }; |
| type BannerStepProps = { | ||
| handleTasksInputChange: ( | ||
| e: React.ChangeEvent<HTMLInputElement>, | ||
| index: number | ||
| ) => void; | ||
| step: StepMap; | ||
| index: number; | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Add explicit import for StepMap type and resolve ambiguous type definition
The StepMap type is not imported in bannerStep.tsx, and there are two different definitions in the codebase:
types/frontTypes.d.ts: Base type definitionapp/admin/quests/dashboard/[questId]/page.tsx: Extended type withidfield
Add an explicit import from types/frontTypes.d.ts to ensure type consistency and prevent potential type mismatches.
🔗 Analysis chain
Verify the StepMap type import.
It's unclear how StepMap is declared or imported; ensure that this type is properly defined or imported to avoid potential “cannot find name StepMap” errors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for StepMap type definition or import references.
# We'll look for StepMap definition or import statements across the codebase.
rg 'type StepMap\b|interface StepMap\b|import.*StepMap'
Length of output: 155
Script:
#!/bin/bash
# Get the type definitions and imports
echo "=== StepMap definition in types/frontTypes.d.ts ==="
rg -A 10 "type StepMap =" types/frontTypes.d.ts
echo -e "\n=== StepMap definition in app/admin/quests/dashboard/[questId]/page.tsx ==="
rg -A 10 "type StepMap =" "app/admin/quests/dashboard/[questId]/page.tsx"
echo -e "\n=== Imports in bannerStep.tsx ==="
rg "^import" "components/admin/taskSteps/bannerStep.tsx"
Length of output: 1859
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/admin/formSteps/BannerDetailsForm.tsx (1)
6-12: Prop Types are well-defined but consider adding validation.
TheBannerDetailsFormPropscovers most scenarios. However, if more robust runtime validation or stricter typing is required for each banner field, consider libraries such asyuporreact-hook-form.app/admin/quests/dashboard/[questId]/page.tsx (1)
817-830: IntegratingBannerDetailsFormis consistent with the new flow.
This block properly saves the banner data and shows a success notification. However, note that additional server-side validation or error handling might be beneficial if requests fail or partial data is submitted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/admin/quests/dashboard/[questId]/page.tsx(5 hunks)components/admin/formSteps/BannerDetailsForm.tsx(1 hunks)constants/admin.ts(1 hunks)types/backTypes.d.ts(1 hunks)
🔇 Additional comments (9)
components/admin/formSteps/BannerDetailsForm.tsx (3)
1-4: Imports look good.
All imported modules appear necessary and relevant to this new component.
14-71: No explicit input validation on user-entered data.
While the form is straightforward and uses placeholders, there is no sanitization or dynamic validation for the banner fields. This might lead to incomplete or invalid data if integrated into a production flow without additional checks.Would you like to see an example snippet showcasing minimal data validation for these fields?
73-73: Exporting the component is properly handled.
No issues found with the default export.constants/admin.ts (1)
12-12: Including "Banner" step in the form flow is consistent with the new design.
Ensure that any step-index-based logic is updated to reflect the insertion of the new step in the flow.types/backTypes.d.ts (1)
309-316: Addition of thebannerproperty supports the new form fields.
Defining this property as optional is helpful, but consider whether any fields (e.g.,title,image) should be mandatory at the type level if they are strictly required for the banner.app/admin/quests/dashboard/[questId]/page.tsx (4)
28-28: Importing the newBannerDetailsFormis aligned with the revised form flow.
Please confirm that all relevant components and type imports are consistently updated to handle the new step.
359-359: Ensure robust handling ofindexinhandleTasksInputChange.
This function may fail if theindexprovided is out of range or invalid. Consider adding safeguards to avoid potential runtime errors.
374-374: Potential parse ambiguity with non-numericstartTime.
UsingparseInt(startTime)may yieldNaNif users input invalid data. Consider adding numeric validation or fallback logic to handle errors gracefully.
707-708: Correct approach to gating the banner step, but triple-check the step indices.
Lines 707–708 introducebannerValidinitialization. Lines 714 and 716 expand theif/elsechain to accommodate the new step. Ensure no off-by-one errors in the step navigation logic across the codebase.Also applies to: 714-714, 716-716
sandragcarrillo
force-pushed
the
fix-1008
branch
from
813b1fd to
0a0ffb8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/admin/quests/dashboard/[questId]/page.tsx (1)
707-708: Consider adding type validation for banner fields.The banner validation checks for empty fields but doesn't validate the field types. Consider adding type validation to ensure the fields contain the correct data types.
- const bannerValid = !questInput.banner?.tag || !questInput.banner?.title || !questInput.banner?.description || !questInput.banner?.cta || !questInput.banner?.href || !questInput.banner?.image; + const bannerValid = !questInput.banner?.tag?.trim() || + !questInput.banner?.title?.trim() || + !questInput.banner?.description?.trim() || + !questInput.banner?.cta?.trim() || + !questInput.banner?.href?.trim() || + !questInput.banner?.image?.trim() || + !(questInput.banner?.href?.startsWith('http://') || questInput.banner?.href?.startsWith('https://'));Also applies to: 716-717
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/admin/quests/dashboard/[questId]/page.tsx(5 hunks)components/admin/formSteps/BannerDetailsForm.tsx(1 hunks)constants/admin.ts(1 hunks)types/backTypes.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- constants/admin.ts
- types/backTypes.d.ts
- components/admin/formSteps/BannerDetailsForm.tsx
🔇 Additional comments (2)
app/admin/quests/dashboard/[questId]/page.tsx (2)
28-28: LGTM!The import statement for the BannerDetailsForm component is correctly added.
723-723: LGTM!The banner dependency is correctly added to the
isButtonDisabledhook dependencies.
| await handleUpdateQuest(); | ||
| showNotification("Banner updated successfully", "success"); | ||
| setCurrentPage((prev) => prev + 1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state to prevent multiple submissions.
The banner update submission handler doesn't prevent multiple submissions while the update is in progress. This could lead to race conditions or duplicate updates.
Add the buttonLoading prop to the BannerDetailsForm component and use it to disable the submit button during updates.
| } else if (currentPage === 3) { | ||
| return ( | ||
| <BannerDetailsForm | ||
| setQuestInput={setQuestInput} | ||
| questInput={questInput} | ||
| handleQuestInputChange={handleQuestInputChange} | ||
| submitButtonDisabled={isButtonDisabled} | ||
| onSubmit={async () => { | ||
| await handleUpdateQuest(); | ||
| showNotification("Banner updated successfully", "success"); | ||
| setCurrentPage((prev) => prev + 1); | ||
| }} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add error handling for banner update.
The banner update logic lacks error handling. If the update fails, the user won't receive proper feedback and the form will still proceed to the next step.
onSubmit={async () => {
- await handleUpdateQuest();
- showNotification("Banner updated successfully", "success");
- setCurrentPage((prev) => prev + 1);
+ try {
+ setButtonLoading(true);
+ await handleUpdateQuest();
+ showNotification("Banner updated successfully", "success");
+ setCurrentPage((prev) => prev + 1);
+ } catch (error) {
+ showNotification("Failed to update banner. Please try again.", "error");
+ console.error("Error updating banner:", error);
+ } finally {
+ setButtonLoading(false);
+ }
}}
+ buttonLoading={buttonLoading}Committable suggestion skipped: line range outside the PR's diff.
|
Also don't forget to resolve conflicts @sandragcarrillo |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/admin/quests/dashboard/[questId]/page.tsx (1)
860-874:⚠️ Potential issueAdd loading state and error handling to banner update.
The banner update submission handler doesn't prevent multiple submissions while the update is in progress. This could lead to race conditions or duplicate updates.
Apply this diff to add proper loading state and error handling:
return ( <BannerDetailsForm setQuestInput={setQuestInput} questInput={questInput} handleQuestInputChange={handleQuestInputChange} submitButtonDisabled={isButtonDisabled} onSubmit={async () => { - await handleUpdateQuest(); - showNotification("Banner updated successfully", "success"); - setCurrentPage((prev) => prev + 1); + try { + setButtonLoading(true); + await handleUpdateQuest(); + showNotification("Banner updated successfully", "success"); + handleTabChange(currentPage + 1); + } catch (error) { + showNotification("Failed to update banner. Please try again.", "error"); + console.error("Error updating banner:", error); + } finally { + setButtonLoading(false); + } }} + buttonLoading={buttonLoading} /> ); }Note: Also replaced
setCurrentPagewithhandleTabChangeto maintain consistency with the URL state management.
🧹 Nitpick comments (1)
app/admin/quests/dashboard/[questId]/page.tsx (1)
749-750: Consider using optional chaining for banner validation.The current validation might throw if
questInput.banneris undefined. Consider using optional chaining to handle this case gracefully.- const bannerValid = !questInput.banner?.tag || !questInput.banner?.title || !questInput.banner?.description || !questInput.banner?.cta || !questInput.banner?.href || !questInput.banner?.image; + const bannerValid = !questInput.banner?.tag || + !questInput.banner?.title || + !questInput.banner?.description || + !questInput.banner?.cta || + !questInput.banner?.href || + !questInput.banner?.image;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/admin/quests/dashboard/[questId]/page.tsx(5 hunks)
🔇 Additional comments (2)
app/admin/quests/dashboard/[questId]/page.tsx (2)
28-28: LGTM!The import statement for the BannerDetailsForm component is correctly placed.
756-759: LGTM!The form step conditions are correctly updated to include the new banner step.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/admin/quests/dashboard/[questId]/page.tsx (2)
883-885:⚠️ Potential issueAdd loading state to prevent multiple submissions.
The banner update submission handler doesn't prevent multiple submissions while the update is in progress. This could lead to race conditions or duplicate updates.
875-889:⚠️ Potential issueAdd error handling for banner update.
The banner update logic lacks error handling. If the update fails, the user won't receive proper feedback and the form will still proceed to the next step.
🧹 Nitpick comments (2)
app/admin/quests/dashboard/[questId]/page.tsx (2)
360-374: Consider initializing banner state with a dedicated constant.The banner object initialization is embedded within the change handler. This could lead to inconsistency in the initial state across different parts of the component.
Extract the initial state to a constant:
+const INITIAL_BANNER_STATE = { + tag: "", + title: "", + description: "", + cta: "", + href: "", + image: "", +}; const handleQuestInputChange = useCallback( (e: React.ChangeEvent<HTMLInputElement>) => { const { name, value } = e.target; setQuestInput((prev) => ({ ...prev, banner: { - ...(prev.banner ?? { - tag: "", - title: "", - description: "", - cta: "", - href: "", - image: "", - }), + ...(prev.banner ?? INITIAL_BANNER_STATE), [name]: value, }, })); }, [] );
764-765: Improve banner validation readability.The banner validation is a long boolean expression that could be hard to maintain.
Consider extracting it to a separate function:
+const isBannerValid = (banner?: UpdateQuest['banner']) => { + if (!banner) return false; + return !banner.tag || !banner.title || !banner.description || + !banner.cta || !banner.href || !banner.image; +}; const isButtonDisabled = useMemo(() => { // ... other validation logic ... - const bannerValid = !questInput.banner?.tag || !questInput.banner?.title || - !questInput.banner?.description || !questInput.banner?.cta || - !questInput.banner?.href || !questInput.banner?.image; + const bannerValid = isBannerValid(questInput.banner); // ... rest of the function }, [/* dependencies */]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/admin/quests/dashboard/[questId]/page.tsx(6 hunks)
🔇 Additional comments (1)
app/admin/quests/dashboard/[questId]/page.tsx (1)
771-774: LGTM: Form step conditions are properly ordered.The conditions for different form steps are logically ordered and include the new banner step.
Resolves: #1008
Changes:
Summary by CodeRabbit
Release Notes
New Features
BannerDetailsFormfor configuring banner details, including fields for tag, title, description, CTA, link, and image.Improvements
This update enhances quest management by enabling detailed banner customization during the quest creation and editing process.