refactor(chat): remove inputText in ChatState for reduce rebuild#3441
refactor(chat): remove inputText in ChatState for reduce rebuild#3441lsaudon wants to merge 1 commit into
Conversation
|
Some stuff was recently committed for this. I would compare it to that if you get a chance. Yours might be better but I haven't looked. https://github.com/lichess-org/mobile/commits/main/ |
|
Indeed, this commit c09e3db already solves the problem. |
|
Got it! My brain can only do so much and it's maxed out for a few days. :) |
There was a problem hiding this comment.
Pull request overview
This PR fixes a chat UX regression where typing into the chat input caused provider recomputation that replaced the UI (closing the keyboard). It does this by removing draft input text from the provider-backed ChatState and relying solely on the TextEditingController as the single source of truth for the input.
Changes:
- Remove
inputTextfromChatStateand deleteChatMixin.setInputText. - Stop syncing the chat input field to provider state (removes
initStatelistener + draft restoration). - Replace
authUser != nullchecks withisLoggedInProviderin the chat bottom bar.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/view/chat/chat_screen_test.dart | Updates the fake notifier to align with removal of setInputText. |
| test/model/chat/chat_mixin_test.dart | Removes tests for setInputText, since the API is removed. |
| lib/src/view/chat/chat_screen.dart | Stops provider↔controller draft syncing; uses isLoggedInProvider for UI gating. |
| lib/src/model/chat/chat_mixin.dart | Removes provider-level inputText state and the setInputText mutator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b7098ce to
a2b061e
Compare
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| final draft = ref.read(chatProvider(widget.options)).asData?.value?.inputText ?? ''; |
There was a problem hiding this comment.
Not sure but removing this may introduce a regression: there was an issue where the chat input (draft text not yet sent) was lost when temporarily putting the app into background.
Also since the draft text was saved in the mixin state, if you closed the chat window during a game with a text already entered it would save it for if you open the chat window again. Ideally we'd want to keep that.
Ideally this fix should take that into account. So the inputText should be renamed to draftText in the state and it should be saved only when the app is leaving foreground or the chat window is closed.
We would also need tests to assert this is working properly.
Thank you!
There was a problem hiding this comment.
Actually, I hadn't thought of that. My pull request is therefore completely useless.
Do you want me to create a testWidget to check if the text is preserved when the user leaves and returns to the page?
There was a problem hiding this comment.
I would not say it is useless because we don't really want to rebuild the screen on each key stroke. It could be slow and lead to a poor user experience.
If you can modify the PR to keep both it would be nice. So yes, I'd start with a widget test that checks this spec is respected (also when app goes to background).
a2b061e to
004924c
Compare
On v0.25.3, every time I type a letter, the keyboard disappears.
This prevents the watch from being triggered in the parent.
There is no processing of
inputTextin the provider.We can remove it to avoid duplicating the truth between the TextController and the Provider.
I also replace
authUser == nullbyisLoggedInfromisLoggedInProvider