Skip to content

Conversation

stefanbitcr
Copy link
Contributor

@stefanbitcr stefanbitcr commented Jun 3, 2025

User description

📝 Description

Depends on Wildcat Keysets being exposed, Wildcat discounted being exposed and Wildcat enable keyset getting merged.

image
image
image
image


✅ Checklist

Please ensure the following tasks are completed before requesting a review:

  • My code adheres to the style guidelines of this project.
  • I have run npm run lint or the equivalent linting command.
  • I have added or updated tests (if applicable).
  • My changes have been tested thoroughly in different browsers/resolutions (if applicable).
  • I have updated the documentation (if applicable).
  • I have checked that there are no console errors or warnings.
  • I have verified that the application builds without errors.
  • I have tested responsiveness for mobile and desktop views (if applicable).

PR Type

Enhancement


Description

  • Added support for new quote statuses: Canceled and OfferExpired

    • Updated sidebar and routing to include new statuses
    • Extended quote type definitions and UI handling
  • Displayed discounted amount and keyset information in quote details

    • Show discounted amount for relevant statuses
    • Show keyset ID and active status with badge
  • Improved keyset activation logic and button state

    • Disabled activate keyset button if already active
    • Fetched and displayed keyset active status from backend
  • Enhanced API client and types for keyset info

    • Added new endpoint and types for keyset information

Changes walkthrough 📝

Relevant files
Enhancement
types.gen.ts
Extend quote types and add keyset info types                         

src/generated/client/types.gen.ts

  • Added new quote statuses: Canceled and OfferExpired
  • Extended InfoReply union with new fields (discounted, keyset_id,
    tstamp)
  • Added types for keyset info endpoint and response
  • +37/-3   
    sdk.gen.ts
    Add keyset info API endpoint                                                         

    src/generated/client/sdk.gen.ts

  • Imported new keyset info types
  • Added keysetInfo API endpoint for fetching keyset details
  • +12/-1   
    AppSidebar.tsx
    Add sidebar navigation for new quote statuses                       

    src/components/AppSidebar.tsx

    • Added sidebar links for Offer Expired and Canceled quote statuses
    +8/-0     
    main.tsx
    Add routing for new quote statuses                                             

    src/main.tsx

  • Added routes for Canceled and OfferExpired quote statuses
  • Cleaned up commented code
  • +2/-5     
    QuotePage.tsx
    Show discounted amount and keyset info in quote details   

    src/pages/quotes/QuotePage.tsx

  • Displayed discounted amount for relevant statuses
  • Displayed keyset ID and active status with badge
  • Fetched keyset info and used it to control UI and button state
  • Disabled activate keyset button if already active
  • Cleaned up PayeePublicDataCard
  • +128/-59
    StatusQuotePage.tsx
    Support new quote statuses in status page                               

    src/pages/quotes/StatusQuotePage.tsx

  • Added new quote statuses (Canceled, OfferExpired) to type definitions
  • +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.
  • Copy link

    qodo-merge-pro bot commented Jun 3, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    State Mutation

    The keysetActive variable is directly mutated in the onSubmit handler of the ConfirmDrawer component, which is not a React-recommended pattern. This mutation may not properly trigger re-renders.

      keysetActive = true
    }}
    
    URL Parameter

    The keysetInfo endpoint URL contains a parameter placeholder '{keyset_id}' but it's unclear if this gets properly replaced with the actual value when making the API call.

    url: '/v1/admin/keysets/{keyset_id}',
    ...options

    Copy link

    codecov bot commented Jun 3, 2025

    Codecov Report

    Attention: Patch coverage is 0% with 118 lines in your changes missing coverage. Please review.

    Files with missing lines Patch % Lines
    src/pages/quotes/QuotePage.tsx 0.00% 101 Missing ⚠️
    src/components/AppSidebar.tsx 0.00% 8 Missing ⚠️
    src/generated/client/sdk.gen.ts 0.00% 7 Missing ⚠️
    src/main.tsx 0.00% 2 Missing ⚠️

    📢 Thoughts on this report? Let us know!

    Copy link

    qodo-merge-pro bot commented Jun 3, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix state mutation
    Suggestion Impact:The commit removed the direct assignment to keysetActive as suggested, which would have caused issues since it's a local variable from a React hook

    code diff:

    -            keysetActive = true

    Direct assignment to keysetActive won't work as expected since it's a local
    variable from a React hook. Use state management to update this value properly
    after the keyset activation.

    src/pages/quotes/QuotePage.tsx [418-428]

     onSubmit={() => {
       onActivateKeyset()
       setActivateKeysetConfirmDrawerOpen(false)
    -  keysetActive = true
    +  // The keysetActive state will update automatically when the query refetches
     }}

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 7

    __

    Why: Correctly identifies that direct assignment to keysetActive won't trigger React re-renders since it's a local variable derived from query data.

    Medium
    General
    Improve query refetching

    The query doesn't handle refetching after keyset activation. Add
    refetchOnWindowFocus and include the mutation in the invalidateQueries to ensure
    UI updates correctly after activation.

    src/pages/quotes/QuotePage.tsx [605-612]

     const { data: keysetData } = useQuery({
       queryKey: ["keyset", keysetId],
       queryFn: () =>
         keysetInfo({
           path: { keyset_id: keysetId },
         }),
       enabled: shouldFetchKeyset,
    +  refetchOnWindowFocus: true,
     })
    • Apply / Chat
    Suggestion importance[1-10]: 4

    __

    Why: Minor improvement that adds refetchOnWindowFocus but doesn't address the core issue of invalidating queries after mutation.

    Low
    • Update

    Copy link

    cloudflare-workers-and-pages bot commented Jun 3, 2025

    Deploying wildcat-dashboard with  Cloudflare Pages  Cloudflare Pages

    Latest commit: 76a6883
    Status: ✅  Deploy successful!
    Preview URL: https://19b75125.wildcat-dashboard.pages.dev
    Branch Preview URL: https://stefan-update3.wildcat-dashboard.pages.dev

    View logs

    Copy link

    @zupzup zupzup left a comment

    Choose a reason for hiding this comment

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

    LGTM 👍

    enabled: shouldFetchKeyset,
    })

    let keysetActive = false
    Copy link

    Choose a reason for hiding this comment

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

    Could this be something like

    const keysetActive = keysetData?.data?.active ?? false;
    

    It would also check for null in addition to undefined. Basically, anything that's not { data: { active: true } } would be false, which it looks like you're trying to do here

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Thank you ! Yes I will try your approach it seems simpler.

    @stefanbitcr
    Copy link
    Contributor Author

    stefanbitcr commented Jun 3, 2025

    Display whether ebill is paid @mtbitcr
    image
    image
    image

    @stefanbitcr stefanbitcr merged commit 82ab953 into master Jun 5, 2025
    5 of 6 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.

    Indicate Keyset activation Show offered amount Canceled qoute Expired quote

    3 participants