Skip to content

feat(client): delete multiple jobs changes #1160

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

Merged
merged 18 commits into from
May 30, 2025
Merged

Conversation

SindhuHarika
Copy link
Contributor

Description

Made changes to implement multiple delete functionality in delete the jobs

Changes Made

Key changes are for enabling deletion of multiple jobs at once.

How to Test

In the Jobs page of settings after selecting , multiple files users can delete them.

@SindhuHarika SindhuHarika requested a review from a team as a code owner May 16, 2025 07:48
Copy link

@CodiumAI-Agent /describe

@CodiumAI-Agent
Copy link

Title

feat(client): delete multiple jobs changes


User description

Description

Made changes to implement multiple delete functionality in delete the jobs

Changes Made

Key changes are for enabling deletion of multiple jobs at once.

How to Test

In the Jobs page of settings after selecting , multiple files users can delete them.


PR Type

Enhancement


Description

  • Add multiple job deletion support

  • Update deleteJob to accept arrays

  • Modify DeleteJobModal for multiple jobs

  • Add "Delete Selected" button UI


Changes walkthrough 📝

Relevant files
Enhancement
DeleteJobModal.tsx
Support multiple job deletion in modal                                     

packages/client/src/pages/jobs/DeleteJobModal.tsx

  • Change job prop to Job[] type
  • Update deleteJob signature to accept arrays
  • Display generic text for multiple deletions
  • Map jobs to id/group arrays on confirm
  • +9/-5     
    JobsPage.tsx
    Implement multi-job deletion functionality                             

    packages/client/src/pages/jobs/JobsPage.tsx

  • Add state for multiple job deletion flags
  • Implement deleteMultipleJobs function
  • Show "Delete Selected" button on selection
  • Update deleteJob to handle id/group arrays
  • +64/-9   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    @CodiumAI-Agent /review

    Copy link

    @CodiumAI-Agent /improve

    @CodiumAI-Agent
    Copy link

    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

    Query Formatting

    The pixel query is constructed with inconsistent quoting and spacing (e.g., using join(' , ') and nested quotes), which may produce invalid queries. Consider sanitizing inputs and using a consistent separator like ', '.

    let pixel;
    if (jobId.length > 1 && jobGroup.length > 1) {
        const jobIdStr = `"${jobId.join(' , ')}"`;
        const jobGroupStr = `"${jobGroup.join(' , ')}"`;
        pixel = `META | RemoveJobFromDB(jobId=[${jobIdStr}], jobGroup=[${jobGroupStr}]) `;
    } else {
        pixel = 'META | RemoveJobFromDB(';
        pixel += 'jobId=["' + jobId[0] + '"], ';
        pixel += 'jobGroup=["' + jobGroup[0] + '"]) ';
    }
    Empty Selection Handling

    When the job array is empty, the modal message uses job[0]?.name, resulting in "undefined". Add a guard or default text to handle zero-selection scenarios gracefully.

    Are you sure you want to delete{' '}
    {job.length > 1 ? 'all selected jobs' : job[0]?.name}?
    This action is permanent.
    Typo in Notification

    The success message for multiple deletions misspells "Successfully" as "Sucessfully". Correct the spelling to maintain UX quality.

    jobId.length > 1 && jobGroup.length > 1
        ? `Sucessfully deleted all selected files`
        : `Successfully deleted ${type}`,

    @CodiumAI-Agent
    Copy link

    CodiumAI-Agent commented May 16, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 34e7de3

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix typo in state variable name

    Rename the deleteMutliple state variable to deleteMultiple to correct the typo and
    ensure all references match the intended naming. This prevents potential undefined
    variable errors when toggling multi-delete mode.

    packages/client/src/pages/jobs/JobsPage.tsx [64-65]

     const [jobsToDelete, setJobsToDelete] = useState<Job[]>([]);
    -const [deleteMutliple, setDeleteMultiple] = useState<boolean>(false);
    +const [deleteMultiple, setDeleteMultiple] = useState<boolean>(false);
    Suggestion importance[1-10]: 6

    __

    Why: Renaming the state variable deleteMutliple to deleteMultiple corrects a typo and avoids potential undefined variable errors when toggling multi-delete mode.

    Low
    General
    Simplify side-effect conditionals

    Replace the nested ternary operators used solely for side effects with a clear
    if/else block. This improves readability and ensures no unintended values are
    returned.

    packages/client/src/pages/jobs/JobsPage.tsx [166-183]

     monolithStore.runQuery(pixel).then((response) => {
         const type = response.pixelReturn[0].operationType;
         if (type.indexOf('ERROR') === -1) {
             notification.add({ ... });
    -        jobId.length > 1 && jobGroup.length > 1
    -            ? setJobsToDelete([])
    -            : setJobToDelete(null);
    -        jobId.length > 1 && jobGroup.length > 1
    -            ? setDeleteMultiple(false)
    -            : '';
    +        if (jobId.length > 1) {
    +            setJobsToDelete([]);
    +            setDeleteMultiple(false);
    +        } else {
    +            setJobToDelete(null);
    +        }
             getJobs();
         }
     });
    Suggestion importance[1-10]: 6

    __

    Why: Replacing nested ternaries with an if/else block improves readability and ensures no unintended values are returned in side-effect code.

    Low
    Clean up pixel string formatting

    Avoid embedding extra quotes and spaces by wrapping each ID and group in quotes,
    then joining them with a comma and space. This produces cleaner pixel strings and
    prevents double-quoting issues.

    packages/client/src/pages/jobs/JobsPage.tsx [158-160]

    -const jobIdStr = `"${jobId.join(' , ')}"`;
    -const jobGroupStr = `"${jobGroup.join(' , ')}"`;
    -pixel = `META | RemoveJobFromDB(jobId=[${jobIdStr}], jobGroup=[${jobGroupStr}]) `;
    +const quotedIds = jobId.map((id) => `"${id}"`).join(', ');
    +const quotedGroups = jobGroup.map((g) => `"${g}"`).join(', ');
    +pixel = `META | RemoveJobFromDB(jobId=[${quotedIds}], jobGroup=[${quotedGroups}]) `;
    Suggestion importance[1-10]: 5

    __

    Why: Refactoring the pixel string assembly using map and join produces cleaner and more maintainable code and prevents double-quoting issues.

    Low

    Previous suggestions

    Suggestions up to commit b7a6b5c
    CategorySuggestion                                                                                                                                    Impact
    General
    Simplify side‐effect logic

    Replace the ternary-based side‐effects with a clear if/else block to avoid returning
    an unused empty string and improve readability.

    packages/client/src/pages/jobs/JobsPage.tsx [178-183]

    -jobId.length > 1 && jobGroup.length > 1
    -    ? setJobsToDelete([])
    -    : setJobToDelete(null);
    -jobId.length > 1 && jobGroup.length > 1
    -    ? setDeleteMultiple(false)
    -    : '';
    +if (jobId.length > 1) {
    +    setJobsToDelete([]);
    +    setDeleteMultiple(false);
    +} else {
    +    setJobToDelete(null);
    +}
    Suggestion importance[1-10]: 6

    __

    Why: Converting ternary-based side effects into a clear if/else block avoids returning unused values and improves readability.

    Low
    Make modal title pluralizable

    Update the modal title to correctly reflect when multiple jobs are being deleted by
    making it dynamic based on the length of job.

    packages/client/src/pages/jobs/DeleteJobModal.tsx [14]

    -<Modal.Title>Delete Job</Modal.Title>
    +<Modal.Title>
    +  {job.length > 1 ? 'Delete Jobs' : 'Delete Job'}
    +</Modal.Title>
    Suggestion importance[1-10]: 5

    __

    Why: Dynamically pluralizing the modal title improves UX by accurately reflecting singular or multiple job deletions.

    Low
    Fix spelling of deleteMultiple

    Rename deleteMutliple and related functions to deleteMultiple to fix the spelling
    and improve code clarity.

    packages/client/src/pages/jobs/JobsPage.tsx [65-526]

    -const [deleteMutliple, setDeleteMultiple] = useState<boolean>(false);
    -const deleteMutlipleJobs = () => {
    +const [deleteMultiple, setDeleteMultiple] = useState<boolean>(false);
    +const deleteMultipleJobs = () => {
         setDeleteMultiple(true);
         const rowsToBeDeleted = jobs.filter((job) =>
             rowSelectionModel.includes(job.id),
         );
         setJobsToDelete(rowsToBeDeleted);
     };
    Suggestion importance[1-10]: 4

    __

    Why: Renaming deleteMutliple to deleteMultiple fixes a typo and enhances code clarity across related declarations.

    Low
    Possible issue
    Fix typo in success message

    Correct the typo in the success message keying “Successfully” instead of
    “Sucessfully”.

    packages/client/src/pages/jobs/JobsPage.tsx [173-176]

     message:
         jobId.length > 1 && jobGroup.length > 1
    -        ? `Sucessfully deleted all selected files`
    +        ? `Successfully deleted all selected files`
             : `Successfully deleted ${type}`,
    Suggestion importance[1-10]: 4

    __

    Why: Correcting “Sucessfully” to “Successfully” improves professionalism with minimal code impact.

    Low

    @SindhuHarika SindhuHarika linked an issue May 16, 2025 that may be closed by this pull request
    3 tasks
    message: `Successfully deleted ${type}`,
    message:
    jobId.length > 1 && jobGroup.length > 1
    ? `Successfully deleted all selected files`
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Change this to "... selected jobs"

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    committed the change.

    @johbaxter
    Copy link
    Contributor

    image

    image

    Error handling toast message, instead of showing removal was successful

    @SindhuHarika
    Copy link
    Contributor Author

    committed the required changes for error handling

    @johbaxter
    Copy link
    Contributor

    image

    There may be a follow up bugfix ticket

    @johbaxter johbaxter merged commit ccbc984 into dev May 30, 2025
    3 checks passed
    @johbaxter johbaxter deleted the 1143-delete-mutliple-jobs branch May 30, 2025 16:00
    Copy link

    @CodiumAI-Agent /update_changelog

    @CodiumAI-Agent
    Copy link

    Changelog updates: 🔄

    2025-05-30 #1160

    Added

    • Support for deleting multiple jobs at once via a new “Delete Selected” button on the Jobs page.
    • Batch delete modal and backend query updated to handle removal of multiple jobs.

    to commit the new content to the CHANGELOG.md file, please type:
    '/update_changelog --pr_update_changelog.push_changelog_changes=true'

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Create multi-delete functionality for jobs (FE)
    5 participants