London | 26-SDC-MAR | Craig D'Silva | Sprint 2 | Chat App#87
London | 26-SDC-MAR | Craig D'Silva | Sprint 2 | Chat App#87craig-dsilva wants to merge 38 commits into
Conversation
Send message
Message Time
Healthcheck
Likes and Dislikes
craig-dsilva
added
🐋 Size X-Large
|
The spec has this requirement:
The Changelist section of the PR description would be the best place to include this info. Also, why not include the frontend and backend links in the PR description? Make it easier to find everything in one place. |
cjyuan
added
the
Review in progress
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
-
The code is well-structured and highly modular. Nice work. 👍
-
The deployed frontend appears to be unavailable and returns a "Bad Gateway" error, so I wasn't able to test the application.
-
Input validation and error handling could be strengthened. The current implementation has limited safeguards against invalid input and unexpected failures.
-
Code could use some documentation (comments).
A useful rule of thumb is to ask yourself whether you could still explain or figure out how the code works few months from now. If the answer is uncertain, consider adding a comment to help future readers understand the intent and reasoning behind it.
| const dt = new Date(messageTime); | ||
| const date = new Intl.DateTimeFormat("en-GB").format(new Date(dt)); | ||
| const time = new Date(messageTime).toTimeString().split(" ")[0]; | ||
| return { date, time }; |
There was a problem hiding this comment.
What if the user is from a country in a different time zone?
| key={i} | ||
| i={i} | ||
| index={message.index} |
There was a problem hiding this comment.
It seems some of these props are not in used.
Could you think of a better prop name than i?
| }, [url]); | ||
|
|
||
| const sendMessage = async (text) => { | ||
| const msg = { text, time: new Date().toISOString(), likes: 0, dislikes: 0 }; |
There was a problem hiding this comment.
Could you investigate why using client-side timestamps in a chat app may lead to issues?
| const cursorRef = useRef(0); | ||
| const activeRef = useRef(true); |
There was a problem hiding this comment.
In the long polling implementation, how does a user get the latest messages from the server during an active conversation?
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
This is the coursework for the Chat App.