Priscilla Andresen - Possum by priandresen · Pull Request #34 · Ada-C24/react-chatlog

priandresen · 2025-12-16T20:55:23Z

No description provided.

Copilot

Pull request overview

This PR implements a chat application with interactive like functionality. The changes include creating new components for displaying chat messages and adding the ability to toggle likes on messages with a live count display.

Key Changes:

Added ChatLog component to render lists of chat entries
Implemented like/unlike functionality with state management
Added a header widget to display the total count of likes

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/components/ChatLog.jsx	New component that maps over message entries and renders ChatEntry components
src/components/ChatEntry.jsx	Implements message display with sender, body, timestamp, and interactive like button
src/App.jsx	Adds state management for messages, like toggle functionality, and helper functions for counting likes and extracting sender names
src/App.css	Styling updates for the widget section padding and h2 color
src/components/ChatEntry.css	Minor formatting change (trailing whitespace)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-16T20:58:25Z

+};
+
+ChatLog.propTypes = {
+  chatEntries: PropTypes.arrayOf(


The PropTypes definition uses chatEntries as the prop name, but the component actually receives and uses entries (as seen in line 7: props.entries.map). This mismatch means the PropTypes validation won't work correctly.

Suggested change

chatEntries: PropTypes.arrayOf(

entries: PropTypes.arrayOf(

👀 The reason that you didn't see a warning about this is that this prop is not marked required. The individual fields of the inner shape are required, but the entries (chatEntries) prop itself is not, even though it should be. We would mark that as shown a little farther down.

Copilot · 2025-12-16T20:58:25Z

+
+
+const countTotalLikes = (messageData) => {
+  return messageData.reduce((acc, entry) => acc + entry.liked, 0);


The countTotalLikes function treats liked as a number and sums it up, but according to the PropTypes definitions in both ChatEntry and ChatLog, liked is a boolean. This will result in incorrect counting - instead of counting the number of likes, it will sum boolean values (true = 1, false = 0), which happens to work but is semantically incorrect and confusing. The function should check if entry.liked is true and count those instances.

Suggested change

return messageData.reduce((acc, entry) => acc + entry.liked, 0);

return messageData.reduce((acc, entry) => acc + (entry.liked === true ? 1 : 0), 0);

Copilot · 2025-12-16T20:58:26Z

+  padding: 0.2em;
 }

 #App .widget {


The selector uses .widget class but the actual element has id="widget". CSS class selectors start with . but this should be an id selector #widget or the HTML should use className="widget" instead of id="widget".

Suggested change

#App .widget {

#App #widget {

Copilot · 2025-12-16T20:58:26Z

-}
+}
+
+h2 {


The h2 rule is too broad and will affect all h2 elements in the application. Since this appears to be styling for the widget section specifically, consider using a more specific selector like #widget h2 to avoid unintended side effects on other h2 elements.

Suggested change

h2 {

#App .widget h2 {

anselrognlie

Looks good overall, but please review my comments, and let me know if you have any questions.

anselrognlie · 2026-01-12T23:56:37Z

+}
+
+
+
+
+


Try to avoid making stray changes to other files. When reviewing the contents of a commit, if we see a change like this, it would be preferable to revert it rather than committing it.

anselrognlie · 2026-01-12T23:57:00Z

+  id: PropTypes.number.isRequired,
+  sender: PropTypes.string.isRequired,
+  body: PropTypes.string.isRequired,
+  timeStamp: PropTypes.string.isRequired,
+  liked: PropTypes.bool.isRequired,
+  onLike: PropTypes.func.isRequired,


The id, sender, body, timeStamp, and liked props are always passed (they're defined explicitly in the data and also provided in the test) so we can (and should) mark them isRequired.

The remaining props were up to you, and the tests don't know about them. As a result, using isRequired causes a warning when running any tests that only pass the known props. If you didn't see those warnings when running the tests, be sure to also try running the terminal npm test since the warnings are more visible.

To properly mark any other props isRequired, we would also need to update the tests to include at least dummy values (such as an empty callback () => {} for the like handler) to make the proptype checking happy.

Alternatively, for any props that we leave not required, we should also have logic in our component to not try to use the value if it's undefined.

anselrognlie · 2026-01-12T23:58:04Z

-    // Replace the outer tag name with a semantic element that fits our use case
-    <replace-with-relevant-semantic-element className="chat-entry local">
-      <h2 className="entry-name">Replace with name of sender</h2>
+    <div className="chat-entry local">


👀 Note that div is not a semantic element. There are two potential semantic tags that would be more appropriate (each which usually recommends including a heading tag h of some level). Prefer to use one of those.

anselrognlie · 2026-01-12T23:58:29Z

-        <p className="entry-time">Replace with TimeStamp component</p>
-        <button className="like">🤍</button>
+        <p>{props.body}</p>
+        <p className="entry-time"><TimeStamp time={props.timeStamp} /></p>


Nice use of the supplied TimeStamp. We pass in the timeStamp string from the message data and it takes care of the rest. All we had to do was confirm the name and type of the prop it was expecting (which we could do through its PropTypes) and we're all set!

anselrognlie · 2026-01-12T23:59:11Z

+import PropTypes from 'prop-types';
+import TimeStamp from './TimeStamp';
+
+const ChatEntry = (props) => {


Personally, I prefer to destructure the props since that helps give a clear picture of the props that are actually used within the component in one place which is actually tied to the implementation (as opposed to the PropTypes, which is more loosely connected, and deprecated in more recent React versions). When accessing values on the props instance directly, we need to look throughout the body of the component to see what fields on props are actually used.

anselrognlie · 2026-01-14T01:58:48Z


 const App = () => {
+
+  const [messageData, setMessages] = useState(messagesJSON);


Prefer to have the value and setter functions match, so here, either

const [messageData, setMessageData] = useState(messagesJSON);

or

const [messages, setMessages] = useState(messagesJSON);

anselrognlie · 2026-01-14T01:59:20Z

+        return entry;
+      }
+    });
+    setMessages(entries);


In this case, calculating the next version of the message data using the current state variable and passing that updated version to the state setter shouldn't cause any problems, but we still generally prefer using the callback style of setters. Using that approach, we pass a function to the setter whose parameter will receive the latest state value, and which returns the new value to use for the state.

setMessages(messages => messages.map(chat => { if (message.id === id) { return {...message, liked: !message.liked}; // or call a function that returns a toggled message } else { return message; }; }));

anselrognlie · 2026-01-14T01:59:40Z

+  const likeMessage = (messageId) => {
+    const entries = messageData.map((entry) => {
+      if (entry.id === messageId) {
+        return { ...entry, liked: !entry.liked };


We showed this approach in class, but technically, we're mixing a few responsibilities here. Rather than this function needing to know how to change the liked status itself, we could move this update logic to a helper function. This would better mirror how we eventually update records when there's an API call involved.

In this project, our messages are very simple objects, but if we had more involved operations, it could be worthwhile to create an actual class with methods to work with them, or at least have a set of dedicated helper functions to centralize any such mutation logic.

anselrognlie · 2026-01-14T19:50:07Z

+  const findSenderNames = (messageData) => {
+    const senderNames = messageData.map((entry) => {
+      return entry.sender;
+    });
+    return new Set(senderNames);
+  };


👍 Great way to get a list of the users in the messages. Sets preserve insertion order in JS, so we know that the first encountered sender would appear first in the list. Extracting the senders this way also scales to however many senders could be in this conversation.

anselrognlie · 2026-01-14T19:51:56Z

    <div id="App">
      <header>
-        <h1>Application title</h1>
+        <h1>Chat Between {Array.from(findSenderNames(messageData)).join(' and ')}</h1>


👍 Nice use of the calculated senders to build the header.

This would work for any number of senders, though the phrasing would be a little unnatural. For a challenge, see if you can come up with logic that would generate appropriate phrasings regardless of the number of participants.

anselrognlie

👍 Thanks for the updates. Looks good!

anselrognlie · 2026-01-15T22:23:12Z

  return (
-    <div className="chat-entry local">
-      <h2 className="entry-name">{props.sender}</h2>
+    <article className="chat-entry local">


👍 article is a more appropriate choice for a semantic tag, since one of its uses is to represent self-contained, reusable page elements.

anselrognlie · 2026-01-15T22:25:00Z


  const handleLike = () => {
-    props.onLike(props.id);
+    onLike(id);


Relaxing the isRequired on the onLike proptypes avoids the warning during tests, but as a result, we should also check that onLike actually has a value before calling it so that we don't end up trying to invoke undefined, which would result in an error.

anselrognlie · 2026-01-15T22:25:42Z


 ChatLog.propTypes = {
-  chatEntries: PropTypes.arrayOf(
+  entries: PropTypes.arrayOf(


👍 This matches the expected name now.

anselrognlie · 2026-01-15T22:29:03Z

+    setMessagesData((prevMessagesData) => {
+      return prevMessagesData.map((entry) => {
+        if (entry.id === messageId) {
+          return toggleLike(entry);


👍 Toggling is the only business logic we have for messages in this small app, but we can imagine if there are other operations, those could all have their own functions and potentially be grouped into a class (or at least be collected in a separate file).

Of course, when the true data representation is actually coming from a backend, we might omit such helpers entirely, and just get the updated record as a JSON response, using it directly to replace the previous entry in the list, freeing the frontend from needing to know anything about how to manage the record at all!

anselrognlie · 2026-01-15T22:37:07Z

+    return senderNames.reduce((acc, name, index) => {
+      if (index === senderNames.length - 1 && index !== 0) {
+        return `${acc} and ${name}`;
+      } else if (index === 0) {
+        return name;
+      } else {
+        return `${acc}, ${name}`;
+      }
+    }, '');


Fancy! And it handles the regular rule format of "item, item and item" (personally, I'm an Oxford comma fan, so would prefer "item, item, and item").

While not a major concern for any reasonable conversation group size, keep in mind that each of the new intermediate strings created while reduce iterates will grow longer and longer, tending towards quadratic time with the total length of the names. Instead, we could determine the part of the list that gets joined with ",", and handle those in one join call, then tack on the "and" part. There is still some extra copying, but it's a fixed number, so ternding more towards linear. Alernatively, we could build up a list of all the parts (names as well as separators), then join them with no separator in a single pass. This is the closest approach to a typical "string buffer/string builder" approach.

anselrognlie · 2026-01-15T22:39:10Z

-      } else {
-        return entry;
-      }
+    setMessagesData((prevMessagesData) => {


👍 Another benefit to using the callback style of setter is that it now no longer depends on anything in the enclosing scope (the current state value is passed in as a parameter), so this whole chunk of logic for finding and updating the message could be moved out to a helper function!

priandresen added 8 commits December 14, 2025 09:24

refactors ChatEntry to take an instance of TimeStamp

7267278

changes heart when clicked

c06d5ca

adds a section to display the count of likes

a002d43

refactors timestamp, chatlog proptypes

410a46b

fixing spacing

29fa918

refactors likeCount

b043f92

refactors likesCount

d889b20

refactors time to time stamp to match tests

8f626c7

Copilot AI review requested due to automatic review settings December 16, 2025 20:55

Copilot started reviewing on behalf of priandresen December 16, 2025 20:55 View session

Copilot AI reviewed Dec 16, 2025

View reviewed changes

anselrognlie reviewed Jan 14, 2026

View reviewed changes

refactoring after feedback

af7e4c3

anselrognlie reviewed Jan 15, 2026

View reviewed changes



		const countTotalLikes = (messageData) => {
		return messageData.reduce((acc, entry) => acc + entry.liked, 0);

-              }

                
                    No newline at end of file
+              }
+              h2 {


		const App = () => {

		const [messageData, setMessages] = useState(messagesJSON);

Conversation

priandresen commented Dec 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants