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

chore(platoform): Swapped all legacy API calls with @keyshade/api-client #584

Conversation

poswalsameer
Copy link
Contributor

@poswalsameer poswalsameer commented Dec 9, 2024

User description

Description

This PR changes all the old API calls made in the platform to new by utilizing the @keyshade/api-client package.

Dependencies

No dependencies.

Future Improvements

Mentions

@rajdip-b

Screenshots of relevant screens

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

enhancement


Description

  • Refactored API calls in the secret page to use SecretController from @keyshade/api-client, replacing legacy methods.
  • Updated the project layout to use ProjectController from @keyshade/api-client, enhancing the data fetching process.
  • Improved error handling and loading state management in both components.
  • Modified data structure references to align with the new API schema.

Changes walkthrough 📝

Relevant files
Enhancement
page.tsx
Refactor secret page to use new API client                             

apps/platform/src/app/(main)/project/[project]/@secret/page.tsx

  • Replaced legacy API calls with SecretController from
    @keyshade/api-client.
  • Updated data fetching logic to use async/await.
  • Modified data structure references to align with new API.
  • Improved error handling and loading state management.
  • +33/-19 
    layout.tsx
    Refactor project layout to use new API client                       

    apps/platform/src/app/(main)/project/[project]/layout.tsx

  • Replaced legacy API calls with ProjectController from
    @keyshade/api-client.
  • Updated data fetching logic to use async/await.
  • Improved error handling.
  • +23/-8   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    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

    Possible Bug
    The pathname.split('/') array access has a syntax error - '[2]' is outside the split call which will cause undefined behavior

    Loading State
    The loading state is set to false before the async API call completes, which could lead to showing empty content while data is still loading

    Type Safety
    Usage of @ts-ignore should be avoided. The type mismatch between the API response and state should be properly handled

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Dec 9, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Fix incorrect string array indexing syntax that would cause path parsing failure
    Suggestion Impact:The incorrect array indexing syntax in pathname.split() was corrected, which aligns with the suggestion to fix the critical bug.

    code diff:

    -        {projectSlug: pathname.split('/'[2])},
    -        {}
    -      )
    +    async function getAllSecretsByProjectSlug() {
    +      const { success, error, data } =
    +        await secretController.getAllSecretsOfProject(
    +          { projectSlug: pathname.split('/')[2] },

    Fix the array indexing syntax in pathname.split() - currently using incorrect string
    literal syntax that will cause incorrect path parsing.

    apps/platform/src/app/(main)/project/[project]/@secret/page.tsx [49]

    -{projectSlug: pathname.split('/'[2])}
    +{projectSlug: pathname.split('/')[2]}
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: The current syntax 'split('/'[2])' is critically incorrect and would cause the path parsing to fail completely. This is a serious bug that would break the entire feature.

    10
    ✅ Fix incorrect loading state management in async data fetching

    Move setIsLoading(false) inside the getAllSecretsByProjectSlug function after data
    is processed, otherwise loading state will be set to false before data is actually
    loaded.

    apps/platform/src/app/(main)/project/[project]/@secret/page.tsx [47-65]

     async function getAllSecretsByProjectSlug(){
    -  const {success, error, data} = await secretController.getAllSecretsOfProject(
    -    {projectSlug: pathname.split('/'[2])},
    -    {}
    -  )
    +  try {
    +    const {success, error, data} = await secretController.getAllSecretsOfProject(
    +      {projectSlug: pathname.split('/'[2])},
    +      {}
    +    )
     
    -  if( success && data ){
    -    //@ts-ignore
    -    setAllSecrets(data)
    -  }
    -  else{
    -    // eslint-disable-next-line no-console -- we need to log the error
    -    console.error(error)
    +    if( success && data ){
    +      //@ts-ignore
    +      setAllSecrets(data)
    +    }
    +    else{
    +      console.error(error)
    +    }
    +  } finally {
    +    setIsLoading(false)
       }
     }
     
     getAllSecretsByProjectSlug()
     
    -setIsLoading(false)
    -

    [Suggestion has been applied]

    Suggestion importance[1-10]: 9

    Why: The current implementation sets loading state to false before the async operation completes, which could lead to showing empty content before data is loaded. Moving it to a finally block ensures correct loading state management.

    9
    General
    ✅ Ensure proper type safety by removing ts-ignore and adding proper type assertion
    Suggestion Impact:The commit removed @ts-ignore and properly typed the data by using GetAllSecretsOfProjectResponse['items'] type and accessing data.items

    code diff:

    -  const [allSecrets, setAllSecrets] = useState<Secret[]>()
    +  const [allSecrets, setAllSecrets] =
    +    useState<GetAllSecretsOfProjectResponse['items']>()
       const [isLoading, setIsLoading] = useState<boolean>(true)
       const pathname = usePathname()
     
       useEffect(() => {
         setIsLoading(true)
     
    -    const secretController = new SecretController(
    -      process.env.NEXT_PUBLIC_BACKEND_URL
    -    )
    +    async function getAllSecretsByProjectSlug() {
    +      const { success, error, data } =
    +        await ControllerInstance.getInstance().secretController.getAllSecretsOfProject(
    +          { projectSlug: pathname.split('/')[2] },
    +          {}
    +        )
     
    -    async function getAllSecretsByProjectSlug(){
    -      const {success, error, data} = await secretController.getAllSecretsOfProject(
    -        {projectSlug: pathname.split('/'[2])},
    -        {}
    -      )
    -
    -      if( success && data ){
    -        //@ts-ignore
    -        setAllSecrets(data)
    -      }
    -      else{
    +      if (success && data) {
    +        setAllSecrets(data.items)

    Remove the @ts-ignore comment and properly type the API response data to match the
    Secret[] type expected by setAllSecrets.

    apps/platform/src/app/(main)/project/[project]/@secret/page.tsx [54-55]

    -//@ts-ignore
    -setAllSecrets(data)
    +setAllSecrets(data as Secret[])
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: While using @ts-ignore is not ideal, the suggestion to use type assertion provides better type safety and makes the code's intent clearer, though it's not addressing a critical runtime issue.

    5

    💡 Need additional feedback ? start a PR chat

    @keyshade-xyz keyshade-xyz deleted a comment from codiumai-pr-agent-free bot Dec 9, 2024
    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

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

    can you delete the existing controllers in /lib/api-functions?

    @poswalsameer
    Copy link
    Contributor Author

    can you delete the existing controllers in /lib/api-functions?

    Sure, will do it.

    @poswalsameer
    Copy link
    Contributor Author

    can you delete the existing controllers in /lib/api-functions?

    deleted two files inside the lib folder, controller-instance.ts is getting utilised in the auth and utils.ts is related to tailwind, so kept these two files as it is.

    @rajdip-b
    Copy link
    Member

    We also have auth functions in API Client. Could you please try to refactor that aswell? Do lmk if you run into any issues.

    @poswalsameer
    Copy link
    Contributor Author

    We also have auth functions in API Client. Could you please try to refactor that aswell? Do lmk if you run into any issues.

    Sure, will do it.

    @rajdip-b rajdip-b changed the title fix: Changed all the legacy api calls to new one by using the api-client package chore(platoform): Swapped all legacy API calls with @keyshade/api-client Dec 13, 2024
    @rajdip-b
    Copy link
    Member

    @poswalsameer could you please wrap this up? it would be very helpful to us

    @poswalsameer
    Copy link
    Contributor Author

    @poswalsameer could you please wrap this up? it would be very helpful to us

    Hi, I am done with this. Removed all the legacy code I found in the codebase.

    @rajdip-b
    Copy link
    Member

    I think the auth functions are still missing as per our previous convo

    @poswalsameer
    Copy link
    Contributor Author

    I think the auth functions are still missing as per our previous convo

    You talking about this file inside the api-client, because I didn't find any other file related to auth.
    Screenshot 2024-12-17 163755

    @rajdip-b
    Copy link
    Member

    can you delete the existing controllers in /lib/api-functions?

    deleted two files inside the lib folder, controller-instance.ts is getting utilised in the auth and utils.ts is related to tailwind, so kept these two files as it is.

    Im talking about this

    @rajdip-b rajdip-b merged commit c600db7 into keyshade-xyz:develop Dec 17, 2024
    7 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