West Midlands | 25-ITP-Sep | Ali Naru | Sprint 2 | feature/BookLibrary #334

MohammedNaru · 2025-11-28T21:21:56Z

Book Library Pull Request

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Changelist

Refactored code to include debugging and pass requirements.

Questions

No questions for reviewer.

… storage handling

github-actions · 2025-11-28T21:22:10Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Wrong number of parts separated by |s

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.

github-actions · 2025-11-28T21:23:17Z

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

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.

github-actions · 2025-11-28T21:24:40Z

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

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.

github-actions · 2025-11-28T21:24:40Z

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

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.

cjyuan

There are some console.log() statements left in your code. If they are not needed for the app to function, you should remove them to make the code clean.
Can you check if any of this general feedback can help you further improve your code?
https://github.com/cjyuan/Module-Data-Flows/blob/book-library-feedback/debugging/book-library/feedback.md

Doing so can help me speed up the review process. Thanks.

…nd input validation

MohammedNaru · 2025-12-03T14:47:03Z

There are some console.log() statements left in your code. If they are not needed for the app to function, you should remove them to make the code clean.

Can you check if any of this general feedback can help you further improve your code?
https://github.com/cjyuan/Module-Data-Flows/blob/book-library-feedback/debugging/book-library/feedback.md

Doing so can help me speed up the review process. Thanks.

I have now reviewed and refactored these changes, please review.

Copilot

Pull request overview

This PR refactors and debugs a book library application, fixing several critical bugs and improving code quality. The changes modernize the JavaScript code with better practices while maintaining functionality.

Key changes:

Fixed bugs including incorrect variable references (e.g., title.value instead of author.value), missing parentheses in loops, and typo in event listener names
Refactored the render function to use modern DOM manipulation with forEach and event delegation patterns
Added input validation with trimming and proper type conversion, plus form clearing after submission

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File	Description
debugging/book-library/script.js	Fixed multiple bugs, refactored render logic with modern JavaScript patterns, improved variable naming, and added input validation and form clearing
debugging/book-library/index.html	Added semantic HTML attributes (lang, title), corrected input types from custom to standard text/number, changed script loading to module type, and improved code formatting

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

Copilot · 2025-12-03T14:51:54Z

debugging/book-library/index.html

+        class="form-control"
+        id="pages"
+        name="pages"
+        required


The HTML input type is number, but the JavaScript parses it with parseInt(). While this works, it's redundant since number inputs already return numeric strings. The HTML5 number input also doesn't prevent negative values by default. Consider adding a min="1" attribute to the pages input in the HTML to enforce positive values at the form level.

Suggested change

required

required

min="1"

debugging/book-library/script.js

Copilot · 2025-12-03T14:51:55Z

debugging/book-library/script.js

+  if (!title || !author || isNaN(pages)) {
+    alert("Please fill all fields correctly!");


The validation doesn't check for negative page numbers or zero pages. Consider adding validation: if (!title || !author || isNaN(pages) || pages <= 0) to ensure meaningful book data is stored.

Suggested change

if (!title || !author || isNaN(pages)) {

alert("Please fill all fields correctly!");

if (!title || !author || isNaN(pages) || pages <= 0) {

alert("Please fill all fields correctly! Page number must be a positive integer.");

Copilot · 2025-12-03T14:51:55Z

debugging/book-library/script.js

-      myLibrary.splice(i, 1);
+  document.querySelectorAll(".delete-book").forEach(button => {
+    button.addEventListener("click", (e) => {
+      const index = e.target.dataset.index;


[nitpick] The dataset.index property returns a string. While JavaScript will coerce this string to a number when used with splice(), it's better to be explicit about type conversions. Consider: const index = parseInt(e.target.dataset.index, 10);

Copilot · 2025-12-03T14:51:55Z

debugging/book-library/index.html

+        <th>Author</th>
+        <th>Number of Pages</th>
+        <th>Read</th>
+        <th></th>


The last table header is empty (<th></th>), which is poor for accessibility. Screen readers won't announce what this column represents. Consider adding appropriate text like "Actions" or visually hiding it with <th><span class="sr-only">Actions</span></th> using Bootstrap's sr-only class.

Suggested change

<th></th>

<th><span class="sr-only">Actions</span></th>

Copilot · 2025-12-03T14:51:56Z

debugging/book-library/script.js

+  // Add event listeners for buttons
+  document.querySelectorAll(".toggle-read").forEach(button => {
+    button.addEventListener("click", (e) => {
+      const index = e.target.dataset.index;


[nitpick] The dataset.index property returns a string. While JavaScript will coerce this string to a number when used with array indexing, it's better to be explicit about type conversions. Consider: const index = parseInt(e.target.dataset.index, 10);

Copilot · 2025-12-03T14:51:56Z

debugging/book-library/script.js

+  document.querySelectorAll(".toggle-read").forEach(button => {
+    button.addEventListener("click", (e) => {
+      const index = e.target.dataset.index;
+      myLibrary[index].check = !myLibrary[index].check;
      render();
    });
+  });


Event listeners are being re-added every time render() is called, which can cause performance issues and unexpected behavior. Since the table is completely regenerated each time (line 64 clears innerHTML), the old event listeners are removed with the DOM elements, but this approach is inefficient.

Consider using event delegation by attaching a single event listener to the table or tbody instead:

tbody.addEventListener("click", (e) => { if (e.target.classList.contains("toggle-read")) { const index = parseInt(e.target.dataset.index, 10); myLibrary[index].check = !myLibrary[index].check; render(); } });

Copilot · 2025-12-03T14:51:56Z

debugging/book-library/script.js

+      // Display a non-blocking notification
+      const notification = document.createElement("div");
+      notification.textContent = "Book deleted successfully.";
+      notification.className = "notification";
+      document.body.appendChild(notification);
+
+      // Remove the notification after 3 seconds
+      setTimeout(() => {
+        notification.remove();
+      }, 3000);


The notification element is created and added after calling render(), which means if render() throws an error, the user won't see the notification. Additionally, multiple rapid deletions will create multiple notification elements that aren't cleaned up properly. Consider moving the notification display before render() or implementing a notification queue system to handle multiple notifications gracefully.

cjyuan

It's a good idea to get a second opinion at the code from AI.
I am sure there is a way to use it outside the scope of a PR so that the AI messages won't interfere with the review.

cjyuan · 2025-12-03T15:40:01Z

debugging/book-library/index.html

There are errors in the HTML code. Please fix them.

cjyuan · 2025-12-03T15:51:09Z

debugging/book-library/script.js

+      const notification = document.createElement("div");
+      notification.textContent = "Book deleted successfully.";
+      notification.className = "notification";
+      document.body.appendChild(notification);
+


This approach is better than using alert() for showing notification.

Please note that when the notification is added to the bottom of the page, there is a chance that the user won't see the message when there are too many books on the list. (Change is optional as this is not part of the original implementation)

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

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

debugging/book-library/index.html

Copilot · 2025-12-03T17:54:54Z

debugging/book-library/index.html

+        type="button"
+        value="Submit"
+        class="btn btn-primary"
+        onclick="submit();"


Using inline event handlers like onclick="submit();" is considered a bad practice and violates Content Security Policy (CSP) best practices. Consider moving this to an event listener in the JavaScript file: document.querySelector('input[type="button"]').addEventListener('click', submit);

Suggested change

onclick="submit();"

Copilot · 2025-12-03T17:54:55Z

debugging/book-library/script.js

  this.title = title;
  this.author = author;
  this.pages = pages;
  this.check = check;


The property name check in the Book constructor is ambiguous. It should be renamed to something more descriptive like isRead or hasBeenRead to clearly indicate what it represents. This would make the code more self-documenting.

Copilot · 2025-12-03T17:54:55Z

debugging/book-library/index.html

+  <meta
      charset="utf-8"
      name="viewport"
      content="width=device-width, initial-scale=1.0"
-    />
+  />


The meta tag has invalid HTML structure. The charset and viewport attributes cannot be combined in a single meta tag. They should be split into two separate tags:

<meta charset="utf-8" /> <meta name="viewport" content="width=device-width, initial-scale=1.0" />

Copilot · 2025-12-03T17:54:55Z

debugging/book-library/script.js

+const titleInput = document.getElementById("title");
+const authorInput = document.getElementById("author");
+const pagesInput = document.getElementById("pages");
+const readCheckbox = document.getElementById("check");


The script is loaded with type="module", which creates a module scope. However, the window.submit = submit; assignment at the end of script.js won't work reliably because the DOM elements (titleInput, authorInput, etc.) are queried at the top level before the DOM is fully loaded. These queries happen immediately when the script loads, but the elements don't exist yet. Move the DOM element queries inside the submit() function or into a DOMContentLoaded event handler.

Copilot · 2025-12-03T17:54:55Z

debugging/book-library/script.js

+  // Add event listeners for buttons
+  document.querySelectorAll(".toggle-read").forEach(button => {
+    button.addEventListener("click", (e) => {
+      const index = e.target.dataset.index;


The dataset.index returns a string, not a number. When using myLibrary[index] with a string index on an array, it still works due to JavaScript's type coercion, but it's better practice to convert it to a number explicitly: const index = parseInt(e.target.dataset.index, 10);. This ensures proper array indexing and prevents potential bugs if the array is later used in contexts where type matters.

Copilot · 2025-12-03T17:54:56Z

debugging/book-library/script.js

+  document.querySelectorAll(".delete-book").forEach(button => {
+    button.addEventListener("click", (e) => {
+      const index = e.target.dataset.index;
+      myLibrary.splice(index, 1);
      render();
+
+      // Display a non-blocking notification
+      const notification = document.createElement("div");
+      notification.textContent = "Book deleted successfully.";
+      notification.className = "notification";
+      document.body.appendChild(notification);
+
+      // Remove the notification after 3 seconds
+      setTimeout(() => {
+        notification.remove();
+      }, 3000);
    });
-  }
+  });


Every time render() is called, new event listeners are attached to all delete buttons. This creates memory leaks as old listeners aren't removed. Use event delegation instead by attaching a single listener to the table element.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

MohammedNaru · 2025-12-03T18:25:38Z

It's a good idea to get a second opinion at the code from AI. I am sure there is a way to use it outside the scope of a PR so that the AI messages won't interfere with the review.

Hi CJ I have refactored changes, I did not meant to intentionally add copilot as a reviewer, I don't have access to remove it, please can you remove it as a reviewer with Admin access, thanks

cjyuan · 2025-12-03T20:36:56Z

According the W3C validator, your HTML code still have errors.

…input validation, and styling

MohammedNaru · 2025-12-04T00:20:51Z

@copilot open a new pull request to apply changes based on the comments in this thread

According the W3C validator, your HTML code still have errors.

I have updated now, I had rebase errors so I had to restart my changes but all should be working now, please review

cjyuan · 2025-12-04T01:05:11Z

Changes looks great! Well done!

Refactor book form and submission logic; enhance validation and local…

d2fb66a

… storage handling

MohammedNaru changed the title ~~Refactor book form and submission logic; enhance validation and local storage handling~~ West Midlands | 25-ITP-Sep | Ali Naru | Sprint 1 | feature/BookLibrary Nov 28, 2025

MohammedNaru added 📅 Sprint 2 Assigned during Sprint 2 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Module-Data-Flows The name of the module. labels Nov 28, 2025

cjyuan reviewed Nov 29, 2025

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Nov 29, 2025

MohammedNaru added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Dec 3, 2025

Refactor HTML structure and JavaScript logic; improve form handling a…

96b6342

…nd input validation

Copilot AI review requested due to automatic review settings December 3, 2025 14:46

Copilot started reviewing on behalf of MohammedNaru December 3, 2025 14:46 View session

Copilot finished reviewing on behalf of MohammedNaru December 3, 2025 14:49

Copilot AI reviewed Dec 3, 2025

View reviewed changes

cjyuan reviewed Dec 3, 2025

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Dec 3, 2025

MohammedNaru requested a review from Copilot December 3, 2025 17:50

Copilot started reviewing on behalf of MohammedNaru December 3, 2025 17:50 View session

Copilot finished reviewing on behalf of MohammedNaru December 3, 2025 17:52

Copilot AI reviewed Dec 3, 2025

View reviewed changes

MohammedNaru and others added 2 commits December 3, 2025 18:22

Update debugging/book-library/index.html

61da010

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update debugging/book-library/script.js

25b0dd3

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Dec 3, 2025

MohammedNaru added 2 commits December 4, 2025 00:09

Merge branch 'main' into Sprint-2

73aaa68

Refactor HTML structure and JavaScript logic; improve form handling, …

e66aa7d

…input validation, and styling

MohammedNaru added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Dec 4, 2025

cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Dec 4, 2025

		if (!title \|\| !author \|\| isNaN(pages)) {
		alert("Please fill all fields correctly!");

Uh oh!

West Midlands | 25-ITP-Sep | Ali Naru | Sprint 2 | feature/BookLibrary #334

Are you sure you want to change the base?

West Midlands | 25-ITP-Sep | Ali Naru | Sprint 2 | feature/BookLibrary #334

Uh oh!

Conversation

MohammedNaru commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Book Library Pull Request

Changelist

Questions

Uh oh!

github-actions bot commented Nov 28, 2025

Uh oh!

github-actions bot commented Nov 28, 2025

Uh oh!

github-actions bot commented Nov 28, 2025

Uh oh!

github-actions bot commented Nov 28, 2025

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

MohammedNaru commented Dec 3, 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 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

cjyuan Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

MohammedNaru commented Nov 28, 2025 •

edited

Loading