Skip to content

Chat App#134

Closed
arberzylyftari wants to merge 4 commits into
mate-academy:masterfrom
arberzylyftari:develop
Closed

Chat App#134
arberzylyftari wants to merge 4 commits into
mate-academy:masterfrom
arberzylyftari:develop

Conversation

@arberzylyftari

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I’m requesting changes because there are a few critical blockers that affect core requirements and basic functionality.

  • Critical blockers to fix:

    1. src/test.js posts to a non-existent endpoint (/messages) and will 404. Update it to /rooms/:name/messages (e.g., /rooms/general/messages) and wrap the request in try/catch so the test actually validates message creation.
    2. client/src/App.tsx sends messages without a time field (requirement 1.4/2.4). Either add time: new Date().toISOString() on send or ensure the server always assigns it before broadcasting; otherwise the message shape is violated when the server doesn’t add it.
    3. src/index.js can throw on WebSocket join if a client joins a non-existent room (accessing rooms[ws.room].messages). Add a room-existence check or default/fallback before reading .messages.
  • Important robustness fixes (please address next):

    • Wrap JSON.parse of incoming WS messages in try/catch and validate type to prevent handler crashes.
    • Handle room rename/delete for connected clients (update ws.room or notify/reassign) to avoid inconsistent state after admin actions.

What’s good: you implemented the end-to-end client/server, room management, username persistence, and history-on-join flows. Addressing the above will make the app meet the requirements reliably. Keep going—these are focused fixes and should be quick to implement.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread client/.gitignore
node_modules
dist
dist-ssr
*.local

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider explicitly ignoring environment files to prevent accidental commits of secrets/configuration. Add patterns like .env and .env.* (or .env.local) in addition to or instead of *.local to make intent clear and robust across different tools and platforms.

Comment thread client/.gitignore
*.local

# Editor directories and files
.vscode/*

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .vscode/* entry with an allow-list for !.vscode/extensions.json is a common pattern to share recommended extensions while ignoring personal editor settings. Ensure that extensions.json does not contain any workspace secrets or credentials before committing.

Comment thread client/README.md
@@ -0,0 +1,69 @@
# React + TypeScript + Vite

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is a project README and does not contain any implementation. The task requires an implemented client and server for the chat app (username handling, localStorage, room management, persisted messages). Please provide the source code files (client/src/* and server/*) so those requirements can be validated.

Comment thread client/README.md
@@ -0,0 +1,69 @@
# React + TypeScript + Vite

This template provides a minimal setup to get React working in Vite with HMR and some ESLint rules.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines here are documentation about React + Vite and ESLint configuration. They do not implement or document the required server API, socket events, message schema (author, time, text), or room management operations (create/rename/join/delete). Add documentation that explicitly describes the API/contract and persistence strategy for messages per room.

Comment thread client/README.md
Comment on lines +14 to +40
```js
export default tseslint.config([
globalIgnores(['dist']),
{
files: ['**/*.{ts,tsx}'],
extends: [
// Other configs...

// Remove tseslint.configs.recommended and replace with this
...tseslint.configs.recommendedTypeChecked,
// Alternatively, use this for stricter rules
...tseslint.configs.strictTypeChecked,
// Optionally, add this for stylistic rules
...tseslint.configs.stylisticTypeChecked,

// Other configs...
],
languageOptions: {
parserOptions: {
project: ['./tsconfig.node.json', './tsconfig.app.json'],
tsconfigRootDir: import.meta.dirname,
},
// other options...
},
},
])
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block shows example ESLint config (lines 14-39). While useful, it does not replace the need for the required implementation files. If you intended to submit the code, please attach the implementation files instead of (or in addition to) this README.

