London | 26-SDC-March | Onur Atas | Sprint 2 | Chat app#102
Conversation
onurat
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Could you revert the changes made in the middleware-* folders to keep this PR clean?
| const { username, message } = req.body; | ||
|
|
||
| if (!username.trim() || !message.trim()) { |
There was a problem hiding this comment.
It's good practice to validate all request inputs.
Note: A more robust implementation
- Should also handle the case where
req.bodyisnull, which would causeconst { text, sender } = req.body;to throw. - Should not assume
textandsenderare strings (if they existed)
In this exercise, let's assume the request is always coming from the code in script.js. So no change needed.
| username: usernameInput.value, | ||
| message: messageInput.value, |
There was a problem hiding this comment.
Why not also validate the user input on the client side to reduce server's workload?
| const status = document.getElementById("status"); | ||
|
|
||
| async function loadMessages() { | ||
| const response = await fetch(`${server}/messages`); |
There was a problem hiding this comment.
How should the app reacts to a failed fetch operation?
|
|
||
| const time = new Date(message.timestamp).toLocaleTimeString(); | ||
|
|
||
| p.innerHTML = `<strong>${message.username}</strong> <small>${time}</small><br>${message.message}`; |
There was a problem hiding this comment.
Could username or message contain any special character that may mess up the display?
cjyuan
added
Reviewed
|
The changed files in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the changed files tab at the top of the page, we are only expecting changes in this directory: If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
1 similar comment
|
The changed files in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the changed files tab at the top of the page, we are only expecting changes in this directory: If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
2 participants
Self checklist
Deployed Frontend
https://wqpq4rsx4fzl0n6g8r8f0vp8.hosting.codeyourfuture.io/
Deployed Backend
https://gziswcuniy8wailsojnj6aki.hosting.codeyourfuture.io/messages
Features
Users can send messages.
Users can view all previous messages.
Messages automatically appear using polling.
Frontend and backend validation for empty messages.
Success and error feedback.
Message timestamps.
Responsive user interface.