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

feat(platform): implement secrets version history and rollback functionality #794

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

anaypurohit0907
Copy link

@anaypurohit0907 anaypurohit0907 commented Feb 23, 2025

User description

Description

Added complete version history tracking and rollback capabilities for secrets. Users can now view version history of secrets across environments and roll back to previous versions when needed.

Fixes #700
Fixes #563

Dependencies

No new dependencies were added.

Future Improvements

As discussed on call with @rajdip-b we wiil be integrating the rollback api.

Mentions

Team name for fossHack20225: BunLock
Team Members:
@anaypurohit0907
@Dhanushranga1
@hanish-rishen

Thanks @rajdip-b for guiding us and answering all of our questions.

Screenshots of relevant screens

image

image

image

Tests

image

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, Tests


Description

  • Added rollback functionality for secrets and variables.

    • Created RollbackSecretSheet and RollbackVariableSheet components for version history and rollback actions.
    • Integrated rollbackSecretOpenAtom and rollbackVariableOpenAtom for state management.
    • Implemented UI for version history with rollback confirmation dialogs.
  • Enhanced SecretCard and VariableCard components.

    • Added "Show Version History" context menu option.
    • Improved type safety and accessibility.
  • Introduced ConfirmRollbackDialog for rollback confirmation.

    • Ensured proper focus management and accessibility compliance.
  • Updated project state management in store/index.ts.

    • Added atoms for rollback dialog state.

Changes walkthrough 📝

