Improve sockets #10

sebthtan · 2021-06-15T04:06:40Z

Implemented JWT auth for sockets.
Implemented indicator to show when the other user is actively typing.
- In components/ActiveChat/Input.js, onChange triggers a dispatch for newly made changeTypingStatus thunk
- changeTypingStatus thunk emits 'typing-status' to socket
- 'typing-status' event dispatches displayTypingStatus thunk to store typingStatus (boolean) in conversation state
- typingStatus-dependent conditional rendering of newly made TypingIndicator component

Mayo19

Great job, I really liked your implementation. I just left a few comments regarding naming, modifying redux state and a couple other small things.

Edit: One additional thing we would've liked to see is improving onlineUsers to be accessed more efficiently.

Let us know on slack when you're ready for a re-review.

Mayo19 · 2021-06-15T15:22:56Z

client/src/Signup.js

@@ -72,7 +70,7 @@ const Login = (props) => {
                className={classes.input}
              />
            </FormControl>
-            <FormControl margin='normal' className={classes.full} error={!!formErrorMessage.confirmPassword}>


Are the changes in this file related to this PR?

Line 75 was extraneous code from when there was still a confirm-password field on signup. I realized (after completing the login and signup redesign) that I had left that line there; even though it's a single line, is it generally best practice to just make that change in the login/signup-redesign branch?

Generally, best practice would be to open a new PR to do cleanup like this. If you want to include it in an unrelated PR like you've done here, it's helpful to reviewers to mention it in the PR description. I wouldn't remove it from this PR at this point.

Mayo19 · 2021-06-15T15:25:45Z

client/src/components/ActiveChat/Input.js

+    this.typing = false
+    this.timeout = ''


These should probably be react state variables, like text.

Refactored so that these are now state variables.

Mayo19 · 2021-06-15T15:52:59Z

client/src/store/utils/reducerFunctions.js

+  conversations.forEach(convo => convo.typingStatus = false)
+  return conversations


Even though conversations is just data from the api in this case, we should still follow convention and return a new object instead of mutating it.

Suggested change

conversations.forEach(convo => convo.typingStatus = false)

return conversations

return conversations.map(convo => {

const newConvo = {...convo, typingStatus: false};

return newConvo;

}

Pushed a fix for this.

Mayo19 · 2021-06-15T16:20:03Z

server/bin/www

+  socket.on('typing-status', (data) => {
+    socket.to(data.recipientId).emit('typing-status', {
+      sender: data.sender,
+      boolean: data.boolean


A more descriptive name like isTyping would be preferred over boolean

Globally renamed boolean to isTyping.

Mayo19 · 2021-06-15T16:41:02Z

client/src/store/utils/reducerFunctions.js

+  const newState = [...state]
+  const targetConvo = newState.find(convo => {
+    return convo.otherUser.id === sender.id
+  })
+  targetConvo.typingStatus = boolean
+
+  return newState


I believe since [...state] only creates a shallow clone you're actually mutating the original reference to targetConvo. One way of avoiding this is doing something similar to the other reducer functions in the file where we map over the state, create a copy of the object and modify the copy.

Pushed a fix for this.

Mayo19 · 2021-06-15T17:09:27Z

One additional thing we would've liked to see was improving onlineUsers to be more efficient.

…ut in Input.js to state

etchao

Overall looks like you've addressed the main issues and added a typing status as well, which is great! There's a few more things that could be improved.

After you've addressed everything, you can merge this PR without waiting for another approval 🙂

etchao · 2021-06-17T15:25:39Z

client/src/Login.js

@@ -70,7 +70,7 @@ const Login = (props) => {
                className={classes.input}
                InputProps={{
                  endAdornment: (
-                    <InputAdornment component={Link} position="start">
+                    <InputAdornment component={'div'} position="start">


I'd suggest using Box here instead of 'div' - since this project uses Material UI, it'd be better to stick to using Material UI components instead of mixing regular jsx in to keep things consistent.

etchao · 2021-06-17T15:28:35Z

client/src/Signup.js

@@ -72,7 +70,7 @@ const Login = (props) => {
                className={classes.input}
              />
            </FormControl>
-            <FormControl margin='normal' className={classes.full} error={!!formErrorMessage.confirmPassword}>


Generally, best practice would be to open a new PR to do cleanup like this. If you want to include it in an unrelated PR like you've done here, it's helpful to reviewers to mention it in the PR description. I wouldn't remove it from this PR at this point.

client/src/components/ActiveChat/Messages.js

etchao · 2021-06-17T15:43:27Z

server/bin/www

@@ -29,31 +30,63 @@ const server = http.createServer(app);
 * Listen on provided port, on all network interfaces, and sync database.
 */

-const io = require("socket.io")(server);
+// TO-DO


Looks like this comment can be removed? Or it needs to be updated with more detail

server/bin/www

server/onlineUsers.js

…use MUI

sebthtan added 7 commits June 12, 2021 18:08

fixed html nesting error

8d72ada

removed unnecessary imports and added linting

69dcfba

fixed offline/online display

2d65ec6

typing indicator

4e204bd

fixed css selector style

b23eaa5

removed consolelogs

ac3b1f0

added auto-scroll

b1696a9

Mayo19 reviewed Jun 15, 2021

View reviewed changes

Mayo19 mentioned this pull request Jun 15, 2021

6) Optional goody: Small improvement of your choice #6

Closed

Mayo19 linked an issue Jun 15, 2021 that may be closed by this pull request

6) Optional goody: Small improvement of your choice #6

Closed

sebthtan added 6 commits June 15, 2021 17:35

changed boolean to isTyping for typing status, moved typing and timeo…

a749c45

…ut in Input.js to state

setConvosToStore no longer mutates state

179f6d7

typing indicator properly disappears on message send

68e3368

fixed shallow copy in setConvosToStore

bbb1918

fixed shallow copy in setConvosToStore

ca86282

fixed shallow copy in changeConvoTypingStatus

8af42ae

sebthtan requested a review from Mayo19 June 16, 2021 05:43

sebthtan added 3 commits June 16, 2021 00:49

enable cors

e33a0f7

enable cors

eb72b21

onlineUsers changed to obj

1b9289a

etchao approved these changes Jun 17, 2021

View reviewed changes

refactored logout for onlineUsers object, changed input adornment to …

70343e0

…use MUI

sebthtan merged commit 3e10d82 into main Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve sockets #10

Improve sockets #10

sebthtan commented Jun 15, 2021

Mayo19 left a comment •

edited

Loading

Mayo19 Jun 15, 2021

sebthtan Jun 15, 2021

etchao Jun 17, 2021

Mayo19 Jun 15, 2021

sebthtan Jun 15, 2021

Mayo19 Jun 15, 2021

sebthtan Jun 16, 2021

Mayo19 Jun 15, 2021

sebthtan Jun 15, 2021 •

edited

Loading

Mayo19 Jun 15, 2021

sebthtan Jun 16, 2021

Mayo19 commented Jun 15, 2021

etchao left a comment •

edited

Loading

etchao Jun 17, 2021

etchao Jun 17, 2021

etchao Jun 17, 2021

		conversations.forEach(convo => convo.typingStatus = false)
		return conversations

-  conversations.forEach(convo => convo.typingStatus = false)
-  return conversations
+  return conversations.map(convo => {
+    const newConvo = {...convo, typingStatus: false};
+    return newConvo;
+  }

Improve sockets #10

Improve sockets #10

Conversation

sebthtan commented Jun 15, 2021

Mayo19 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebthtan Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mayo19 commented Jun 15, 2021

etchao left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mayo19 left a comment •

edited

Loading

sebthtan Jun 15, 2021 •

edited

Loading

etchao left a comment •

edited

Loading