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

feat: add automatical saving conversation in local storage #26

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mejsiejdev
Copy link
Member

No description provided.

@mejsiejdev mejsiejdev self-assigned this Dec 10, 2024
@mejsiejdev mejsiejdev linked an issue Dec 10, 2024 that may be closed by this pull request
@mejsiejdev mejsiejdev added the enhancement New feature or request label Dec 10, 2024
@mejsiejdev mejsiejdev marked this pull request as draft December 10, 2024 11:42
@mejsiejdev mejsiejdev marked this pull request as ready for review December 10, 2024 12:51
@mejsiejdev mejsiejdev requested a review from chewmanji December 10, 2024 12:52
Copy link
Member

@chewmanji chewmanji left a comment

Choose a reason for hiding this comment

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

May you request review from Maciej King also?

@@ -57,6 +57,7 @@ export function AutoResizeTextArea({
setIsSubmitting(true);
const newUuid = v4();
//TODO adding conversation to localstorage there?
localStorage.setItem(newUuid, JSON.stringify({ prompt: text }));
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll need list of chats in local storage {"chats" : chat[]} where each one have uuid, prompt and other data related to response from api - it will allow us to generate sidebar items easily

Copy link
Member

@maciejkrol18 maciejkrol18 Dec 12, 2024

Choose a reason for hiding this comment

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

The amount of code in the onClick callback can be reduced. Notice how this piece of code repeats two times:

localStorage.setItem(
  "chats",
  JSON.stringify([
    ...chats,
    { uuid: newUuid, prompt: text, recommendation: {} },
  ]),
);

I think it's always best to stick to the DRY principle (do not repeat yourself). Here's my take on the callback function - from 17 to 8 lines, and it works the same:

onClick={() => {
  setIsSubmitting(true);
  const newUuid = v4();  
  const chats = localStorage.getItem("chats") == null ? [] : JSON.parse(localStorage.getItem("chats")!) as Chat[]
  localStorage.setItem(
    "chats",
    JSON.stringify([
      ...chats,
      { uuid: newUuid, prompt: text, recommendation: {} },
    ]),
  );
  router.push(`/chat/${newUuid}`);
}}

I'll try my best to explain the thought process behind my changes:

const chats = localStorage.getItem("chats") == null ? [] : JSON.parse(localStorage.getItem("chats")!) as Chat[]

This line removes the need for the entire if block and utilizes a ternary operator instead. If a given local storage key doesn't exist then .getItem() method returns null, otherwise the json string, therefore we explicitly check whether .getItem("chats") is null (this also makes eslint rule @typescript/strict-boolean-expressions happy).

If true ("chats" are null)
it means no chats were saved yet, so we set "chats" variable to an empty array, therefore no type assertions are needed

If false ("chats" are not null)
it means at least one chat is saved, so we set "chats" variable to the parsed json string. Here we still use non-null assertion since typescript still thinks that .getItem("chats") can be null + we do a type assertion since JSON.parse() return type is any

localStorage.setItem(
  "chats",
  JSON.stringify([
    ...chats,
    { uuid: newUuid, prompt: text, recommendation: {} },
  ]),
);

And then we just set the "chats" local storage item to an array consisting of already existing chats and the newly created one. If "chats" variable is an empty array then we can still spread it without a problem.

This solution is compliant with our linter rules and passes all checks

EDIT: This is of course more of a nitpick and such refactor could be done later but since our project won't take that long and it isn't too complex we might as well tweak such things early on

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

Successfully merging this pull request may close these issues.

Add automatical saving conversation in local storage
3 participants