fix: resolve all 12 debugging bugs in launch checklist app#2
Open
arunod-wijesena wants to merge 15 commits intocodezelaca:mainfrom
Open
fix: resolve all 12 debugging bugs in launch checklist app#2arunod-wijesena wants to merge 15 commits intocodezelaca:mainfrom
arunod-wijesena wants to merge 15 commits intocodezelaca:mainfrom
Conversation
…unctionality in app.js
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFixes in js/app.js: align storage key, wire and validate add-check form, tighten load/save parsing, expand filtering and slugify status classes, persist status changes and refresh metrics/views, correct delete handling, fix demo JSON path and CSV export, and reorganize helper/comment blocks. ChangesLaunch Checklist Bug Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
js/app.js (2)
289-294: 💤 Low valueRedundant
updateMetrics()call.
applyFilters()already callsrenderApp()which callsupdateMetrics(). The explicit call at line 292 results in metrics being computed twice.♻️ Remove redundant call
check.status = statusSelect.value; saveChecks(); applyFilters(); - updateMetrics(); logActivity(`Changed "${check.title}" to ${check.status}.`); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app.js` around lines 289 - 294, The code sets check.status, calls saveChecks(), then calls applyFilters() and updateMetrics(), but updateMetrics() is redundant because applyFilters() -> renderApp() already calls updateMetrics(); remove the explicit updateMetrics() invocation after applyFilters() so metrics are only computed once (leave check.status assignment, saveChecks(), applyFilters(), and logActivity(`Changed "${check.title}" to ${check.status}.`) intact).
202-202: 💤 Low valueConsider using
replaceAllfor consistency.
replace(" ", "-")only replaces the first space. While current statuses only have one space max, usingreplaceAllwould be more robust and consistent with other string replacements in this file (e.g.,escapeHtmlat lines 384-388).♻️ Suggested improvement
- const statusClass = `status-${check.status.toLowerCase().replace(" ", "-")}`; + const statusClass = `status-${check.status.toLowerCase().replaceAll(" ", "-")}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app.js` at line 202, The current construction of statusClass uses check.status.toLowerCase().replace(" ", "-") which only replaces the first space; change this to use replaceAll so all spaces are converted consistently (e.g., update the expression building statusClass to call replaceAll(" ", "-") on check.status.toLowerCase()), ensuring behavior matches other replacements in the file like escapeHtml and handles multi-word statuses robustly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@js/app.js`:
- Around line 289-294: The code sets check.status, calls saveChecks(), then
calls applyFilters() and updateMetrics(), but updateMetrics() is redundant
because applyFilters() -> renderApp() already calls updateMetrics(); remove the
explicit updateMetrics() invocation after applyFilters() so metrics are only
computed once (leave check.status assignment, saveChecks(), applyFilters(), and
logActivity(`Changed "${check.title}" to ${check.status}.`) intact).
- Line 202: The current construction of statusClass uses
check.status.toLowerCase().replace(" ", "-") which only replaces the first
space; change this to use replaceAll so all spaces are converted consistently
(e.g., update the expression building statusClass to call replaceAll(" ", "-")
on check.status.toLowerCase()), ensuring behavior matches other replacements in
the file like escapeHtml and handles multi-word statuses robustly.
arunod-wijesena
changed the title
arunod-wijesena
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/app.js (1)
106-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate persisted/fetched payload shape before assigning to
checks.
JSON.parse(...)andresponse.json()can return non-array values. When that happens, flows usingchecks.filter(...)/checks.unshift(...)fail at runtime. Add anArray.isArrayguard at both ingress points.Proposed fix
function loadChecks() { const saved = localStorage.getItem(STORAGE_LOAD_KEY); if (!saved) { return [...demoChecks]; } try { - return JSON.parse(saved); + const parsed = JSON.parse(saved); + return Array.isArray(parsed) ? parsed : [...demoChecks]; } catch (error) { console.warn("Could not parse saved launch checks.", error); return [...demoChecks]; } }async function resetDemoData() { formMessage.textContent = ""; try { const response = await fetch("data/launch-checks.json"); if (!response.ok) { throw new Error(`Demo data request failed with ${response.status}`); } - checks = await response.json(); + const data = await response.json(); + if (!Array.isArray(data)) { + throw new Error("Demo data payload is not an array."); + } + checks = data; saveChecks(); applyFilters(); logActivity("Demo checklist reloaded from JSON."); } catch (error) {Also applies to: 309-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app.js` around lines 106 - 119, The parsed payload from localStorage in loadChecks (STORAGE_LOAD_KEY) and the payload returned by response.json() must be validated as arrays before assigning to checks to avoid runtime errors when calling checks.filter or checks.unshift; update loadChecks to JSON.parse(saved) then use Array.isArray(...) to return the parsed value only if it's an array (otherwise return [...demoChecks]) and apply the same pattern where you consume response.json() (the fetch/response handling that assigns to checks) so that non-array responses fall back to [...demoChecks] and do not replace checks with a non-array value.
🧹 Nitpick comments (1)
js/app.js (1)
206-206: ⚡ Quick winMake status class slugging fully whitespace-safe.
At Line 206,
.replace(" ", "-")only replaces the first space. Using a global whitespace replacement makes class naming stable for any multi-word status.Proposed refactor
- const statusClass = `status-${check.status.toLowerCase().replace(" ", "-")}`; + const statusClass = `status-${String(check.status ?? "") + .trim() + .toLowerCase() + .replace(/\s+/g, "-")}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app.js` at line 206, The class generation for statusClass uses check.status.toLowerCase().replace(" ", "-") which only replaces the first space; update it to replace all whitespace (e.g., use a global regex like /\s+/g or replaceAll for spaces) so multi-word check.status values are slugged consistently, ensuring statusClass is built from check.status with all whitespace replaced by hyphens.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@js/app.js`:
- Around line 106-119: The parsed payload from localStorage in loadChecks
(STORAGE_LOAD_KEY) and the payload returned by response.json() must be validated
as arrays before assigning to checks to avoid runtime errors when calling
checks.filter or checks.unshift; update loadChecks to JSON.parse(saved) then use
Array.isArray(...) to return the parsed value only if it's an array (otherwise
return [...demoChecks]) and apply the same pattern where you consume
response.json() (the fetch/response handling that assigns to checks) so that
non-array responses fall back to [...demoChecks] and do not replace checks with
a non-array value.
---
Nitpick comments:
In `@js/app.js`:
- Line 206: The class generation for statusClass uses
check.status.toLowerCase().replace(" ", "-") which only replaces the first
space; update it to replace all whitespace (e.g., use a global regex like /\s+/g
or replaceAll for spaces) so multi-word check.status values are slugged
consistently, ensuring statusClass is built from check.status with all
whitespace replaced by hyphens.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/app.js`:
- Line 137: The code uses toISOString() and new Date('YYYY-MM-DD') which
treat/produce UTC dates and cause off-by-one local-date errors; change places
that create or parse date-only values (e.g., dueDate default via
dueDateInput.value / dueDate, the parser used in the block around lines 375-392,
and any dueSoon comparisons) to construct dates in local time by splitting the
YYYY-MM-DD string into year/month/day and using new Date(year, month-1, day)
(and when defaulting build today's YYYY-MM-DD from new Date() local values), and
ensure any comparisons normalize to local-midnight Date objects so due dates and
dueSoon logic use local calendar days instead of UTC.
- Around line 115-116: The code assigns JSON-parsed data to checks using only
Array.isArray(parsed) which is unsafe; update the logic where parsed is used
(the block returning parsed vs [...demoChecks], and the similar block at lines
~316-318) to validate each item is an object and has string fields "priority",
"status", and "dueDate" (and any other required checklist properties) before
accepting it—e.g., filter or map parsed to include only items that pass typeof
checks and required key presence, and if validation fails for the array, fall
back to demoChecks or call resetDemoData(); reference the parsed variable and
the resetDemoData handling so malformed localStorage/demo payloads never get
assigned to checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+115
to
+116
| const parsed = JSON.parse(saved); | ||
| return Array.isArray(parsed) ? parsed : [...demoChecks] |
There was a problem hiding this comment.
Validate checklist items before assigning them to checks.
Array.isArray(parsed) is too weak here, and resetDemoData() accepts any JSON payload. The render path later assumes every item has string priority/status/dueDate fields, so malformed localStorage or a bad demo file can still take down the app on the next render.
Suggested hardening
+function cloneDemoChecks() {
+ return demoChecks.map((check) => ({ ...check }))
+}
+
+function isValidCheck(check) {
+ return (
+ check &&
+ Number.isFinite(check.id) &&
+ typeof check.title === "string" &&
+ typeof check.category === "string" &&
+ typeof check.priority === "string" &&
+ typeof check.status === "string" &&
+ typeof check.owner === "string" &&
+ typeof check.dueDate === "string" &&
+ typeof check.notes === "string"
+ )
+}
+
+function normalizeChecks(value) {
+ return Array.isArray(value) && value.every(isValidCheck)
+ ? value
+ : cloneDemoChecks()
+}
+
function loadChecks() {
const saved = localStorage.getItem(STORAGE_LOAD_KEY);
@@
try {
const parsed = JSON.parse(saved);
- return Array.isArray(parsed) ? parsed : [...demoChecks]
+ return normalizeChecks(parsed)
} catch (error) {
console.warn("Could not parse saved launch checks.", error);
- return [...demoChecks];
+ return cloneDemoChecks();
}
}
@@
- checks = await response.json();
+ checks = normalizeChecks(await response.json());Also applies to: 316-318
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/app.js` around lines 115 - 116, The code assigns JSON-parsed data to
checks using only Array.isArray(parsed) which is unsafe; update the logic where
parsed is used (the block returning parsed vs [...demoChecks], and the similar
block at lines ~316-318) to validate each item is an object and has string
fields "priority", "status", and "dueDate" (and any other required checklist
properties) before accepting it—e.g., filter or map parsed to include only items
that pass typeof checks and required key presence, and if validation fails for
the array, fall back to demoChecks or call resetDemoData(); reference the parsed
variable and the resetDemoData handling so malformed localStorage/demo payloads
never get assigned to checks.
| @@ -132,8 +136,7 @@ function handleAddCheck(event) { | |||
| const owner = ownerInput.value.trim() || "Unassigned"; | |||
| const dueDate = dueDateInput.value || new Date().toISOString().slice(0, 10); | |||
There was a problem hiding this comment.
Handle date-only values in local time instead of UTC.
Line 137 uses toISOString(), and Lines 375-392 parse YYYY-MM-DD with new Date(...). In US time zones that shifts calendar dates, so due dates can render a day early, dueSoon can be off by one, and blank due dates can default to the wrong day around midnight.
Suggested fix
+function toLocalDateString(date = new Date()) {
+ const year = date.getFullYear()
+ const month = String(date.getMonth() + 1).padStart(2, "0")
+ const day = String(date.getDate()).padStart(2, "0")
+ return `${year}-${month}-${day}`
+}
+
+function parseLocalDate(dateValue) {
+ if (!dateValue) {
+ return null
+ }
+
+ const [year, month, day] = dateValue.split("-").map(Number)
+ return new Date(year, month - 1, day)
+}
+
function handleAddCheck(event) {
@@
- const dueDate = dueDateInput.value || new Date().toISOString().slice(0, 10);
+ const dueDate = dueDateInput.value || toLocalDateString();
@@
function daysUntil(dateValue) {
- const today = new Date();
- const target = new Date(dateValue);
+ const today = new Date();
+ today.setHours(0, 0, 0, 0);
+
+ const target = parseLocalDate(dateValue);
+ if (!target || Number.isNaN(target.getTime())) {
+ return Number.POSITIVE_INFINITY;
+ }
+
const difference = target.getTime() - today.getTime();
return Math.ceil(difference / 86400000);
}
@@
function formatDate(dateValue) {
if (!dateValue) {
return "No date";
}
+ const parsed = parseLocalDate(dateValue);
+ if (!parsed || Number.isNaN(parsed.getTime())) {
+ return "Invalid date";
+ }
+
return new Intl.DateTimeFormat("en", {
month: "short",
day: "numeric",
year: "numeric",
- }).format(new Date(dateValue));
+ }).format(parsed);
}Also applies to: 252-255, 375-392
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/app.js` at line 137, The code uses toISOString() and new
Date('YYYY-MM-DD') which treat/produce UTC dates and cause off-by-one local-date
errors; change places that create or parse date-only values (e.g., dueDate
default via dueDateInput.value / dueDate, the parser used in the block around
lines 375-392, and any dueSoon comparisons) to construct dates in local time by
splitting the YYYY-MM-DD string into year/month/day and using new Date(year,
month-1, day) (and when defaulting build today's YYYY-MM-DD from new Date()
local values), and ensure any comparisons normalize to local-midnight Date
objects so due dates and dueSoon logic use local calendar days instead of UTC.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
js/app.js (1)
298-301: 💤 Low valueRedundant
updateMetrics()call.
applyFilters()at line 299 already callsrenderApp(), which in turn callsupdateMetrics(). The explicitupdateMetrics()call at line 300 is redundant.Suggested fix
check.status = statusSelect.value; saveChecks(); applyFilters(); - updateMetrics(); logActivity(`Changed "${check.title}" to ${check.status}.`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app.js` around lines 298 - 301, The explicit call to updateMetrics() is redundant because applyFilters() already triggers renderApp(), which calls updateMetrics(); remove the standalone updateMetrics() invocation in the sequence (the block containing saveChecks(), applyFilters(), updateMetrics(), logActivity(...)) so the flow becomes saveChecks(); applyFilters(); logActivity(...), keeping renderApp()/updateMetrics() behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@js/app.js`:
- Around line 298-301: The explicit call to updateMetrics() is redundant because
applyFilters() already triggers renderApp(), which calls updateMetrics(); remove
the standalone updateMetrics() invocation in the sequence (the block containing
saveChecks(), applyFilters(), updateMetrics(), logActivity(...)) so the flow
becomes saveChecks(); applyFilters(); logActivity(...), keeping
renderApp()/updateMetrics() behavior intact.
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Chores