Wow out of stock #521
Wow out of stock #521
Conversation
WalkthroughThe pull request introduces a new navigation link titled "Out of Stock" within the "World of Warcraft" section of the sidebar, directing users to the URL Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
🪧 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 (
|
Deploying saddlebag-with-pockets with
|
| Latest commit: |
c86ddcf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d05c05b2.saddlebag-with-pockets.pages.dev |
| Branch Preview URL: | https://wow-out-of-stock.saddlebag-with-pockets.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
app/requests/WoW/OutOfStock.ts (2)
4-15: Add JSDoc comments and consider adding type constraints.The interface properties would benefit from documentation explaining their purpose and valid ranges. Additionally, consider using more specific types for numeric fields.
Example improvement:
interface WoWOutOfStockProps { + /** The WoW server region to query */ region: WoWServerRegion + /** Minimum sales per day threshold (must be positive) */ - salesPerDay: number + salesPerDay: Positive<number> + /** Minimum average price threshold in gold */ - avgPrice: number + avgPrice: Positive<number> // ... add similar comments for other fields } +type Positive<T extends number> = number extends T ? never : T
1-67: Consider implementing a service layer pattern.The current file directly handles API communication. Consider implementing a service layer pattern to:
- Centralize error handling and response parsing
- Add retry logic for failed requests
- Implement request caching
- Add request/response logging
- Handle rate limiting
This would improve maintainability and reusability across the application.
Would you like me to provide an example implementation of the service layer pattern?
app/routes/wow.out-of-stock.tsx (2)
145-151: Refactor form submission handling to prevent multiple submissionsCurrently,
handleSubmitprevents the default action if the form is already submitting. A more standard approach is to disable the submit button while the form is submitting, preventing multiple submissions without additional event handling.Apply this diff to disable the submit button when
isSubmittingistrue:{/* Remove the onClick handler from SmallFormContainer */} <SmallFormContainer title="Out of Stock Items" description="Find items that are out of stock on your realm!" - onClick={handleSubmit} error={error} loading={isSubmitting} action={getActionUrl(PAGE_URL, searchParams)} > {/* Ensure the submit button is disabled during submission */} <SubmitButton type="submit" disabled={isSubmitting} // other props />And remove the unnecessary
handleSubmitfunction:-const handleSubmit = ( - event: React.MouseEvent<HTMLButtonElement, MouseEvent> -) => { - if (isSubmitting) { - event.preventDefault() - } -}
218-222: Simplify theuseEffecthook by removing unnecessary checksThe check for
windowanddocumentinsideuseEffectis unnecessary becauseuseEffectonly runs on the client side, where these objects are always available.You can simplify the code as follows:
useEffect(() => { - if (window && document) { window.scroll({ top: 0, behavior: 'smooth' }) - } }, [])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
app/components/navigation/sidebar/Sidebar.tsx(1 hunks)app/requests/WoW/OutOfStock.ts(1 hunks)app/routes/[robots.txt].tsx(1 hunks)app/routes/wow._index.tsx(1 hunks)app/routes/wow.out-of-stock.tsx(1 hunks)app/tailwind.css(0 hunks)
💤 Files with no reviewable changes (1)
- app/tailwind.css
🧰 Additional context used
🪛 Biome (1.9.4)
app/routes/wow.out-of-stock.tsx
[error] 62-62: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (6)
app/routes/[robots.txt].tsx (1)
49-49: LGTM! Appropriate crawler restriction for dynamic data endpoint.
The addition of the disallow rule for the new out-of-stock page is correct and follows best practices for preventing crawling of dynamic data endpoints.
app/routes/wow._index.tsx (3)
141-146: LGTM! The new entry follows the established pattern.
The new "Out of Stock" entry maintains consistency with other entries in terms of structure and styling.
141-146: Integration with existing component structure is clean.
The new entry seamlessly integrates with the existing TileLink component and rendering logic.
146-146: Verify the existence of the target route.
The href points to '/wow/out-of-stock'. Let's verify that the corresponding route file exists and is properly implemented.
✅ Verification successful
Route implementation verified successfully
The target route /wow/out-of-stock is properly implemented in app/routes/wow.out-of-stock.tsx with:
- Complete React component implementation
- API integration through
WoWOutOfStockrequest handler - Proper form handling and data validation
- Results display with table component
- Meta tags and canonical URL setup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence of the out-of-stock route file and its implementation
# Look for the route file
echo "Checking for route file..."
fd -e tsx -e ts "out-of-stock" app/routes
# Look for API integration
echo "Checking for API integration..."
rg -l "api.saddlebagexchange.com/api/wow/outofstock" app/
Length of output: 267
Script:
#!/bin/bash
# Let's check the implementation of the out-of-stock route file
echo "Checking route implementation..."
cat "app/routes/wow.out-of-stock.tsx"
# Also check for any API-related code with a broader search
echo -e "\nChecking for API integration with broader pattern..."
rg "outofstock|out.*stock" app/routes/wow.out-of-stock.tsx
Length of output: 9149
app/components/navigation/sidebar/Sidebar.tsx (1)
279-283: LGTM! Navigation link properly implemented.
The new "Out of Stock" navigation link is well-structured and aligns with the existing navigation patterns. It correctly implements the required route for the out-of-stock items page.
app/routes/wow.out-of-stock.tsx (1)
216-216:
Ensure 'region' is available in the 'Results' component
The region variable is being destructured from useLoaderData, but the loader function does not return a region property. This could lead to region being undefined within the Results component.
Consider modifying the loader function to include region in its returned data. Here's how you can adjust the loader:
+import { getUserSessionData } from '~/sessions'
export const loader: LoaderFunction = async ({ request }) => {
+ const session = await getUserSessionData(request)
+ const region = session.getWoWSessionData().region
const params = new URL(request.url).searchParams
// ... existing code ...
if (!validParams.success) {
return json({
exception: parseZodErrorsToDisplayString(validParams.error, inputMap),
+ region,
})
}
- return json(validParams.data)
+ return json({ ...validParams.data, region })
}This ensures region is included in the loader's response and available in the Results component.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
app/routes/wow.out-of-stock.tsx (4)
51-59: Consider adding validation constraintsThe validation schema could benefit from additional constraints to ensure data quality.
Consider adding these constraints:
const validateInput = z.object({ - salesPerDay: z.string().transform((value) => parseFloat(value)), + salesPerDay: z.string() + .refine((val) => !isNaN(parseFloat(val)), "Must be a valid number") + .transform((value) => parseFloat(value)) + .refine((val) => val >= 0, "Must be non-negative"), avgPrice: parseStringToNumber, minMarketValue: parseStringToNumber, populationWP: parseStringToNumber, populationBlizz: parseStringToNumber, rankingWP: parseStringToNumber, - expansionNumber: parseStringToNumber + expansionNumber: parseStringToNumber.refine((val) => val >= -1, "Invalid expansion") })
145-151: Simplify handleSubmit functionThe current implementation can be simplified.
-const handleSubmit = ( - event: React.MouseEvent<HTMLButtonElement, MouseEvent> -) => { - if (isSubmitting) { - event.preventDefault() - } -} +const handleSubmit = (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => + isSubmitting && event.preventDefault()
218-222: Improve scroll behavior implementationThe current implementation has unnecessary checks and might not work in all browsers.
Consider this more robust implementation:
useEffect(() => { - if (window && document) { - window.scroll({ top: 0, behavior: 'smooth' }) - } + try { + window.scrollTo({ top: 0, behavior: 'smooth' }) + } catch (e) { + // Fallback for browsers that don't support smooth scrolling + window.scrollTo(0, 0) + } }, [])
261-263: Improve realm names overflow handlingThe current overflow implementation might lead to a poor user experience.
Consider using text truncation with a tooltip:
-<p className="px-3 py-2 max-w-[200px] overflow-x-scroll"> +<p className="px-3 py-2 max-w-[200px] truncate" title={row.realmNames}> {row.realmNames} </p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/routes/wow.out-of-stock.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/routes/wow.out-of-stock.tsx
[error] 62-62: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (2)
app/routes/wow.out-of-stock.tsx (2)
61-64:
Fix the empty object type definition
Using {} as a type is discouraged as it represents any non-nullable value. Let's use a more specific type.
Apply this diff to fix the issue:
type ActionResponseType =
- | {}
+ | Record<string, never>
| { exception: string }
| ({ data: OutOfStockItem[] } & { sortby: string })🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
124-126: Verify the canonical URL
Please ensure the canonical URL matches the production environment.
✅ Verification successful
The canonical URL is consistent with the codebase's URL structure
The canonical URL https://saddlebagexchange.com/wow/out-of-stock follows the same pattern as other routes in the codebase and matches the established URL structure. This is evidenced by:
- The domain
saddlebagexchange.comis consistently used across all canonical URLs in the codebase - The path structure
/wow/out-of-stockfollows the same pattern as other WoW-related routes - The URL is properly referenced in both the sitemap and route definitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the canonical URL is consistent across the codebase
# Search for other canonical URLs in the codebase
rg -i "canonical.*saddlebagexchange.com" --type tsx --type jsx
# Search for the domain name usage
rg "saddlebagexchange.com" --type tsx --type jsx
Length of output: 169
Script:
#!/bin/bash
# Description: Check if the canonical URL is consistent across the codebase
# Note: Using ripgrep without file type filters since tsx/jsx types weren't recognized
# Search for other canonical URLs in the codebase
rg -i "canonical.*saddlebagexchange.com"
# Search for the domain name usage
rg "saddlebagexchange.com"
# List all files with potential route definitions that might contain canonical URLs
fd -e tsx -e jsx -e ts -e js
Length of output: 35729
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
app/routes/wow.out-of-stock.tsx (4)
51-59: Consider adding constraints to the validation schemaThe validation schema could benefit from additional constraints to ensure data quality:
salesPerDayshould have a minimum value (e.g., 0)avgPriceandminMarketValueshould have reasonable minimum valuespopulationWPandpopulationBlizzshould be positive integersrankingWPshould be between 0 and 100const validateInput = z.object({ - salesPerDay: z.string().transform((value) => parseFloat(value)), + salesPerDay: z.string().transform((value) => parseFloat(value)) + .refine((val) => val >= 0, "Sales per day must be non-negative"), - avgPrice: parseStringToNumber, + avgPrice: parseStringToNumber + .refine((val) => val >= 0, "Average price must be non-negative"), - minMarketValue: parseStringToNumber, + minMarketValue: parseStringToNumber + .refine((val) => val >= 0, "Market value must be non-negative"), - populationWP: parseStringToNumber, + populationWP: parseStringToNumber + .refine((val) => Number.isInteger(val) && val >= 0, "Population must be a positive integer"), - populationBlizz: parseStringToNumber, + populationBlizz: parseStringToNumber + .refine((val) => Number.isInteger(val) && val >= 0, "Population must be a positive integer"), - rankingWP: parseStringToNumber, + rankingWP: parseStringToNumber + .refine((val) => val >= 0 && val <= 100, "Ranking must be between 0 and 100"), expansionNumber: parseStringToNumber })
210-224: Enhance form field accessibilityConsider adding ARIA attributes to improve form field accessibility:
<InputWithLabel key={key} labelTitle={inputMap[key]} name={key} type="number" defaultValue={loaderData[key as keyof typeof defaultFormValues]} step={key === 'salesPerDay' ? '0.1' : '1'} min={0} + aria-required="true" + aria-describedby={`${key}-description`} onChange={(e) => handleFormChange( key as keyof typeof defaultFormValues, e.target.value ) } />
241-245: Consider adding a loading state for smooth scrollThe auto-scroll behavior might be jarring if the content is still loading. Consider adding a loading check:
useEffect(() => { - if (window && document) { + const timer = setTimeout(() => { + if (window && document) { window.scroll({ top: 0, behavior: 'smooth' }) + } + }, 100) // Small delay to ensure content is rendered + return () => clearTimeout(timer) - } }, [])
284-286: Improve realm names overflow handlingThe current overflow handling might not provide the best user experience. Consider using a tooltip or ellipsis:
-<p className="px-3 py-2 max-w-[200px] overflow-x-scroll"> +<p className="px-3 py-2 max-w-[200px] truncate" title={row.realmNames}> {row.realmNames} </p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/routes/wow.out-of-stock.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/routes/wow.out-of-stock.tsx
[error] 62-62: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (2)
app/routes/wow.out-of-stock.tsx (2)
66-89: LGTM! Well-structured loader with proper error handling
The loader function effectively:
- Parses and validates URL parameters
- Provides meaningful error messages
- Handles edge cases appropriately
135-147: LGTM! Good SEO practices
The meta tags and canonical link are well-defined for SEO optimization.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/routes/wow.out-of-stock.tsx (4)
51-59: Enhance input validation constraintsThe current validation schema only handles type conversion but lacks business logic constraints. Consider adding:
- Minimum/maximum bounds for numeric fields
- Custom validation for expansion numbers
const validateInput = z.object({ - salesPerDay: z.string().transform((value) => parseFloat(value)), + salesPerDay: z.string() + .transform((value) => parseFloat(value)) + .refine((val) => val >= 0 && val <= 100, "Sales per day must be between 0 and 100"), - avgPrice: parseStringToNumber, + avgPrice: parseStringToNumber + .refine((val) => val >= 0, "Average price must be positive"), - minMarketValue: parseStringToNumber, + minMarketValue: parseStringToNumber + .refine((val) => val >= 0, "Market value must be positive"), // Add similar constraints for other fields expansionNumber: parseStringToNumber + .refine((val) => val >= -1 && val <= 10, "Invalid expansion number") })
139-147: Enhance SEO meta tagsConsider adding more meta tags to improve SEO:
export const meta: MetaFunction = () => { return { charset: 'utf-8', viewport: 'width=device-width,initial-scale=1', title: 'Saddlebag Exchange: WoW Out of Stock Items', description: 'Find items that are out of stock on your realm with our WoW Out of Stock search!', + 'og:title': 'Saddlebag Exchange: WoW Out of Stock Items', + 'og:description': 'Find items that are out of stock on your realm with our WoW Out of Stock search!', + 'og:type': 'website', + 'twitter:card': 'summary', + keywords: 'World of Warcraft, WoW, Out of Stock, Items, Auction House, Trading' } }
158-159: Optimize component performanceConsider these performance improvements:
- Memoize the search params state to prevent unnecessary re-renders
- Use a debounce function for form changes
+import { useMemo } from 'react' +import debounce from 'lodash/debounce' const isSubmitting = transition.state === 'submitting' -const [searchParams, setSearchParams] = useState(loaderData) +const [searchParams, setSearchParams] = useState(() => loaderData) + +const debouncedHandleFormChange = useMemo( + () => + debounce((name: keyof typeof defaultFormValues, value: string) => { + handleSearchParamChange(name, value) + setSearchParams((prev) => ({ ...prev, [name]: value })) + }, 300), + [] +)
245-249: Improve scroll behaviorThe current scroll implementation has potential issues:
- No fallback for browsers that don't support smooth scrolling
- No check for
window.scrollTosupportuseEffect(() => { - if (window && document) { + if (typeof window !== 'undefined') { + try { + if ('scrollBehavior' in document.documentElement.style) { window.scroll({ top: 0, behavior: 'smooth' }) + } else { + window.scrollTo(0, 0) + } + } catch (e) { + console.warn('Browser does not support smooth scrolling:', e) + window.scrollTo(0, 0) } + } }, [])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/routes/wow.out-of-stock.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/routes/wow.out-of-stock.tsx
[error] 62-62: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
app/routes/wow.out-of-stock.tsx (6)
66-89: Improve type safety in the loader functionThe loader function's return type could be more explicit to ensure type safety throughout the component.
Consider adding a specific return type:
-export const loader: LoaderFunction = async ({ request }) => { +export const loader: LoaderFunction<typeof defaultFormValues | { exception: string }> = async ({ request }) => {
112-117: Add timeout to API callLong-running API calls could impact user experience. Consider adding a timeout to prevent indefinite waiting.
Add timeout handling:
const result = await WoWOutOfStock({ ...validatedFormData.data, region, includeCategories: [], - excludeCategories: [] + excludeCategories: [], + signal: AbortSignal.timeout(10000) // 10 second timeout })
140-148: Consider dynamic meta tags based on search parametersThe meta tags could be more specific based on the search parameters to improve SEO.
Consider making meta tags dynamic:
-export const meta: MetaFunction = () => { +export const meta: MetaFunction<typeof loader> = ({ data }) => { + const expansion = data?.expansionNumber === '-1' ? 'All Expansions' : `Expansion ${data?.expansionNumber}` return { charset: 'utf-8', viewport: 'width=device-width,initial-scale=1', - title: 'Saddlebag Exchange: WoW Out of Stock Items', + title: `Saddlebag Exchange: WoW Out of Stock Items (${expansion})`, description: - 'Find items that are out of stock on your realm with our WoW Out of Stock search!' + `Find items that are out of stock on your realm for ${expansion} with our WoW Out of Stock search!` } }
179-185: Enhance form validation feedbackConsider providing immediate feedback when users input invalid values rather than waiting for form submission.
Add client-side validation:
const handleFormChange = ( name: keyof typeof defaultFormValues, value: string ) => { + const validation = validateInput.safeParse({ ...searchParams, [name]: value }) + if (!validation.success) { + // Update UI to show field-specific error + const error = parseZodErrorsToDisplayString(validation.error, inputMap) + // You'll need to add state for field-level errors + setFieldErrors(prev => ({ ...prev, [name]: error })) + } handleSearchParamChange(name, value) setSearchParams((prev) => ({ ...prev, [name]: value })) }
246-250: Improve scroll behavior for accessibilityThe current scroll implementation might interfere with screen readers and keyboard navigation.
Consider using a more accessible approach:
useEffect(() => { - if (window && document) { - window.scroll({ top: 0, behavior: 'smooth' }) + if (typeof window !== 'undefined') { + const main = document.querySelector('main') + if (main) { + main.focus() + main.scrollIntoView({ behavior: 'smooth', block: 'start' }) + } } }, [])
295-303: Enhance external link security and accessibilityExternal links should have additional security and accessibility attributes.
Add security and accessibility attributes to external links:
{ columnId: 'saddlebagLink', header: 'Where to Buy', - accessor: ({ getValue }) => <ExternalLink link={getValue() as string} /> + accessor: ({ getValue }) => ( + <ExternalLink + link={getValue() as string} + rel="noopener noreferrer" + aria-label="View item details on Saddlebag Exchange" + /> + ) }, { columnId: 'undermineLink', header: 'Undermine Link', - accessor: ({ getValue }) => <ExternalLink link={getValue() as string} /> + accessor: ({ getValue }) => ( + <ExternalLink + link={getValue() as string} + rel="noopener noreferrer" + aria-label="View item details on The Undermine Journal" + /> + ) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/routes/wow.out-of-stock.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/routes/wow.out-of-stock.tsx
[error] 62-62: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
app/components/form/WoW/OutOfStockForm/index.tsx (2)
26-104: Consider adding form validation feedbackThe form lacks visual feedback for invalid inputs and doesn't prevent submission of invalid data.
Consider adding:
- Error messages for invalid inputs
- Visual indicators for validation state
- Form-level validation before submission
Would you like me to provide an example implementation using a form validation library like Zod or Yup?
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 29-29: app/components/form/WoW/OutOfStockForm/index.tsx#L29
Returning a void expression from an arrow function shorthand is forbidden. Please add braces to the arrow function.
[failure] 51-51: app/components/form/WoW/OutOfStockForm/index.tsx#L51
Unsafe member access .target on ananyvalue.
[failure] 61-61: app/components/form/WoW/OutOfStockForm/index.tsx#L61
Unsafe argument of typeanyassigned to a parameter of typestring.
[failure] 61-61: app/components/form/WoW/OutOfStockForm/index.tsx#L61
Unsafe member access .target on ananyvalue.
[failure] 71-71: app/components/form/WoW/OutOfStockForm/index.tsx#L71
Unsafe argument of typeanyassigned to a parameter of typestring.
[failure] 81-81: app/components/form/WoW/OutOfStockForm/index.tsx#L81
Unsafe member access .target on ananyvalue.
[failure] 91-91: app/components/form/WoW/OutOfStockForm/index.tsx#L91
Returning a void expression from an arrow function shorthand is forbidden. Please add braces to the arrow function.
[failure] 91-91: app/components/form/WoW/OutOfStockForm/index.tsx#L91
Unsafe argument of typeanyassigned to a parameter of typestring.
1-108: Consider extracting common input handling logicThe form contains multiple numeric inputs with similar validation and change handling patterns. This could be simplified using a custom hook or higher-order component.
Consider creating a custom hook like
useNumericInputto encapsulate the common logic:function useNumericInput( name: string, options: { min?: number; max?: number; step?: number } ) { return { type: "number", min: options.min, max: options.max, step: options.step, onChange: (e: React.ChangeEvent<HTMLInputElement>) => { const value = e.target.value; if (value === '' || ( Number(value) >= (options.min ?? -Infinity) && Number(value) <= (options.max ?? Infinity) )) { onFormChange(name, value); } } }; }This would reduce duplication and centralize input handling logic.
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 29-29: app/components/form/WoW/OutOfStockForm/index.tsx#L29
Returning a void expression from an arrow function shorthand is forbidden. Please add braces to the arrow function.
[failure] 51-51: app/components/form/WoW/OutOfStockForm/index.tsx#L51
Unsafe member access .target on ananyvalue.
[failure] 61-61: app/components/form/WoW/OutOfStockForm/index.tsx#L61
Unsafe argument of typeanyassigned to a parameter of typestring.
[failure] 61-61: app/components/form/WoW/OutOfStockForm/index.tsx#L61
Unsafe member access .target on ananyvalue.
[failure] 71-71: app/components/form/WoW/OutOfStockForm/index.tsx#L71
Unsafe argument of typeanyassigned to a parameter of typestring.
[failure] 81-81: app/components/form/WoW/OutOfStockForm/index.tsx#L81
Unsafe member access .target on ananyvalue.
[failure] 91-91: app/components/form/WoW/OutOfStockForm/index.tsx#L91
Returning a void expression from an arrow function shorthand is forbidden. Please add braces to the arrow function.
[failure] 91-91: app/components/form/WoW/OutOfStockForm/index.tsx#L91
Unsafe argument of typeanyassigned to a parameter of typestring.app/routes/wow.out-of-stock.tsx (3)
21-21: Remove unused importThe
MetaFunctiontype is imported but never used in the code.-import type { MetaFunction, LinksFunction } from '@remix-run/node' +import type { LinksFunction } from '@remix-run/node'🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 21-21: app/routes/wow.out-of-stock.tsx#L21
'MetaFunction' is defined but never used.
151-211: Consider performance optimizations for the formThe form implementation could benefit from performance optimizations:
- Memoize the submit handler
- Use a debounced version of handleFormChange
+import { useCallback, useMemo } from 'react' +import debounce from 'lodash/debounce' const OutOfStock = () => { // ... - const handleSubmit = (event: React.MouseEvent<HTMLButtonElement>) => { + const handleSubmit = useCallback((event: React.MouseEvent<HTMLButtonElement>) => { if (isSubmitting) { event.preventDefault() } - } + }, [isSubmitting]) - const handleFormChange = ( + const handleFormChange = useMemo(() => debounce(( name: keyof typeof defaultFormValues, value: string ) => { handleSearchParamChange(name, value) setSearchParams((prev) => ({ ...prev, [name]: value })) - } + }, 300), [])
222-226: Improve scroll behavior implementationThe scroll behavior implementation could be more robust:
useEffect(() => { - if (window && document) { + if (typeof window !== 'undefined') { window.scroll({ top: 0, behavior: 'smooth' }) } }, [])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/components/form/WoW/OutOfStockForm/index.tsx(1 hunks)app/consts.ts(1 hunks)app/routes/wow.out-of-stock.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/components/form/WoW/OutOfStockForm/index.tsx
[error] 20-20: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🪛 GitHub Check: Codacy Static Code Analysis
app/components/form/WoW/OutOfStockForm/index.tsx
[failure] 29-29: app/components/form/WoW/OutOfStockForm/index.tsx#L29
Returning a void expression from an arrow function shorthand is forbidden. Please add braces to the arrow function.
[failure] 51-51: app/components/form/WoW/OutOfStockForm/index.tsx#L51
Unsafe member access .target on an any value.
[failure] 61-61: app/components/form/WoW/OutOfStockForm/index.tsx#L61
Unsafe argument of type any assigned to a parameter of type string.
[failure] 61-61: app/components/form/WoW/OutOfStockForm/index.tsx#L61
Unsafe member access .target on an any value.
[failure] 71-71: app/components/form/WoW/OutOfStockForm/index.tsx#L71
Unsafe argument of type any assigned to a parameter of type string.
[failure] 81-81: app/components/form/WoW/OutOfStockForm/index.tsx#L81
Unsafe member access .target on an any value.
[failure] 91-91: app/components/form/WoW/OutOfStockForm/index.tsx#L91
Returning a void expression from an arrow function shorthand is forbidden. Please add braces to the arrow function.
[failure] 91-91: app/components/form/WoW/OutOfStockForm/index.tsx#L91
Unsafe argument of type any assigned to a parameter of type string.
app/routes/wow.out-of-stock.tsx
[failure] 21-21: app/routes/wow.out-of-stock.tsx#L21
'MetaFunction' is defined but never used.
[failure] 51-51: app/routes/wow.out-of-stock.tsx#L51
Unsafe assignment of an error typed value.
[failure] 52-52: app/routes/wow.out-of-stock.tsx#L52
Unsafe assignment of an error typed value.
[failure] 52-52: app/routes/wow.out-of-stock.tsx#L52
Unsafe call of an error type typed value.
[failure] 53-53: app/routes/wow.out-of-stock.tsx#L53
Unsafe assignment of an error typed value.
[failure] 53-53: app/routes/wow.out-of-stock.tsx#L53
Unsafe member access .string on an error typed value.
[failure] 54-54: app/routes/wow.out-of-stock.tsx#L54
Unsafe argument of type any assigned to a parameter of type string.
[failure] 54-54: app/routes/wow.out-of-stock.tsx#L54
Unsafe assignment of an error typed value.
[failure] 56-56: app/routes/wow.out-of-stock.tsx#L56
Unsafe argument of type any assigned to a parameter of type string.
[failure] 56-56: app/routes/wow.out-of-stock.tsx#L56
Unsafe assignment of an error typed value.
[failure] 56-56: app/routes/wow.out-of-stock.tsx#L56
Unsafe member access .string on an error typed value.
[failure] 77-77: app/routes/wow.out-of-stock.tsx#L77
Unsafe argument of type any assigned to a parameter of type string | URL.
[failure] 92-92: app/routes/wow.out-of-stock.tsx#L92
Unsafe assignment of an error typed value.
[failure] 112-112: app/routes/wow.out-of-stock.tsx#L112
Unsafe member access .formData on an any value.
[failure] 113-113: app/routes/wow.out-of-stock.tsx#L113
Unsafe member access .safeParse on an error typed value.
[failure] 118-118: app/routes/wow.out-of-stock.tsx#L118
Unsafe member access .error on an error typed value.
🔇 Additional comments (3)
app/consts.ts (1)
18-32: Verify gaps in category values
The category values have gaps (e.g., missing 5, 6, 10, 11, etc.). While this might be intentional if matching WoW's internal category IDs, please confirm this is expected behavior.
✅ Verification successful
Let me search for more context about these categories.
Let me search for any documentation or comments about these categories.
Based on the codebase analysis, I can now provide a definitive response about the category values:
Gaps in category values are intentional and correct
The gaps in the category values (missing 5, 6, 10, 11, etc.) are intentional as they match World of Warcraft's internal item class IDs. This is evidenced by:
- The consistent usage of these exact category IDs throughout the codebase in multiple components and API requests
- The values being used as direct mappings in API requests (e.g.,
item_class: itemClass) - The values being used in predefined URLs for specific item categories (e.g.,
weapons: ${BASE_URL}?itemClass=2) - The systematic use in marketshare queries and best deals filters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any documentation or constants that might validate these category IDs
rg -i "item.*category.*id|item.*class.*id" --type ts
Length of output: 324
Script:
#!/bin/bash
# Search for any references to these categories or related documentation
rg -i "wowcategories|item.*class" --type ts -A 3
Length of output: 36464
Script:
#!/bin/bash
# Search for documentation or comments about WoW item categories
rg -i "wow.*item.*categor|item.*class" --type md --type txt
Length of output: 61
app/routes/wow.out-of-stock.tsx (2)
103-149: LGTM! Well-implemented action function
The action function includes:
- Proper region validation
- Comprehensive error handling
- API response validation
- Type-safe error messages
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 112-112: app/routes/wow.out-of-stock.tsx#L112
Unsafe member access .formData on an any value.
[failure] 113-113: app/routes/wow.out-of-stock.tsx#L113
Unsafe member access .safeParse on an error typed value.
[failure] 118-118: app/routes/wow.out-of-stock.tsx#L118
Unsafe member access .error on an error typed value.
92-95: 🛠️ Refactor suggestion
Add region validation in loader
The loader should validate that the region exists before returning it.
const session = await getUserSessionData(request)
const { region } = session.getWoWSessionData()
+ if (!region) {
+ return json({
+ exception: 'Region is required. Please select a region in your settings.'
+ })
+ }
return json({ ...validParams.data, region })Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 92-92: app/routes/wow.out-of-stock.tsx#L92
Unsafe assignment of an error typed value.
| <InputWithLabel | ||
| defaultValue={defaultValues.salesPerDay} | ||
| type="number" | ||
| labelTitle="Minimum Sales Per Day" | ||
| inputTag="Sales" | ||
| name="salesPerDay" | ||
| min={0} | ||
| step={0.01} | ||
| onChange={(e) => onFormChange('salesPerDay', e.target.value)} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper TypeScript event typing and validation
The numeric input handlers have type safety issues and could benefit from additional validation.
- onChange={(e) => onFormChange('salesPerDay', e.target.value)}
+ onChange={(e: React.ChangeEvent<HTMLInputElement>) => {
+ const value = e.target.value;
+ if (value === '' || Number(value) >= 0) {
+ onFormChange('salesPerDay', value);
+ }
+ }}Apply similar changes to all numeric input handlers.
Also applies to: 53-62, 63-72, 73-82, 83-92, 93-103
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 51-51: app/components/form/WoW/OutOfStockForm/index.tsx#L51
Unsafe member access .target on an any value.
| const parseCategories = (value: string | number | number[]): number[] => { | ||
| if (Array.isArray(value)) return value | ||
| if (typeof value === 'string' && value.trim() !== '') { | ||
| return value | ||
| .split(',') | ||
| .map(Number) | ||
| .filter((n) => !isNaN(n)) | ||
| } | ||
| return [] | ||
| } |
There was a problem hiding this comment.
Replace isNaN with Number.isNaN for safer type checking
The global isNaN function performs type coercion which can lead to unexpected results. Use Number.isNaN instead for more precise number validation.
return value
.split(',')
.map(Number)
- .filter((n) => !isNaN(n))
+ .filter((n) => !Number.isNaN(n))📝 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 parseCategories = (value: string | number | number[]): number[] => { | |
| if (Array.isArray(value)) return value | |
| if (typeof value === 'string' && value.trim() !== '') { | |
| return value | |
| .split(',') | |
| .map(Number) | |
| .filter((n) => !isNaN(n)) | |
| } | |
| return [] | |
| } | |
| const parseCategories = (value: string | number | number[]): number[] => { | |
| if (Array.isArray(value)) return value | |
| if (typeof value === 'string' && value.trim() !== '') { | |
| return value | |
| .split(',') | |
| .map(Number) | |
| .filter((n) => !Number.isNaN(n)) | |
| } | |
| return [] | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| defaultValues: Record<string, string | number | number[]> | ||
| onFormChange: (name: string, value: string) => void |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type the onFormChange callback parameters more strictly
The callback accepts any string for the name parameter, which could lead to runtime errors. Consider using a union type of valid field names.
- onFormChange: (name: string, value: string) => void
+ type FormField = 'expansionNumber' | 'includeCategories' | 'excludeCategories' |
+ 'salesPerDay' | 'avgPrice' | 'minMarketValue' | 'populationWP' |
+ 'populationBlizz' | 'rankingWP';
+ onFormChange: (name: FormField, value: string) => void📝 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.
| defaultValues: Record<string, string | number | number[]> | |
| onFormChange: (name: string, value: string) => void | |
| defaultValues: Record<string, string | number | number[]> | |
| type FormField = 'expansionNumber' | 'includeCategories' | 'excludeCategories' | | |
| 'salesPerDay' | 'avgPrice' | 'minMarketValue' | 'populationWP' | | |
| 'populationBlizz' | 'rankingWP'; | |
| onFormChange: (name: FormField, value: string) => void |
| const validateInput = z.object({ | ||
| salesPerDay: z.string().transform((value) => parseFloat(value)), | ||
| avgPrice: z.string().transform((value) => parseInt(value)), | ||
| minMarketValue: z.string().transform((value) => parseInt(value)), | ||
| populationWP: z.string().transform((value) => parseInt(value)), | ||
| populationBlizz: z.string().transform((value) => parseInt(value)), | ||
| rankingWP: z.string().transform((value) => parseInt(value)), | ||
| expansionNumber: z.string().transform((value) => parseInt(value)), | ||
| includeCategories: z | ||
| .string() | ||
| .transform((value) => | ||
| value.trim() === '' | ||
| ? [] | ||
| : value.split(',').map((value) => parseInt(value)) | ||
| ), | ||
| excludeCategories: z | ||
| .string() | ||
| .transform((value) => | ||
| value.trim() === '' | ||
| ? [] | ||
| : value.split(',').map((value) => parseInt(value)) | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in validation schema
The validation schema has potential type safety issues with string-to-number transformations.
const validateInput = z.object({
- salesPerDay: z.string().transform((value) => parseFloat(value)),
+ salesPerDay: z.string().pipe(z.coerce.number()),
- avgPrice: z.string().transform((value) => parseInt(value)),
+ avgPrice: z.string().pipe(z.coerce.number().int()),
- minMarketValue: z.string().transform((value) => parseInt(value)),
+ minMarketValue: z.string().pipe(z.coerce.number().int()),
- populationWP: z.string().transform((value) => parseInt(value)),
+ populationWP: z.string().pipe(z.coerce.number().int()),
- populationBlizz: z.string().transform((value) => parseInt(value)),
+ populationBlizz: z.string().pipe(z.coerce.number().int()),
- rankingWP: z.string().transform((value) => parseInt(value)),
+ rankingWP: z.string().pipe(z.coerce.number().int()),
- expansionNumber: z.string().transform((value) => parseInt(value)),
+ expansionNumber: z.string().pipe(z.coerce.number().int()),Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 51-51: app/routes/wow.out-of-stock.tsx#L51
Unsafe assignment of an error typed value.
[failure] 52-52: app/routes/wow.out-of-stock.tsx#L52
Unsafe assignment of an error typed value.
[failure] 52-52: app/routes/wow.out-of-stock.tsx#L52
Unsafe call of an error type typed value.
[failure] 53-53: app/routes/wow.out-of-stock.tsx#L53
Unsafe assignment of an error typed value.
[failure] 53-53: app/routes/wow.out-of-stock.tsx#L53
Unsafe member access .string on an error typed value.
[failure] 54-54: app/routes/wow.out-of-stock.tsx#L54
Unsafe argument of type any assigned to a parameter of type string.
[failure] 54-54: app/routes/wow.out-of-stock.tsx#L54
Unsafe assignment of an error typed value.
[failure] 56-56: app/routes/wow.out-of-stock.tsx#L56
Unsafe argument of type any assigned to a parameter of type string.
[failure] 56-56: app/routes/wow.out-of-stock.tsx#L56
Unsafe assignment of an error typed value.
[failure] 56-56: app/routes/wow.out-of-stock.tsx#L56
Unsafe member access .string on an error typed value.
closes #479
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Disallowdirective for web crawlers in the robots.txt file.Style