Skip to content

fix: Historical user conversation data has been cleared #2843

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 1 commit into from
Apr 10, 2025

Conversation

shaohuzhang1
Copy link
Contributor

fix: Historical user conversation data has been cleared

Copy link

f2c-ci-robot bot commented Apr 10, 2025

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link

f2c-ci-robot bot commented Apr 10, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

localStorage.setItem(`${token}accessToken`, res.data)
sessionStorage.setItem(`${token}accessToken`, res.data)
localStorage.setItem(`${token}-accessToken`, res.data)
sessionStorage.setItem(`${token}-accessToken`, res.data)
resolve(res)
})
.catch((error) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code snippet has a couple of small optimizations you might consider:

  1. Consistent Item Name Format: It seems that using different storage methods (localStorage and sessionStorage) with the same item name (${token}accessToken) can lead to unexpected behavior if keys overlap due to differences in case sensitivity cross-browsers.

    // Recommended change: Use different item names for each store type
    localStorage.setItem(`${token}-accessToken`, res.data);
    sessionStorage.setItem(`${token.toUpperCase()}-accessToken`, res.data);  // Ensure consistency in casing

2. **Handling Error Cases More Explicitly**: The current error handling does not explicitly catch or log errors. This makes it harder to debug issues related to API responses or server-side problems.

3. **TypeScript Annotations**: If this is TypeScript code, adding appropriate types to arguments and return values could improve readability and maintainability.

```typescript
// Type annotations
const useApplicationStore = defineStore({
  state() {
    return {
      applicationApi: null as any,
      token: '' as string,
      loading: false as boolean,
    };
  },
  actions: {
    setToken(newToken: string): void {
      this.token = newToken;
    },
    postAppAuthentication(
      token: string,
      loading: boolean,
      authentication_value: string | null
    ): Promise<any> { // Add more specific typing as needed
      this.loading = true;  // You might want to start loading here too

      return this.applicationApi.postAppAuthentication(token, loading, authentication_value).then((res) => {
        localStorage.setItem(`${token.toLocaleUpperCase()}-accessToken`, res.data)  // Adjusted format for uniformity
        this.setToken(token) // Assuming you need to update your main context with the new token
        return res; // Return the response promise to allow chaining
      }).catch((error: any) => {  // Catch and handle errors appropriately
        console.error('Error during authentication:', error.message || error.toString());
        reject(error);
      });
    },
  },
});

By implementing these changes, the code becomes cleaner, more robust, and adheres better to best practices for both syntax and functionality.

@shaohuzhang1 shaohuzhang1 merged commit 0213aff into main Apr 10, 2025
4 of 5 checks passed
@shaohuzhang1 shaohuzhang1 deleted the pr@main@fix_conversation branch April 10, 2025 01:48
if (token) {
return token
}
const local_token = localStorage.getItem(`${token}accessToken`)
const local_token = localStorage.getItem(`${token}-accessToken`)
if (local_token) {
return local_token
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code snippet has a small issue that needs to be addressed:

const useUserStore = defineStore({
  state: () => ({
    userType: null,
    userAccessToken: '',
  }),
  actions: {
    getTokenFromStorage(type) {
      let token;
      if (type === 'session') {
        token = sessionStorage.getItem(`${this.userAccessToken}-accessToken`);
      } else { // Assuming 'local' means using localStorage
        token = localStorage.getItem(`${this.userAccessToken}-accessToken`);
      }
      return token || '';
    },
    getAccessToken(token_type) {
      const storedToken = this.getTokenFromStorage(token_type);
      const isSessionToken = token_type === 'session';
      
      const access_token = isSessionToken ? storedToken : this.getAccessToken();
      
      return access_token;
    }
  }
});

Explanation of the Improvement:

  • Variable Naming Consistency: Changed token variable name within the function getTokenFromStorage() to avoid confusion with the existing token property in the store's state.
  • Consistent Logic: The logic inside the getTokensFromStorage() function remains consistent and uses similar pattern checks.
  • Return Value Handling: Ensured an empty string is returned if no token is found instead of undefined, which makes it safer from usage bugs.

This change improves readability and maintainability while maintaining the same functionality.

@@ -271,7 +271,7 @@ const props = withDefaults(
showUserInput?: boolean
sendMessage: (question: string, other_params_data?: any, chat?: chatType) => void
openChatId: () => Promise<string>
validate: () => Promise<boolean | string>
validate: () => Promise<any>
}>(),
{
applicationDetails: () => ({}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code snippet has one primary issue:

  1. Type Inconsistency in validate Function: The original code specifies that validate returns Promise<boolean | string>, which means it can either resolve to a Boolean value or a String indicating an error.

  2. Updated Type for validate:

    validate: () => Promise<any>,

    This change allows validate to return any type of object or data, which is not appropriate according to the previous definition. You should ideally specify string as the return type if you plan on returning errors, or otherwise define a common type for both success outcomes and errors.

To maintain correctness, here is how you could update the function declaration:

validate(): Promise<ResponseData>,


// Example response object structure:
interface ResponseData {
  success: boolean,
  errorMessage?: string // If used, optional
}

This update maintains consistency with the expected behavior specified in the initial withDefaults setup when calling validate().

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.

1 participant