Comment thread src/index.js Outdated
// Broadcast Helper
function broadcast(roomName, data) {
wss.clients.forEach((client) => {
if (client.readyState === 1 && client.room === roomName) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The broadcast helper checks client.readyState === 1. For readability and to avoid magic numbers, consider comparing with the WebSocket OPEN constant (e.g. WebSocket.OPEN) after importing or referencing it from the ws package.

Comment thread src/index.js Outdated

const server = app.listen(PORT, () => {
// eslint-disable-next-line no-console
console.log(`Server is runnig at http://localhost:${PORT}/`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: small typo in the startup message: Server is runnig -> Server is running. Cosmetic, but worth fixing.

Comment thread src/test.js
const axios = require('axios');

describe('Chat API', () => {
it('should save a message in the general room', async () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This POST targets http://localhost:3000/messages, but the server exposes message creation at /rooms/:name/messages (for example /rooms/general/messages). With the current URL the request will likely 404 and no message will be created. Change the URL to include the room (e.g. http://localhost:3000/rooms/general/messages) or build the URL from a room variable so you can test different rooms. See the server message handler which creates messages under /rooms/:name/messages and assigns a time field there.

Comment thread src/test.js Outdated
Comment on lines +4 to +7
it('should save a message in the general room', async () => {
const response = await axios.post(
'http://localhost:3000/rooms/general/messages',
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap the network call in a try/catch to handle request failures and avoid unhandled promise rejections. For example, use try { const response = await axios.post(...) ; console.log(response.data); } catch (err) { console.error('Request failed', err); }. This will make the script more useful for testing and debugging when the server is down or returns an error.

Comment thread src/test.js Outdated
@@ -0,0 +1,18 @@
const axios = require('axios');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script uses require('axios'). Make sure axios is listed in your project's dependencies (package.json) and installed before running the script, otherwise Node will throw a module-not-found error.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I can’t approve yet because there are a few critical blockers that can break functionality and violate explicit checklist requirements.

Must-fix issues:

  • Server WS parsing can crash: In src/index.js, JSON.parse(message) (WS handler) lacks try/catch. Malformed frames will throw and break the handler (req. 3.2). Wrap in try/catch and validate data.type before use.
  • Join to non-existent room can throw: In src/index.js, the join flow accesses rooms[ws.room].messages without confirming the room exists (req. 2.8). Check existence and either reject with an error event or fallback (e.g., to general) before sending history.
  • Missing message validation: In both POST /rooms/:name/messages and WS message handling (src/index.js), author/text are not validated (req. 3.2). Ensure they are strings and text.trim().length > 0; return 400 for HTTP and send an error/ignore for WS.
  • Timestamp format not guaranteed machine-parseable: Message time is set with new Date() (src/index.js). Use new Date().toISOString() for both HTTP and WS messages (req. 3.8).
  • Room rename/delete don’t update/notify connected clients: After PUT /rooms/:name and DELETE /rooms/:name, connected clients retain stale ws.room and are not notified (reqs. 2.9, 2.10, 3.3). Update client.room for affected sockets and/or send WS notifications (e.g., room-renamed, room-deleted) and handle re-init as needed.

Important client fixes (not all blockers, but address robustness and req. 3.1): send username in the WS join payload (client/src/App.tsx) so the server can associate it with the session; guard JSON.parse(event.data) with try/catch; ensure socket.readyState === WebSocket.OPEN before sending; prevent empty messages.

You’ve implemented most core flows well (rooms CRUD, per-room history, WS broadcast, client UI). Address the above items and this will meet the checklist reliably. Keep going—you’re close.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread client/README.md
},
},
])
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This README documents ESLint configuration and plugins — useful. Consider adding a short "How to run" / "Development" section (steps to install deps and start the client, and the expected backend URL/port) so reviewers can quickly launch the client and test the chat app. This will make the project easier to evaluate but does not affect functional requirements.

Comment thread client/eslint.config.js
import globals from 'globals'
import reactHooks from 'eslint-plugin-react-hooks'
import reactRefresh from 'eslint-plugin-react-refresh'
import tseslint from 'typescript-eslint'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import on this line uses the module name 'typescript-eslint'. The widely used TypeScript ESLint packages are under the @typescript-eslint scope (for example, '@typescript-eslint/eslint-plugin' and '@typescript-eslint/parser'). Verify that 'typescript-eslint' is an installed package and that it exports what you expect. If not, replace it with the correct package(s) or update how you consume them.

Comment thread client/eslint.config.js
import tseslint from 'typescript-eslint'
import { globalIgnores } from 'eslint/config'

export default tseslint.config([

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file calls tseslint.config([...]) to build the exported configuration. That assumes the imported tseslint module exposes a config function. Confirm the package you import here actually provides this API. If it does not, adjust the export to the shape ESLint expects (for example, exporting a plain object or using the proper helper provided by the actual package).

Comment thread client/eslint.config.js
js.configs.recommended,
tseslint.configs.recommended,
reactHooks.configs['recommended-latest'],
reactRefresh.configs.vite,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reactRefresh.configs.vite is referenced in the extends array. Verify that the eslint-plugin-react-refresh package is installed and that it exposes a configs.vite export. If that plugin doesn't provide such a named config, ESLint will fail to load. If you only need the plugin, consider adding it to plugins and referencing its rules or use a different plugin/config that offers Vite-specific settings.

Comment thread client/eslint.config.js
import { globalIgnores } from 'eslint/config'

export default tseslint.config([
globalIgnores(['dist']),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

globalIgnores(['dist']) is fine and useful, but ensure the overall exported shape (returned by tseslint.config(...)) matches the expected ESLint config structure. If you change the tseslint import to a different package, re-check that globalIgnores is still applied as intended in the final exported object.

Comment thread src/index.js Outdated
Comment on lines +65 to +66

delete rooms[roomName];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When deleting a room you delete rooms[roomName] but you don't handle current WebSocket members in that room. You must notify or move those clients (for example, set their room to 'general' and send an 'room-deleted' notification). Also ensure the deleted room's message history is made inaccessible (requirement 2.10 / 3.3).

Comment thread src/index.js
Comment on lines +93 to +101

if (app.wss) {
app.wss.clients.forEach((client) => {
if (client.readyState === 1 && client.room === req.params.name) {
client.send(JSON.stringify({ type: 'message', message }));
}
});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor defensive suggestion: there's a small race window in the HTTP POST handler where the room could be deleted between the initial if (!room) check and the broadcast. Before broadcasting, re-check room existence or guard against undefined when matching client.room. This avoids exceptions during concurrent operations.

Comment thread src/index.js
});
}

module.exports = app;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good: exporting app at the end allows tests to require the server (line 145). Keep this to support testing (already used in the provided tests).

Comment thread src/test.js
expect(res.status).toBe(201);
expect(res.body).toHaveProperty('author', 'Arber');
expect(res.body).toHaveProperty('text', 'Hello from test.js');
expect(res.body).toHaveProperty('time');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test currently asserts the time property exists (expect(res.body).toHaveProperty('time')). To enforce the requirement that timestamps be machine-parseable (checklist 3.8), consider checking that time is a non-empty string or an ISO timestamp. Example assertion you can add:

expect(typeof res.body.time).toBe('string');
expect(new Date(res.body.time).toString()).not.toBe('Invalid Date');

This ensures the server returns a usable timestamp rather than an arbitrary object or null.

Comment thread src/test.js
Comment on lines +6 to +8
const res = await request(app)
.post('/rooms/general/messages')
.send({ author: 'Arber', text: 'Hello from test.js' });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To verify message persistence (new users should see previous messages in the room — checklist 1.6 / 2.3), extend the test to fetch the room history after posting and assert the posted message is present. For example, after the POST you can await request(app).get('/rooms/general/messages') and assert the returned array includes an entry with author: 'Arber' and text: 'Hello from test.js'. This makes the test verify both creation and storage, not just the POST response.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Educational approval: nice progress building a full client/server chat with room CRUD, per-room history, ISO timestamps, and message validation. The server covers most endpoints and the client renders messages and room actions clearly. To strengthen your solution, focus on a few critical fixes: (1) client username flow — add a visible username state/input, persist it, send it to the server, and block sends without a username (reqs. 1.2, 3.14, 3.15); (2) add robust fetch error handling across initial rooms/messages, create/rename/delete, and send operations using try/catch and res.ok checks (reqs. 3.4, 3.7, 3.9); (3) improve WS/server robustness by guarding app.wss in PUT/DELETE room handlers and validating ws.room exists before pushing messages, ideally initializing ws.room = 'general' on connection (reqs. 3.4, 3.9). Minor improvements: persist and restore the selected room (req. 3.16), use stable keys for messages, validate/format message time safely, and tighten CSS (box-sizing for pseudo-elements, disabled button hover). Also clean up client/.gitignore single-space lines and consider ignoring .env*, and convert ESLint config to a valid flat config with correct @typescript-eslint imports. You’re very close; addressing these points will make your app reliable and fully aligned with the checklist. (See notes above with line-specific pointers for App.tsx and src/index.js.)


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread client/.gitignore
yarn-error.log*
pnpm-debug.log*
lerna-debug.log*

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line appears to contain only a single space character. That creates a gitignore pattern matching a space character filename (probably unintended). Remove this line to avoid unexpected behavior.

Comment thread client/.gitignore
dist
dist-ssr
*.local

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above — this line appears to contain only a single space character. Remove it to avoid introducing an accidental ignore pattern.

Comment thread client/.gitignore
node_modules
dist
dist-ssr
*.local

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider explicitly ignoring environment files to avoid committing secrets/configuration. Right now you have *.local, but adding explicit entries makes intent clear. For example, add:

  • .env
  • .env.local
  • .env.*
    You can place these entries here (near other local-related ignores).

Comment thread client/README.md
@@ -0,0 +1,69 @@
# React + TypeScript + Vite

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The title and opening line read like a generic Vite + React template. If this README is intended to document the chat client for this project, update the title/intro to describe the chat app and its purpose, not the template.

Comment thread client/README.md
@@ -0,0 +1,69 @@
# React + TypeScript + Vite

This template provides a minimal setup to get React working in Vite with HMR and some ESLint rules.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph describes the template. Replace or augment it with a short project summary that lists main features required by the assignment (username persistence in localStorage, rooms CRUD, message history, server communication via HTTP/WS) so reviewers know what to expect.

Comment thread src/test.js
@@ -0,0 +1,15 @@
const request = require('supertest');
const app = require('./index');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good: you require the app and use supertest to POST to the endpoint. Consider adding test setup/teardown to ensure in-memory server state is reset between tests so runs are deterministic. If the server doesn't expose a reset helper, add one (e.g., app.__resetForTests()), or ensure tests run against a fresh process. This avoids leaking messages/rooms across tests (req. 3.13).

Comment thread src/test.js
Comment on lines +6 to +8
const res = await request(app)
.post('/rooms/general/messages')
.send({ author: 'Arber', text: 'Hello from test.js' });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The POST request (lines 6–8) is fine. After checking the POST response, strengthen the test by performing a GET /rooms/general/messages and assert that the returned messages array includes the message you just sent. This verifies that the server persisted the message in the room history (req. 1.6 / 3.7).

Comment thread src/test.js
expect(res.status).toBe(201);
expect(res.body).toHaveProperty('author', 'Arber');
expect(res.body).toHaveProperty('text', 'Hello from test.js');
expect(res.body).toHaveProperty('time');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You assert the presence of the time field (line 13) but do not validate its format. The server sets time via new Date().toISOString() (see server implementation) so the test should assert it's a valid ISO timestamp. For example, assert new Date(res.body.time).toISOString() === res.body.time or that Date.parse(res.body.time) is not NaN (req. 3.10).

Comment thread src/test.js
Comment on lines +10 to +12
expect(res.status).toBe(201);
expect(res.body).toHaveProperty('author', 'Arber');
expect(res.body).toHaveProperty('text', 'Hello from test.js');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider asserting the entire response shape more explicitly, e.g. expect(res.status).toBe(201); expect(res.body).toEqual({ author: 'Arber', text: 'Hello from test.js', time: expect.any(String) }); then validate the time format. That makes intent clearer and helps catch regressions.

Comment thread src/test.js
it('should save a message in the general room', async () => {
const res = await request(app)
.post('/rooms/general/messages')
.send({ author: 'Arber', text: 'Hello from test.js' });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you plan to extend the test suite, avoid using hard-coded usernames that might collide with other tests. Use a unique author (e.g., include a timestamp or UUID) per test run so assertions that check message arrays are unambiguous.

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.

2 participants