Pull Request for docker/multi-container to compared and implemented the both #65
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| name: NodeJS with Webpack | ||
|
|
||
| on: | ||
| push: | ||
| branches: [ "main" ] | ||
| pull_request: | ||
| branches: [ "main" ] | ||
|
|
||
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| strategy: | ||
| matrix: | ||
| node-version: [18.x, 20.x, 22.x] | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Use Node.js ${{ matrix.node-version }} | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: ${{ matrix.node-version }} | ||
|
|
||
| - name: Build | ||
| run: | | ||
| npm install | ||
| npx webpack | ||
|
Comment on lines
+1
to
+29
There was a problem hiding this comment. The workflow is named 'NodeJS with Webpack' but the repository doesn't use webpack - it's an Express/EJS application. Additionally, the build step runs 'npx webpack' which will fail since webpack is not configured or installed. This workflow appears to be copied from a template and not adapted for this project.
Author
There was a problem hiding this comment. @copilot open a new pull request to apply changes based on this feedback |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| // Enhanced configuration: supports environment-based MongoDB URIs for flexibility and security | ||
| const mongoProdURI = process.env.MONGO_PROD_URI || 'mongodb://todo-database:27017/todoapp'; | ||
|
|
||
| module.exports = { | ||
| mongoProdURI, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,5 +30,24 @@ router.post('/todo/destroy', async (req, res) => { | |
| res.redirect('/'); | ||
| }); | ||
|
|
||
| // POST - Edit todo item | ||
| router.post('/todo/edit', async (req, res) => { | ||
| const taskKey = req.body._key; | ||
| const newTask = req.body.task; | ||
| await Todo.findByIdAndUpdate(taskKey, { task: newTask }); | ||
| res.redirect('/'); | ||
| }); | ||
|
|
||
| // POST - Toggle completion status | ||
| router.post('/todo/toggle', async (req, res) => { | ||
| const taskKey = req.body._key; | ||
| const todo = await Todo.findById(taskKey); | ||
| if (todo) { | ||
| todo.completed = !todo.completed; | ||
| await todo.save(); | ||
| } | ||
| res.redirect('/'); | ||
| }); | ||
|
Comment on lines
+82
to
+164
There was a problem hiding this comment. The new /todo/edit and /todo/toggle endpoints lack input validation. If req.body._key is missing, undefined, or malformed, the routes will fail or behave unpredictably. Add validation to check that _key exists and is a valid MongoDB ObjectId before querying the database. |
||
|
|
||
|
|
||
| module.exports = router; | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -42,24 +42,39 @@ | |||||
| <div class="row"> | ||||||
| <div class="col-12"> | ||||||
| <% if(Object.keys(tasks).length> 0) { %> | ||||||
| <ul class="nav flex-column" role="list" aria-label="Todo list"> | ||||||
| <% tasks.forEach(todo=> { %> | ||||||
| <li class="nav-item" role="listitem"> | ||||||
|
There was a problem hiding this comment. The attribute 'role="listitem"' is not a valid ARIA role. The correct role is "listitem" (one word), but more importantly, when using semantic HTML with a 'ul' element, the child 'li' elements already have an implicit role of listitem. This explicit role attribute is redundant and should be removed.
Suggested change
|
||||||
| <div class="d-flex justify-content-between py-1 align-items-center" aria-live="polite"> | ||||||
| <div class="d-flex flex-row align-items-center"> | ||||||
| <form method="POST" action="/todo/toggle" aria-label="Toggle task completion"> | ||||||
| <input type="hidden" name="_key" value="<%= todo._id %>" /> | ||||||
| <input type="checkbox" name="completed" onchange="this.form.submit()" <%= todo.completed ? 'checked' : '' %> aria-label="Mark task as <%= todo.completed ? 'incomplete' : 'completed' %>"> | ||||||
|
There was a problem hiding this comment. The checkbox has 'onchange="this.form.submit()"' which will cause a traditional form submission and page reload. However, there's also an AJAX handler set up in the script section (lines 94-109) that's meant to handle the submission via AJAX. These two approaches conflict - the traditional form submit will execute before the AJAX handler can preventDefault. Remove the inline onchange attribute to let the AJAX handler manage the submission.
Suggested change
|
||||||
| </form> | ||||||
| <div class="ml-2"> | ||||||
| <span <%= todo.completed ? 'style="text-decoration: line-through; color: #888;"' : '' %>><%= todo.task %></span> | ||||||
| <p class="text-muted mb-0"><small> | ||||||
| <%= moment(todo.created_at).fromNow() %> | ||||||
| </small></p> | ||||||
| <span class="badge badge-<%= todo.completed ? 'success' : 'secondary' %> ml-1" aria-label="Task status"> | ||||||
| <%= todo.completed ? 'Completed' : 'Pending' %> | ||||||
| </span> | ||||||
| </div> | ||||||
| </div> | ||||||
| <a href="#" class="text-info mr-2" onclick="document.getElementById('edit-form-<%= todo._id %>').style.display='block';return false;" aria-label="Edit task"> | ||||||
|
There was a problem hiding this comment. The inline onclick handler 'onclick="document.getElementById('edit-form-<%= todo._id %>').style.display='block';return false;"' contains unescaped single quotes within the EJS template output, which will break the JavaScript string. The todo._id and 'block' values need proper escaping or the attribute should use double quotes with escaped inner quotes.
Suggested change
|
||||||
| <i class="fa fa-pencil" aria-hidden="true"></i> | ||||||
| </a> | ||||||
| <form id="edit-form-<%= todo._id %>" method="POST" action="/todo/edit" style="display:none;" class="form-inline" aria-label="Edit task form"> | ||||||
| <input type="hidden" name="_key" value="<%= todo._id %>" /> | ||||||
| <input type="text" name="task" value="<%= todo.task %>" class="form-control form-control-sm mr-2" aria-label="Edit task input"> | ||||||
| <button type="submit" class="btn btn-success btn-sm" aria-label="Save edit">Save</button> | ||||||
| <button type="button" class="btn btn-secondary btn-sm" onclick="this.parentElement.style.display='none';return false;" aria-label="Cancel edit">Cancel</button> | ||||||
|
There was a problem hiding this comment. Similar to line 64, this inline onclick handler has the same quote escaping issue. The JavaScript 'this.parentElement.style.display='none'' contains unescaped quotes that will break the attribute.
Suggested change
|
||||||
| </form> | ||||||
| <a href="javascript:;" onclick="this.children[0].submit()" class="text-danger" aria-label="Delete task"> | ||||||
|
There was a problem hiding this comment. Using 'href="javascript:;"' for the delete link is an outdated pattern. A better approach would be to use 'href="#"' with 'event.preventDefault()' in a proper click handler, or use a button element with type="button" which is more semantically correct for an action that doesn't navigate.
Suggested change
|
||||||
| <form method="POST" action="/todo/destroy" aria-label="Delete task form"> | ||||||
| <input type="hidden" name="_key" value="<%= todo._id %>" /> | ||||||
| </form> | ||||||
| <i class="fa fa-trash-o" aria-hidden="true"></i> | ||||||
| </a> | ||||||
| </div> | ||||||
| </li> | ||||||
|
|
@@ -74,6 +89,41 @@ | |||||
| </div> | ||||||
| </div> | ||||||
| </div> | ||||||
| <script> | ||||||
| // AJAX for toggling completion | ||||||
| document.querySelectorAll('form[action="/todo/toggle"] input[type="checkbox"]').forEach(function(checkbox) { | ||||||
| checkbox.addEventListener('change', function(e) { | ||||||
| var form = this.form; | ||||||
| var xhr = new XMLHttpRequest(); | ||||||
| xhr.open('POST', form.action, true); | ||||||
| xhr.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded'); | ||||||
| xhr.onload = function() { | ||||||
| if (xhr.status === 200) { | ||||||
| location.reload(); // For simplicity, reload to reflect changes | ||||||
| } | ||||||
| }; | ||||||
| var params = '_key=' + encodeURIComponent(form._key.value); | ||||||
| xhr.send(params); | ||||||
| e.preventDefault(); | ||||||
|
dreadwitdastacc-IFA marked this conversation as resolved.
dreadwitdastacc-IFA marked this conversation as resolved.
|
||||||
| }); | ||||||
| }); | ||||||
|
|
||||||
| // AJAX for editing tasks | ||||||
| document.querySelectorAll('form[action="/todo/edit"]').forEach(function(editForm) { | ||||||
| editForm.addEventListener('submit', function(e) { | ||||||
| e.preventDefault(); | ||||||
| var xhr = new XMLHttpRequest(); | ||||||
| xhr.open('POST', editForm.action, true); | ||||||
| xhr.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded'); | ||||||
| xhr.onload = function() { | ||||||
| if (xhr.status === 200) { | ||||||
| location.reload(); // For simplicity, reload to reflect changes | ||||||
| } | ||||||
| }; | ||||||
| var params = '_key=' + encodeURIComponent(editForm._key.value) + '&task=' + encodeURIComponent(editForm.task.value); | ||||||
| xhr.send(params); | ||||||
| }); | ||||||
| }); | ||||||
| </script> | ||||||
| </body> | ||||||
| </html> | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The npm install command runs in the repository root, but package.json is located in the 'app/' subdirectory. This will fail because there's no package.json in the root. The workflow should change to the app directory before running npm install.