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

Hotfix for bug introduced in pr 537 #538

Merged
merged 2 commits into from
Mar 28, 2024
Merged

Hotfix for bug introduced in pr 537 #538

merged 2 commits into from
Mar 28, 2024

Conversation

kliu57
Copy link
Collaborator

@kliu57 kliu57 commented Mar 28, 2024

Accidentally introduced a bug in this morning's merged PR #537

Took a statement out of an if statement in settings.ts, which may cause some users to be unable to load ChatCraft.

The users that are affected are users who have not logged into ChatCraft since last month.
They have a settings JSON in their localStorage, but do not have settings.currentProvider. (Because the last time they logged in was last month, back before I introduced settings.currentProvider)

Also if a user went to localStorage and manually set settings to {}, they would also be affected.

The code from PR 537 which caused the bug:

image

Fix

Restoring the if statement in this hotfix fixes the issue.

How to test:

  1. Go to localStorage and remove the currentProvider part of the settings json, or just clear the entire settings json and put {}
  2. Load ChatCraft
  3. Result - (it should have an error in production, but not in this PR's preview)

@kliu57 kliu57 self-assigned this Mar 28, 2024
@kliu57 kliu57 requested a review from humphd March 28, 2024 18:58
@mingming-ma mingming-ma self-requested a review March 28, 2024 19:02
Copy link

cloudflare-workers-and-pages bot commented Mar 28, 2024

Deploying chatcraft-org with  Cloudflare Pages  Cloudflare Pages

Latest commit: fa0f626
Status: ✅  Deploy successful!
Preview URL: https://76c85481.console-overthinker-dev.pages.dev
Branch Preview URL: https://hotfix-pr-537.console-overthinker-dev.pages.dev

View logs

@rjwignar rjwignar self-requested a review March 28, 2024 19:06
Copy link
Collaborator

@rjwignar rjwignar left a comment

Choose a reason for hiding this comment

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

I followed your testing steps for both the production website and your branch preview and this change fixes the issue.

Copy link
Collaborator

@mingming-ma mingming-ma left a comment

Choose a reason for hiding this comment

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

I tested and I can see in this pr after reload, the json will be populated not null value and load correct, which is failed in current prod. Code changes LGTM

@kliu57
Copy link
Collaborator Author

kliu57 commented Mar 28, 2024

Thanks @mingming-ma and @rjwignar for reviewing. Due to this being only a restoration of an if statement that was already there previously, and that this fixes the issue in production, I will go ahead and merge it now so users will not be affected.

@kliu57 kliu57 merged commit 9bb3672 into main Mar 28, 2024
4 checks passed
@kliu57 kliu57 deleted the hotfix-pr-537 branch March 28, 2024 19:29
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.

3 participants