-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
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. |
[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 |
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) => { |
There was a problem hiding this comment.
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:
-
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.
if (token) { | ||
return token | ||
} | ||
const local_token = localStorage.getItem(`${token}accessToken`) | ||
const local_token = localStorage.getItem(`${token}-accessToken`) | ||
if (local_token) { | ||
return local_token | ||
} |
There was a problem hiding this comment.
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 functiongetTokenFromStorage()
to avoid confusion with the existingtoken
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: () => ({}), |
There was a problem hiding this comment.
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:
-
Type Inconsistency in
validate
Function: The original code specifies thatvalidate
returnsPromise<boolean | string>
, which means it can either resolve to a Boolean value or a String indicating an error. -
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 specifystring
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()
.
fix: Historical user conversation data has been cleared