Skip to content

Conversation

mjkeaton
Copy link
Contributor

@mjkeaton mjkeaton commented Apr 2, 2025

User description

Partly addresses #10.

Updates to new quote service API.

Display quotes in state denied, expired, rejected and offered.


PR Type

Enhancement, Tests


Description

  • Integrated new quote service API for better functionality.

    • Added support for listing quotes by various states (offered, denied, expired, rejected).
    • Updated endpoints and query options to align with the new API.
  • Enhanced UI for quotes management.

    • Added pages for viewing quotes by state (offered, denied, expired, rejected).
    • Updated navigation and sidebar to include new quote states.
  • Refactored and improved backend mocks and handlers.

    • Updated mock database structure to support new quote states.
    • Adjusted API mocks to align with the updated endpoints.
  • Updated dependencies and improved code formatting.

    • Upgraded react-day-picker to the latest version.
    • Applied consistent formatting and resolved ESLint issues.

Changes walkthrough 📝

Relevant files
Enhancement
15 files
endpoints.ts
Updated API endpoints for quote management                             
+2/-10   
react-query.gen.ts
Added query options for new quote states                                 
+32/-32 
sdk.gen.ts
Added SDK methods for new quote states                                     
+20/-20 
types.gen.ts
Updated types for new quote states                                             
+76/-67 
AppSidebar.tsx
Updated sidebar to include new quote states                           
+0/-4     
calendar.tsx
Adjusted calendar component for better usability                 
+32/-28 
main.tsx
Added routes for new quote state pages                                     
+8/-0     
AcceptedQuotesPage.tsx
Updated accepted quotes page to use new API                           
+15/-6   
DeniedQuotesPage.tsx
Added page for denied quotes                                                         
+136/-0 
ExpiredQuotesPage.tsx
Added page for expired quotes                                                       
+136/-0 
OfferedQuotesPage.tsx
Added page for offered quotes                                                       
+136/-0 
PendingQuotesPage.tsx
Updated pending quotes page to align with new API               
+0/-1     
QuotePage.tsx
Updated quote details page for new API                                     
+6/-13   
QuotesPage.tsx
Enhanced quotes overview page with new states                       
+103/-2 
RejectedQuotesPage.tsx
Added page for rejected quotes                                                     
+136/-0 
Tests
3 files
db.ts
Updated mock database to support new quote states               
+6/-5     
handlers.ts
Added handlers for new quote states                                           
+2/-2     
admin_quotes.ts
Updated admin quote handlers for new states                           
+19/-10 
Documentation
2 files
__dev_openapi.json
Updated OpenAPI spec for new quote states                               
+186/-110
openapi.json
Updated OpenAPI spec for new quote states                               
+1/-1     
Dependencies
1 files
package.json
Upgraded `react-day-picker` dependency                                     
+1/-1     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @mjkeaton mjkeaton self-assigned this Apr 2, 2025
    Copy link

    qodo-merge-pro bot commented Apr 2, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    CI Feedback 🧐

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: test (v22.11.0)

    Failed stage: Lint [❌]

    Failed test name: npm run lint

    Failure summary:

    The action failed because the ESLint check found 6 TypeScript errors across three files:

  • In AcceptedQuotesPage.tsx, DeniedQuotesPage.tsx, and OfferedQuotesPage.tsx, there are unnecessary
    type assertions at lines 59:60 and 68:62 in each file
  • All errors are related to the same ESLint rule: @typescript-eslint/no-unnecessary-type-assertion
  • These errors could be automatically fixed using the --fix option with ESLint

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    144:  ##[endgroup]
    145:  added 661 packages, and audited 662 packages in 14s
    146:  178 packages are looking for funding
    147:  run `npm fund` for details
    148:  1 moderate severity vulnerability
    149:  To address all issues, run:
    150:  npm audit fix
    151:  Run `npm audit` for details.
    152:  ##[group]Run npm run lint
    153:  �[36;1mnpm run lint�[0m
    154:  shell: /usr/bin/bash -e {0}
    155:  ##[endgroup]
    156:  > [email protected] lint
    157:  > eslint .
    158:  /home/runner/work/wildcat-dashboard-ui/wildcat-dashboard-ui/src/pages/quotes/AcceptedQuotesPage.tsx
    159:  ##[error]  59:60  error  This assertion is unnecessary since it does not change the type of the expression  @typescript-eslint/no-unnecessary-type-assertion
    160:  ##[error]  68:62  error  This assertion is unnecessary since it does not change the type of the expression  @typescript-eslint/no-unnecessary-type-assertion
    161:  /home/runner/work/wildcat-dashboard-ui/wildcat-dashboard-ui/src/pages/quotes/DeniedQuotesPage.tsx
    162:  ##[error]  59:60  error  This assertion is unnecessary since it does not change the type of the expression  @typescript-eslint/no-unnecessary-type-assertion
    163:  ##[error]  68:62  error  This assertion is unnecessary since it does not change the type of the expression  @typescript-eslint/no-unnecessary-type-assertion
    164:  /home/runner/work/wildcat-dashboard-ui/wildcat-dashboard-ui/src/pages/quotes/OfferedQuotesPage.tsx
    165:  ##[error]  59:60  error  This assertion is unnecessary since it does not change the type of the expression  @typescript-eslint/no-unnecessary-type-assertion
    166:  ##[error]  68:62  error  This assertion is unnecessary since it does not change the type of the expression  @typescript-eslint/no-unnecessary-type-assertion
    167:  ✖ 6 problems (6 errors, 0 warnings)
    168:  6 errors and 0 warnings potentially fixable with the `--fix` option.
    169:  ##[error]Process completed with exit code 1.
    170:  Post job cleanup.
    

    @mjkeaton mjkeaton changed the title feat: display offered, denied quotes feat: display quotes by state Apr 2, 2025
    @mjkeaton mjkeaton requested a review from mtbitcr April 2, 2025 17:04
    @mjkeaton mjkeaton marked this pull request as ready for review April 2, 2025 17:06
    Copy link

    qodo-merge-pro bot commented Apr 2, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    10 - Fully compliant

    Compliant requirements:

    • Display quotes list
    • Support viewing quotes in different states (denied, expired, rejected, offered)
    • Integrate with new quote service API
    • Update UI to navigate between different quote states

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Inconsistent Data Model

    The component references both value.bill?.payee and value.bill?.holder in the ParticipantsOverviewCard, but the data model has changed from using 'holder' to 'endorsees'. This could cause rendering issues or incorrect data display.

      drawee={value.bill?.drawee}
      drawer={value.bill?.drawer}
      payee={value.bill?.payee}
      holder={value.bill?.payee}
    />
    
    Missing Status Filter

    The fetchAdminQuote handler doesn't properly handle the 'status' query parameter. It uses getAll() but should filter by status from the URL parameters.

    let data = db.quotes.getAll()
    
    const states = url.searchParams.getAll("status")
    if (states.length !== 0) {
      data = data.filter((it) => states.includes(it.status ?? ""))
    }

    Copy link

    qodo-merge-pro bot commented Apr 2, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add case-insensitive status comparison

    The mock handler for fetching quotes doesn't account for the case sensitivity of
    status values. The API expects capitalized status values (like "Accepted"), but
    the client might send lowercase values (like "accepted"). The filter should
    perform a case-insensitive comparison to handle this mismatch.

    src/mocks/handlers/admin_quotes.ts [27-37]

     export const fetchAdminQuote = http.get<never, never, ListReplyLight>(
       `${API_URL}${ADMIN_QUOTE}`,
       async ({ request }) => {
         const url = new URL(request.url)
         await delay(1_000)
         let data = db.quotes.getAll()
     
         const states = url.searchParams.getAll("status")
         if (states.length !== 0) {
    -      data = data.filter((it) => states.includes(it.status ?? ""))
    +      data = data.filter((it) => 
    +        states.some(state => (it.status ?? "").toLowerCase() === state.toLowerCase())
    +      )
         }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: This suggestion addresses a critical issue in the mock handler that could mask the capitalization problem during testing. By implementing case-insensitive comparison, it ensures the mock server behaves consistently regardless of case, preventing hard-to-diagnose issues in production.

    High
    Fix incorrect avatar value

    The component is incorrectly using the payee value for the Holder avatar instead
    of the holder value that was passed as a prop. This creates inconsistency with
    the table row where holder is correctly set to value.bill?.payee.

    src/pages/quotes/QuotePage.tsx [387]

    -<IdentityPublicDataAvatar value={payee} tooltip="Holder" />
    +<IdentityPublicDataAvatar value={holder} tooltip="Holder" />
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: This suggestion correctly identifies a critical issue where the wrong prop value is being used for the Holder avatar. The component is using the payee value instead of the holder value, which would display incorrect information to users.

    High
    Fix status case mismatch

    The type casting to unknown and then to ListQuotesData["query"] is unsafe and
    unnecessary. The API expects string values for status, but the current
    implementation uses lowercase status values while the API expects capitalized
    status values (like "Accepted" instead of "accepted"). This mismatch could cause
    API errors.

    src/pages/quotes/QuotesPage.tsx [31-37]

     const { data: quotesAccepted } = useSuspenseQuery({
       ...listQuotesOptions({
         query: {
    -      status: "accepted",
    -    } as unknown as ListQuotesData["query"],
    +      status: "Accepted",
    +    },
       }),
     })

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies a critical issue where the client code uses lowercase status values ("accepted") while the API expects capitalized values ("Accepted") as defined in the StatusReply type. This mismatch could cause API errors and broken functionality.

    Medium
    Capitalize status values

    Similar to the previous issue, all status values should be capitalized to match
    the API's expected format. In the StatusReply type, status values are defined
    with capital letters (e.g., "Offered" not "offered"). This should be fixed for
    all status queries.

    src/pages/quotes/QuotesPage.tsx [39-45]

     const { data: quotesOffered } = useSuspenseQuery({
       ...listQuotesOptions({
         query: {
    -      status: "offered",
    -    } as unknown as ListQuotesData["query"],
    +      status: "Offered",
    +    },
       }),
     })

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 7

    __

    Why: This suggestion identifies the same capitalization issue in another query, reinforcing the pattern that needs to be fixed across all status queries. It's important for consistent API communication but slightly less critical than the first instance.

    Medium
    General
    Remove redundant animation class

    The animate-spin class is being applied twice when isFetching is true - once
    unconditionally and once conditionally. This creates redundancy and potential
    confusion in the code.

    src/pages/quotes/OfferedQuotesPage.tsx [41-46]

     <LoaderIcon
    -  className={cn("stroke-1 animate-spin", {
    +  className={cn("stroke-1", {
         "animate-spin": isFetching,
         invisible: !isFetching,
       })}
     />
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    __

    Why: This suggestion correctly identifies a redundancy in the animation class application. While not a critical bug, removing the duplicate class improves code clarity and prevents potential confusion or unexpected behavior.

    Low
    Remove leading spaces

    The positioning classes left-1 and right-1 have leading spaces which could cause
    inconsistent styling. Remove the leading spaces to ensure proper class
    application.

    src/components/ui/calendar.tsx [24-33]

     button_previous: cn(
       buttonVariants({ variant: "outline" }),
       "size-7 bg-transparent p-0 opacity-50 hover:opacity-100",
    -  " left-1"
    +  "left-1"
     ),
     button_next: cn(
       buttonVariants({ variant: "outline" }),
       "size-7 bg-transparent p-0 opacity-50 hover:opacity-100",
    -  " right-1"
    +  "right-1"
     ),
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    __

    Why: This suggestion correctly identifies a minor styling issue where leading spaces in class names could cause inconsistent styling. While not critical, this fix improves code quality and prevents potential subtle UI inconsistencies.

    Low
    • More

    @mtbitcr mtbitcr requested a review from cryptosalomao April 3, 2025 08:04
    @mjkeaton mjkeaton merged commit 1ba7137 into master Apr 3, 2025
    3 of 4 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants