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

task(chat): Update what should be skipped in state.json #1966

Merged
merged 6 commits into from
Apr 15, 2024

Conversation

Flemmli97
Copy link
Collaborator

What this PR does 📖

  • Do not (de)serialize chats.
    • Conversations are already fetched from warp on load. This will just make it not save those chats.
    • Chats in sidebar are simply resolved with the uuids
    • Not saving the chats should prevent problems with loading when the internal structure changes.

Which issue(s) this PR fixes 🔨

Additional comments 🎤

We should probably move sidebar chats + favorites to warp.

Copy link
Contributor

github-actions bot commented Apr 8, 2024

⚠️ The title of this PR is invalid.
Please make the title match the regex (?:add|update|task|chore|feat|fix|refactor)\([a-z-A-Z]+\):\s.+.
e.g.) add(cli): enable --verbose flag, fix(api): avoid unexpected error in handler

@github-actions github-actions bot added invalid PR title Lint title job failed for this PR Don't merge yet DO NOT MERGE Missing dev review Still needs to be reviewed by a dev labels Apr 8, 2024
@Flemmli97 Flemmli97 changed the title update what should be skipped task(chat): Update what should be skipped in state.json Apr 8, 2024
@github-actions github-actions bot removed the invalid PR title Lint title job failed for this PR label Apr 8, 2024
Copy link
Contributor

github-actions bot commented Apr 8, 2024

UI Automated Test Results Summary for MacOS/Windows

509 tests   449 ✅  2h 58m 8s ⏱️
 45 suites   60 💤
  3 files      0 ❌

Results for commit d36d151.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Apr 8, 2024

UI Automated Tests execution is complete! You can find the test results report here

@phillsatellite
Copy link
Contributor

So I deleted my state.json and reloaded the app and conversations were missing

Screen.Recording.2024-04-08.at.1.56.05.PM.mov

@phillsatellite phillsatellite added the Changes requested When other dev or QA request a change label Apr 8, 2024
@Flemmli97
Copy link
Collaborator Author

Flemmli97 commented Apr 9, 2024

yes because the sidebars are still saved in state.json. the chats are still there. though you would need to click on the friends page to chat first.
ideally in the future it should be moved to warp but for now thats not the case

@Flemmli97 Flemmli97 removed the Changes requested When other dev or QA request a change label Apr 9, 2024
@dariusc93
Copy link
Contributor

yes because the sidebars are still saved in state.json. the chats are still there. though you would need to click on the friends page to chat first. ideally in the future it should be moved to warp but for now thats not the case

Uplink should be loading Conversation into state during initializing, including anything into the sidebar and not depend on data representing conversations in state.json. This likely could be done with

.

@Flemmli97
Copy link
Collaborator Author

Flemmli97 commented Apr 9, 2024

yes because the sidebars are still saved in state.json. the chats are still there. though you would need to click on the friends page to chat first. ideally in the future it should be moved to warp but for now thats not the case

Uplink should be loading Conversation into state during initializing, including anything into the sidebar and not depend on data representing conversations in state.json. This likely could be done with

.

this is already whats being done.
state now only has data whether the chat should be displayed in the sidebar or not (and favorites). which is just a uuid reference

@dariusc93
Copy link
Contributor

this is already whats being done. state now only has data whether the chat should be displayed in the sidebar or not (and favorites). which is just a uuid reference

If thats the case, shouldnt the conversations be loaded up into the sidebar after the state.json is removed or overridden? The favorites part can be dealt with later on warp side (already have a branch for marking a conversation as favorite).

@Flemmli97
Copy link
Collaborator Author

i guess we can make it so that if the state.json is completly missing it adds all conversations to the sidebar?

@dariusc93
Copy link
Contributor

i guess we can make it so that if the state.json is completly missing it adds all conversations to the sidebar?

That, or probably compare the state to what is available on warp to see if a conversation was deleted or is missing and just remove that invalid entry.

@Flemmli97
Copy link
Collaborator Author

the thing is you can hide a chat on uplink and it will not be in the sidebar anymore. but the chat is still there

@dariusc93
Copy link
Contributor

the thing is you can hide a chat on uplink and it will not be in the sidebar anymore. but the chat is still there

I think that will be fine for now as we can treat this as a client-to-client bases for the time being

@github-actions github-actions bot added Failed Automated Test This PR is failing Luis's Appium test and needs revised and removed Failed Automated Test This PR is failing Luis's Appium test and needs revised labels Apr 10, 2024
@Flemmli97 Flemmli97 added the Changes requested When other dev or QA request a change label Apr 11, 2024
@Flemmli97
Copy link
Collaborator Author

marked as change request cause want to make it so if the state is e.g. new all chats will be loaded into the sidebar

@github-actions github-actions bot added the Failed Automated Test This PR is failing Luis's Appium test and needs revised label Apr 11, 2024
@Flemmli97 Flemmli97 removed the Changes requested When other dev or QA request a change label Apr 12, 2024
@phillsatellite
Copy link
Contributor

Sara and I retested again and still had chats missing when we would delete are state.json but something I noticed is when you login the button on the landing page will say Start a chat and then quickly change to Go to Friends so it does seem like Warp/Uplink is realizing that there are chats to load its just they never appear

Screen.Recording.2024-04-12.at.11.15.41.AM.mov

@phillsatellite phillsatellite added the Changes requested When other dev or QA request a change label Apr 12, 2024
@Flemmli97 Flemmli97 removed the Changes requested When other dev or QA request a change label Apr 12, 2024
@Flemmli97
Copy link
Collaborator Author

changed that field to be serialized. there is prob a better way to do it but that would require some design changes since state gets written to multiple times before the app is completely initialized. (e.g. during login)

@github-actions github-actions bot removed the Failed Automated Test This PR is failing Luis's Appium test and needs revised label Apr 12, 2024
@stavares843 stavares843 added the QA Tested QA has tested and approved label Apr 13, 2024
@dariusc93 dariusc93 added Ready to Merge This PR is ready to merge and removed Missing dev review Still needs to be reviewed by a dev Don't merge yet DO NOT MERGE labels Apr 15, 2024
@stavares843 stavares843 merged commit b5f4b59 into dev Apr 15, 2024
10 checks passed
@stavares843 stavares843 deleted the state_de_ser_update branch April 15, 2024 16:17
@github-actions github-actions bot removed QA Tested QA has tested and approved Ready to Merge This PR is ready to merge labels Apr 15, 2024
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.

task(chats): load the conversations on initialization and not do based on what is there in the state
4 participants