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.

Sign up for GitHub

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

Jump to bottom

Fixed Issue #1865 #1869

Closed
wants to merge 2 commits into from
Closed

Fixed Issue #1865 #1869

wants to merge 2 commits into from

Conversation

shbenzer
Copy link

@shbenzer shbenzer commented Oct 9, 2024 •
edited
Loading

User description

Fixed #1865

Description

I cannot for the life of me figure out where the spaces come from - the problem appears before calling JSON.stringify(), so the issue is in the ide code. In version 3.17.2 this works, but current 4.0.2 does not, and when comparing differences in the files I don't see any changes to the WindowHandleName object's processing.

suspect that we're accidentally setting the text name from the index.ts instead of the key somewhere, but I'm struggling to find the bug.

In order to temporarily fix this, I wrote a function that takes the JSON.stringify() output during the save process and removes spaces from keys in the json string.

Motivation and Context

Issue #186

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix

Description

  • Introduced a new method UpdatedJsonStringify to address JSON serialization issues by removing spaces from object parameters.
  • Updated the save_v3 method to utilize the new UpdatedJsonStringify method, ensuring consistent JSON output.
  • This change resolves the issue where JSON.stringify() was not functioning as expected in version 4.0.2.

Changes walkthrough 📝

Relevant files
Bug fix
index.ts
Fix JSON serialization by removing spaces from object parameters

packages/selenium-ide/src/main/session/controllers/Projects/index.ts

  • Added UpdatedJsonStringify method to remove spaces from JSON object
    parameters.
  • Modified save_v3 method to use UpdatedJsonStringify for JSON
    serialization.
    		•  +7/-1     

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

    @codiumai-pr-agent-pro CodiumAI PR-Agent Pro
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Potential Performance Issue
    The new UpdatedJsonStringify method uses a regular expression to process the entire JSON string, which might be inefficient for large projects.

    Possible Bug
    The UpdatedJsonStringify method may remove spaces from string values within the JSON, potentially altering the data unintentionally.

    @codiumai-pr-agent-pro CodiumAI PR-Agent Pro
    Copy link

    codiumai-pr-agent-pro bot commented Oct 9, 2024 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Implement error handling for file writing operations to improve robustness

    Consider handling potential errors that may occur during file writing operations in
    the save_v3 method.

    packages/selenium-ide/src/main/session/controllers/Projects/index.ts [206-211]

     async save_v3(filepath: string): Promise<boolean> {
-  await fs.writeFile(filepath, this.UpdatedJsonStringify(JSON.stringify(this.project, undefined, 2)))
-  this.recentProjects.add(filepath)
-  this.session.projects.filepath = filepath
-  return true
+  try {
+    await fs.writeFile(filepath, this.UpdatedJsonStringify(JSON.stringify(this.project, undefined, 2)))
+    this.recentProjects.add(filepath)
+    this.session.projects.filepath = filepath
+    return true
+  } catch (error) {
+    console.error('Error saving project:', error)
+    return false
+  }
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Implementing error handling for file writing operations in the save_v3 method significantly improves the robustness and reliability of the code by ensuring that errors are caught and handled gracefully, preventing potential application crashes.

    		9
    Add input validation to ensure the method receives a valid JSON string

    Consider adding input validation to ensure that the json parameter is a valid JSON
    string before processing it.

    packages/selenium-ide/src/main/session/controllers/Projects/index.ts [25-29]

     UpdatedJsonStringify(json: string): string {
+  try {
+    JSON.parse(json);
+  } catch (e) {
+    throw new Error('Invalid JSON string');
+  }
   return json.replace(/("(?:\\"|[^"])*")\s*:/g, (_, p1) => {
     return p1.replace(/\s/g, '') + ':';
   });
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding input validation to check if the input is a valid JSON string is a crucial improvement that prevents potential runtime errors and ensures the method operates on valid data, enhancing the robustness of the code.

    		8
    Enhancement
    Improve method naming to better reflect its specific functionality

    Consider using a more descriptive name for the UpdatedJsonStringify method, such as
    removeSpacesFromJsonKeys, to better reflect its specific functionality.

    packages/selenium-ide/src/main/session/controllers/Projects/index.ts [25-29]

    -UpdatedJsonStringify(json: string): string {
+removeSpacesFromJsonKeys(json: string): string {
   return json.replace(/("(?:\\"|[^"])*")\s*:/g, (_, p1) => {
     return p1.replace(/\s/g, '') + ':';
   });
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to rename UpdatedJsonStringify to removeSpacesFromJsonKeys enhances code readability by providing a more descriptive method name that clearly indicates its functionality, which is beneficial for maintainability.

    		7

    💡 Need additional feedback ? start a PR chat

    @shbenzer
    Copy link
    Author

    shbenzer commented Oct 18, 2024 •
    edited
    Loading

    I've changed my mind about this PR. It's not reliable enough (nor terribly readable, I never like using regex...) and prevents the ability to use a space in json keys moving forward.

    @shbenzer shbenzer closed this Oct 18, 2024
    @shbenzer shbenzer deleted the Issue1865 branch October 18, 2024 20:48
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    Bug fix Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    "Window Handle Name" instead of windowHandleName in .side project
    1 participant
    @shbenzer