Relevant files
Enhancement
9 files
page.tsx
Integrated rollback functionality for secrets                       
+34/-41 
page.tsx
Integrated rollback functionality for variables                   
+24/-25 
index.tsx
Added rollback confirmation dialog for secrets                     
+60/-0   
index.tsx
Created rollback sheet for secrets with version history   
+217/-0 
index.tsx
Enhanced secret card with version history option                 
+24/-9   
index.tsx
Added rollback confirmation dialog for variables                 
+60/-0   
index.tsx
Created rollback sheet for variables with version history
+224/-0 
index.tsx
Enhanced variable card with version history option             
+17/-3   
index.ts
Added rollback state management atoms                                       
+3/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • hanish-rishen and others added 5 commits February 23, 2025 07:06
    …ionality
    
    Changes made:
    - Added rollbackVariableOpenAtom to manage rollback dialog state
    - Enhanced VariableCard component with version history context menu item
    - Created RollbackVariableSheet component for displaying version history
    - Added proper focus and accessibility management for dialogs
    - Implemented UI for version timeline with rollback actions
    - Added proper types for API responses and client data
    
    Key files modified:
    1. /store/index.ts
       - Added rollbackVariableOpenAtom for managing rollback dialog state
    
    2. /components/dashboard/variable/variableCard/index.tsx
       - Added version history context menu item
       - Added rollback state management
       - Implemented handleVersionHistoryClick
    
    3. /components/dashboard/variable/rollbackVariableSheet/index.tsx
       - Created new component for version history UI
       - Implemented version timeline with rollback buttons
       - Added proper focus management and accessibility
       - Added version comparison and rollback confirmation
    
    4. /@variable/page.tsx
       - Added RollbackVariableSheet integration
       - Improved dialog management with data-inert attribute
       - Enhanced type safety for API responses
    
    Testing:
    - Right-click on variable opens context menu
    - "Show Version History" opens timeline view
    - Rollback button triggers confirmation dialog
    - Focus management works correctly
    - Accessibility standards maintained
    - Fixed secret value decryption in RollbackSecretSheet
    - Updated version number format to use "v" prefix
    - Removed unused utility functions and fix TypeScript errors
    - Added isDecrypted prop to handle encrypted/decrypted values
    - Improved type safety in SecretCard component
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    700 - PR Code Verified

    Compliant requirements:

    • Added context menu with "Show Version History" option
    • Implemented version history view in sheet component
    • Added rollback functionality through UI

    Requires further human verification:

    • Verify that rollback API integration works correctly (noted as TODO in code)
    • Test rollback functionality end-to-end
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Error Handling

    The rollback functionality has placeholder success handling but lacks proper error handling for failed API calls

        })
        setRollbackDetails(null)
        setIsRollbackSecretOpen(false) // Close the sheet after rollback
      } finally {
        setIsLoading(false)
        toast.dismiss()
      }
    }, [rollbackDetails, setIsRollbackSecretOpen])
    Duplicate Code

    The rollback sheet implementation is nearly identical between secret and variable components. Consider extracting common functionality into a shared component

    'use client'
    
    import { useCallback, useEffect, useState } from 'react'
    import { toast } from 'sonner'
    import { useAtom, useAtomValue } from 'jotai'
    import { History } from 'lucide-react'
    import * as dayjs from 'dayjs'
    import ConfirmRollbackDialog from '../confirmRollbackDialog'
    import {
      Sheet,
      SheetContent,
      SheetDescription,
      SheetHeader,
      SheetTitle
    } from '@/components/ui/sheet'
    import {
      Accordion,
      AccordionContent,
      AccordionItem,
      AccordionTrigger
    } from '@/components/ui/accordion'
    import {
      rollbackVariableOpenAtom,
      selectedVariableAtom
    } from '@/store'
    import ControllerInstance from '@/lib/controller-instance'
    
    /**
     * Formats a date into a standardized string representation.
     * 
     * @param date - The date to format
     * @returns The formatted date string in "D MMMM, YYYY h:mm A" format
     */
    const formatDate = (date: Date | string): string => {
      return dayjs(date).format('D MMMM, YYYY h:mm A')
    }
    
    interface Revision {
      version: number
      value: string
      createdBy?: {  // Make createdBy optional
        id: string
        name: string
        profilePictureUrl: string | null
      } | null  // Allow null
      createdOn: string
      id: string
      environmentId: string
      createdById: string
      variableId: string
    }
    
    interface EnvironmentRevisions {
      environmentName: string
      environmentSlug: string
      revisions: Revision[]
    }
    
    interface RollbackDetails {
      environmentSlug: string
      version: number
    }
    
    export default function RollbackVariableSheet() {
      const [isRollbackVariableOpen, setIsRollbackVariableOpen] = useAtom(
        rollbackVariableOpenAtom
      )
      const selectedVariableData = useAtomValue(selectedVariableAtom)
      const [isLoading, setIsLoading] = useState<boolean>(false)
      const [environmentRevisions, setEnvironmentRevisions] = useState<EnvironmentRevisions[]>([])
      const [rollbackDetails, setRollbackDetails] = useState<RollbackDetails | null>(null)
    
      useEffect(() => {
        if (selectedVariableData && isRollbackVariableOpen) {
          Promise.all(
            selectedVariableData.values.map(async (value) => {
              const { data, success } = await ControllerInstance.getInstance()
                .variableController.getRevisionsOfVariable({
                  variableSlug: selectedVariableData.variable.slug,
                  environmentSlug: value.environment.slug
                })
              if (success && data) {
                return {
                  environmentName: value.environment.name,
                  environmentSlug: value.environment.slug,
                  revisions: data.items
                } as EnvironmentRevisions
              }
              return null
            })
          ).then((results) => {
            setEnvironmentRevisions(results.filter(Boolean) as EnvironmentRevisions[])
          })
        }
      }, [selectedVariableData, isRollbackVariableOpen])
    
      const handleRollbackClick = useCallback((environmentSlug: string, version: number) => {
        setRollbackDetails({ environmentSlug, version })
      }, [])
    
      const handleConfirmRollback = useCallback(() => {
        if (!rollbackDetails) return
    
        setIsLoading(true)
        toast.loading('Rolling back variable...')
    
        // TODO: Implement actual rollback API call
        try {
          toast.success('Variable rolled back successfully', {
            description: (
              <p className="text-xs text-emerald-300">
                The variable has been rolled back to version {rollbackDetails.version}
              </p>
            )
          })
          setRollbackDetails(null)
          setIsRollbackVariableOpen(false) // Close the sheet after rollback
        } finally {
          setIsLoading(false)
          toast.dismiss()
        }
      }, [rollbackDetails, setIsRollbackVariableOpen])
    
      return (
        <>
          <Sheet
            onOpenChange={(open) => {
              setIsRollbackVariableOpen(open)
              if (!open) {
                setRollbackDetails(null) // Clear rollback details when sheet closes
              }
            }}
            open={isRollbackVariableOpen}
          >
            <SheetContent 
              className="border-white/15 bg-[#222425] w-[1400px] max-w-[95vw]"
              onEscapeKeyDown={(e) => {
                e.preventDefault()
                setIsRollbackVariableOpen(false)
              }}
              onInteractOutside={(e) => {
                e.preventDefault()
              }}
            >
              <SheetHeader>
                <SheetTitle className="text-white">
                  {selectedVariableData?.variable.name} Revisions
                </SheetTitle>
                <SheetDescription className="text-white/60">
                  <b>Version History</b>
                </SheetDescription>
              </SheetHeader>
    
              <div className="mt-8 overflow-y-auto pr-2">
                <Accordion className="w-full space-y-2" collapsible type="single">
                  {environmentRevisions.map((env) => (
                    <AccordionItem 
                      className="border border-white/10 rounded-xl px-6 bg-neutral-800/50"
                      key={env.environmentSlug}
                      value={env.environmentSlug}
                    >
                      <AccordionTrigger className="hover:no-underline py-6">
                        <div className="flex items-center gap-2">
                          <span className="text-white text-lg">{env.environmentName}</span>
                          <span className="text-xs text-white/60">
                            ({env.revisions.length} versions)
                          </span>
                        </div>
                      </AccordionTrigger>
                      <AccordionContent>
                        <div className="space-y-6 py-4">
                          {env.revisions.map((revision) => (
                            <div className="relative group" key={revision.version}>
                              {/* Timeline dot and line */}
                              <div className="absolute left-0 top-0 h-full w-0.5 bg-white/20">
                                <div className="absolute -left-[2px] top-0 h-1 w-1 rounded-full bg-white/60" />
                              </div>
    
                              {/* Version content with rollback icon */}
                              <div className="ml-6 flex flex-col group-hover:bg-white/5 rounded-lg transition-colors p-5 relative">
                                <button 
                                  className="absolute right-4 top-4 opacity-0 group-hover:opacity-100 transition-opacity"
                                  disabled={isLoading}
                                  onClick={() => handleRollbackClick(env.environmentSlug, revision.version)}
                                  type="button"
                                >
                                  <History className="w-5 h-5 text-white/60 hover:text-white" />
                                </button>
    
                                <div className="flex-grow space-y-3 pr-8">
                                  <div className="flex items-center gap-3">
                                    <div className="text-base text-white/80 flex-grow">
                                      {revision.value}
                                      <span className="px-2 py-1 rounded-md bg-blue-500/20 text-blue-400 text-sm font-medium ml-3">
                                        v{revision.version}
                                      </span>
                                    </div>
                                  </div>
                                  <div className="text-sm text-white/40">
                                    {formatDate(revision.createdOn)} by {revision.createdBy?.name ?? 'Unknown'}
                                  </div>
                                </div>
                              </div>
                            </div>
                          ))}
                        </div>
                      </AccordionContent>
                    </AccordionItem>
                  ))}
                </Accordion>
              </div>
            </SheetContent>
          </Sheet>
    
          <ConfirmRollbackDialog
            isLoading={isLoading}
            isOpen={rollbackDetails !== null}
            onClose={() => setRollbackDetails(null)}
            onConfirm={handleConfirmRollback}
            version={rollbackDetails?.version ?? 0}
          />
        </>
      )
    }

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Feb 23, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Implement missing rollback API call

    The rollback functionality is currently incomplete. The handleConfirmRollback
    function only shows a success toast without making the actual API call to
    rollback the secret. Implement the actual rollback API call using the
    ControllerInstance.

    apps/platform/src/components/dashboard/secret/rollbackSecretSheet/index.tsx [101-123]

    -const handleConfirmRollback = useCallback(() => {
    -  if (!rollbackDetails) return
    +const handleConfirmRollback = useCallback(async () => {
    +  if (!rollbackDetails || !selectedSecretData) return
     
       setIsLoading(true)
       toast.loading('Rolling back Secret...')
     
    -  // TODO: Implement actual rollback API call
       try {
    -    toast.success('Secret rolled back successfully', {
    -      description: (
    -        <p className="text-xs text-emerald-300">
    -          The Secret has been rolled back to version {rollbackDetails.version}
    -        </p>
    -      )
    -    })
    -    setRollbackDetails(null)
    -    setIsRollbackVariableOpen(false) // Close the sheet after rollback
    +    const { success } = await ControllerInstance.getInstance()
    +      .secretController.rollbackSecret({
    +        secretSlug: selectedSecretData.secret.slug,
    +        environmentSlug: rollbackDetails.environmentSlug,
    +        version: rollbackDetails.version
    +      })
    +
    +    if (success) {
    +      toast.success('Secret rolled back successfully', {
    +        description: (
    +          <p className="text-xs text-emerald-300">
    +            The Secret has been rolled back to version {rollbackDetails.version}
    +          </p>
    +        )
    +      })
    +      setRollbackDetails(null)
    +      setIsRollbackSecretOpen(false)
    +    }
    +  } catch (error) {
    +    toast.error('Failed to rollback secret')
       } finally {
         setIsLoading(false)
         toast.dismiss()
       }
    -}, [rollbackDetails, setIsRollbackVariableOpen])
    +}, [rollbackDetails, selectedSecretData, setIsRollbackSecretOpen])

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 9

    __

    Why: The suggestion addresses a critical missing functionality - the actual API call to rollback secrets. The current code only shows a success toast without performing the rollback operation, which is a major functional gap.

    High
    • Update

    </div>
    )
    }
    .finally(() => { if (!isLoading) { setIsLoading(false) } })
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Revert back this line, i dont think the change is relevant.


    <div className="flex h-[5rem] w-[30.25rem] flex-col items-center justify-center gap-4">
    <p className="h-[2.5rem] w-[30.25rem] text-center text-[32px] font-[400]">
    Declare your first secret
    Secrete your firstSecret
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Revert this change

    </p>
    <p className="h-[1.5rem] w-[30.25rem] text-center text-[16px] font-[500]">
    Declare and store a secret against different environments
    Declare and store a Secret against different environments
    Copy link
    Member

    Choose a reason for hiding this comment

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

    revert this aswell

    </p>
    </div>

    <Button
    className="h-[2.25rem] rounded-md bg-white text-black hover:bg-gray-300"
    onClick={() => setIsCreateSecretOpen(true)}
    >
    Create secret
    CreateSecret
    Copy link
    Member

    Choose a reason for hiding this comment

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

    And this

    {isDeleteSecretOpen && selectedSecret ? (
    <ConfirmDeleteSecret />
    ) : null}
    <Accordion className="flex h-fit w-full flex-col gap-4" collapsible type="single">
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Any special reason for removing the scroll area?

    }
    }, [selectedSecretData, isRollbackSecretOpen])

    const handleRollbackClick = useCallback((environmentSlug: string, version: number) => {
    Copy link
    Member

    Choose a reason for hiding this comment

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

    instead of passing parameters, callback should get them from state variables and update the deps array

    revisions: Revision[]
    }

    interface RollbackDetails {
    Copy link
    Member

    Choose a reason for hiding this comment

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

    This can be inlined to the state

    interface SecretCardProps {
    secretData: Secret
    secretData: Secret | undefined
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Secret data can never be undefined by design

    Copy link
    Member

    Choose a reason for hiding this comment

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

    text changes should be similar to the ones i mentioned in variables dialog

    Copy link
    Member

    Choose a reason for hiding this comment

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

    type changes and logic changes should be similar to the ones mentioned in secrets

    @rajdip-b
    Copy link
    Member

    Also, can you post a few other screenshots?

    • multiple environments
    • multiple revisions in one environment

    @anaypurohit0907
    Copy link
    Author

    alright, thanks for the reiview, ill make those changes

    Copy link

    @anaypurohit0907, please resolve all open reviews!

    Copy link

    @anaypurohit0907, please resolve all open reviews; otherwise this PR will be closed after Sun Mar 02 2025 14:05:52 GMT+0000 (Coordinated Universal Time)!

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Mar 2, 2025

    hey @anaypurohit0907, any updates?

    @anaypurohit0907
    Copy link
    Author

    yes i have made all the changes you mentioned except to rollbacksecretsheet ill do that by monday , im so sorry for the delay.

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Mar 3, 2025

    no worries man! i'm converting this to a draft. please open it up again once its good for review :) appreciate your efforts

    @rajdip-b rajdip-b marked this pull request as draft March 3, 2025 14:44
    @kriptonian1 kriptonian1 closed this Mar 8, 2025
    @kriptonian1 kriptonian1 reopened this Mar 8, 2025
    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.

    PLATFORM: Rollback Secrets PLATFORM: Rollback variables
    4 participants