Add timeline, lifecycle, dependencies, search, calendar, reminders, attachments - #3
Merged
Merged
Conversation
…ttachments Six high-value features plus Markdown, bulk edits, and attachments, all within the existing no-dependency / single-SQLite-file design. - Activity timeline: an append-only ticket_events log records status, priority, device, due-date, title, and tag changes, recorded inside the same transaction as the change and shown interleaved with comments. - Device lifecycle: serial number, purchase date, warranty expiry, and cost, with a maintenance-sweep warranty check that opens one ticket per lapse. - Device dependencies: a self-referential parent link with a cycle guard, and a device page that shows what it depends on and what depends on it. - Full-text search: an FTS5 index over title, body, and comments kept current by triggers, with a safe query builder and a LIKE fallback. - Calendar feed: GET /api/calendar.ics of due dates and schedules, with the API token accepted in the query string for that read-only endpoint only. - Reminders and digest: a due-soon nudge that re-arms when a due date moves, and an opt-in daily/weekly backlog summary whose cadence is persisted in a meta table. - Attachments stored as BLOBs (single-file backup preserved) behind a short type allowlist; safe Markdown rendering for descriptions and notes; bulk close/resolve/tag from the ticket list. Adds migration 4, four new api/ modules, and 28 tests (150 -> 178). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xiY1C9gt2Z14c2zhQHNS8
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: 9 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour. 📝 WalkthroughWalkthroughThe application adds ticket activity history, full-text search, bulk updates, Markdown rendering, attachments, calendar feeds, device lifecycle data, warranty sweeps, due-soon reminders, and notification digests. ChangesTicket operations and maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MaintenanceScheduler
participant ServerMaintenance
participant WarrantySweep
participant Notifier
participant Database
MaintenanceScheduler->>ServerMaintenance: run periodic maintenance
ServerMaintenance->>WarrantySweep: sweepWarranties()
WarrantySweep->>Database: create warranty tickets and mark devices
ServerMaintenance->>Notifier: sweepDueSoon() and maybeSendDigest()
Notifier->>Database: read tickets and persist notification state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 58.62% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 58 functions across 12 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
src/db.js (1)
201-213: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAdd a trigger for comment updates.
The index tracks comment inserts and deletes only. If a comment body is ever made editable, the search index silently keeps the old text. An
AFTER UPDATE ON commentstrigger costs one statement and removes that failure mode.♻️ Proposed trigger
CREATE TRIGGER comments_fts_ad AFTER DELETE ON comments BEGIN UPDATE tickets_fts SET comments = (SELECT coalesce(group_concat(body, ' '), '') FROM comments WHERE ticket_id = old.ticket_id) WHERE rowid = old.ticket_id; END; + + CREATE TRIGGER comments_fts_au AFTER UPDATE ON comments BEGIN + UPDATE tickets_fts + SET comments = (SELECT coalesce(group_concat(body, ' '), '') FROM comments + WHERE ticket_id = new.ticket_id) + WHERE rowid = new.ticket_id; + END;🤖 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 `@src/db.js` around lines 201 - 213, Add an AFTER UPDATE trigger for the comments table alongside comments_fts_ai and comments_fts_ad. Recompute tickets_fts.comments with group_concat(body, ' ') for the updated comment’s ticket_id, using the existing ticket rowid relationship so edited comment bodies replace stale indexed text.test/tickets.test.js (2)
289-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the filler assertion.
assert.equal(a.id > 0, true)only keepsareferenced. Assert something meaningful instead, for example that a term unique to ticketareturns only that ticket.🤖 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 `@test/tickets.test.js` at line 289, Replace the filler assertion in the ticket test with a meaningful behavioral assertion that verifies searching for a term unique to ticket a returns only ticket a. Keep the assertion focused on the relevant search result and use the existing ticket/search symbols in the surrounding test.
329-333: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe test name promises an oversized case that is absent.
Both assertions cover empty or missing
ids. TheMAX_BULKbranch at src/api/tickets.js Line 223-225 stays untested. Add a case, or rename the test.💚 Proposed test addition
test('a bulk update rejects an empty or oversized id list', () => { const db = fresh(); assert.throws(() => bulkUpdateTickets(db, { ids: [], status: 'closed' }), { status: 400 }); assert.throws(() => bulkUpdateTickets(db, {}), { status: 400 }); + const tooMany = Array.from({ length: 501 }, (_, i) => i + 1); + assert.throws(() => bulkUpdateTickets(db, { ids: tooMany, status: 'closed' }), { status: 400 }); });🤖 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 `@test/tickets.test.js` around lines 329 - 333, Update the test around “a bulk update rejects an empty or oversized id list” to cover the oversized-list validation branch in bulkUpdateTickets, using a non-empty ids array larger than the configured MAX_BULK limit and asserting status 400. Keep the existing empty and missing-ids assertions.test/devices.test.js (1)
149-161: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case with a string identifier.
Every assertion passes numeric ids, so the tests cannot detect the strict-equality gap in
assertParentIsSafe. HTTP route parameters arrive as strings. AddupdateDevice(db, String(a.id), { parent_id: a.id })and expect a 400.💚 Proposed test addition
assert.throws(() => updateDevice(db, a.id, { parent_id: a.id }), { status: 400 }); + // Route parameters arrive as strings; the guard must still reject self-parenting. + assert.throws(() => updateDevice(db, String(a.id), { parent_id: a.id }), { status: 400 }); assert.throws(() => createDevice(db, { name: 'c', parent_id: 9999 }), { status: 400 });🤖 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 `@test/devices.test.js` around lines 149 - 161, Extend the “refuses a parent…” test around assertParentIsSafe/updateDevice with a case that passes String(a.id) as the device identifier while retaining a.id as parent_id, and assert it rejects with status 400. Keep the existing numeric self-parent, missing-parent, and loop assertions unchanged.src/api/devices.js (2)
61-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared open-ticket count expression.
The dependents query repeats the correlated subquery already present in
SELECT_DEVICEat Line 23-24. Extract it into one constant so both stay consistent when the closed-status rule changes.🤖 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 `@src/api/devices.js` around lines 61 - 69, Extract the correlated open-ticket count SQL expression currently duplicated in SELECT_DEVICE and the dependents query into a shared constant, then interpolate that constant in both queries. Preserve the existing device aliases and CLOSED_LIST filtering so both queries remain consistent.
170-188: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the parent walk against pre-existing cycles.
The route already converts
params.idto a number. Add a visited-IDSet; the foreign key does not prevent existing cycles, which can make this loop run forever.🤖 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 `@src/api/devices.js` around lines 170 - 188, Update assertParentIsSafe to track each cursor in a visited-ID Set during the parent walk, and stop or reject when an ID repeats so pre-existing cycles cannot cause an infinite loop. Preserve the existing self-dependency and new-loop validation behavior.src/api/metrics.js (1)
73-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the metric window with
WARRANTY_ALERT_DAYS.The gauge hardcodes 30 days. README Line 256 documents
WARRANTY_ALERT_DAYSas the configurable window for the warranty sweep. If an operator changes that value, the metric and the ticket sweep report different sets. The help text also says "lapses within 30 days", but the query counts already-expired warranties as well.🤖 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 `@src/api/metrics.js` around lines 73 - 78, Update the homelab_warranties_expiring metric query and help text to use the configured WARRANTY_ALERT_DAYS window instead of hardcoded 30-day wording or SQL. Match the warranty sweep’s boundary semantics so only non-retired devices with warranties expiring from now through the configured window are counted, keeping the metric aligned with the sweep.src/api/tickets.js (1)
230-233: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider returning identifiers instead of full tickets.
At the 500-ticket ceiling the response embeds 500 hydrated tickets, each carrying its comments, links, attachments, and event history.
getTicketalso runs four queries per ticket. The client at public/app.js Line 604-606 only readsresult.updated. Returning ids would cut both the query count and the payload size.🤖 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 `@src/api/tickets.js` around lines 230 - 233, Update the transaction return in the ticket update flow around updateTicket so it returns the updated ticket identifiers rather than hydrated ticket objects. Avoid calling the full ticket hydration path for each id, while preserving the existing updated count and the client-facing result.updated field.src/api/events.js (1)
9-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
EVENT_LABELSexport.public/app.js:eventLinerenders event text independently, andrecordChangesrecords status changes rather thanreopened; remove the stalereopenedentry with the map.🤖 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 `@src/api/events.js` around lines 9 - 19, Remove the unused EVENT_LABELS export and its entire map from the events module, including the stale reopened entry; eventLine and recordChanges do not depend on it.
🤖 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 `@public/app.js`:
- Around line 609-621: Update addTag to use the transactional /tickets/bulk
endpoint instead of sequential per-ticket PATCH requests, adding server-side
merge semantics so the new tag is preserved alongside each ticket’s existing
tags. Ensure bulk tagging is atomic and the success toast/render behavior
remains correct.
- Around line 883-887: Update the Activity heading in the card-rendering
expression to count the rendered entries using entries.length instead of
ticket.comments.length, keeping the existing entries.map(row) list unchanged.
- Around line 644-664: Update the call site that maps open tickets to use an
explicit single-argument callback, such as open.map((ticket) =>
ticketRow(ticket)), so ticketRow does not receive the array index as onToggle.
Preserve ticketRow’s existing optional toggle behavior for callers that
intentionally provide a callback.
- Around line 905-918: Update the upload handler around the X-Filename header to
pass an encodeURIComponent-encoded file.name, then update uploadAttachment to
decode that header with decodeURIComponent before forwarding the filename to
addAttachment. Preserve filename handling for Latin-1 and non-Latin-1 names.
In `@public/styles.css`:
- Around line 655-656: Update the .attachment delete-control styles so keyboard
focus also reveals the control, adding a focus-visible condition alongside the
existing hover rule while preserving the current transition and layout behavior.
In `@README.md`:
- Around line 81-83: Update the fenced code block containing the calendar API
URL to specify the text language identifier, changing the opening fence to use
text while preserving the URL example unchanged.
- Around line 121-126: Rewrite the warranty-check sentence in the README so it
directly states that editing the warranty date re-arms the alert, replacing the
incomplete phrase “editing the date re-arming it” while preserving the
surrounding behavior and configuration details.
In `@src/api/attachments.js`:
- Around line 66-72: Update deleteAttachment to refresh the parent ticket’s
timestamp after the attachment deletion succeeds, matching the behavior in
addAttachment. Use the existing ticket update mechanism and ticketId, ensuring
it runs only after db.prepare('DELETE FROM attachments...').run completes.
In `@src/api/tickets.js`:
- Around line 218-234: Deduplicate the validated ticket IDs in bulkUpdateTickets
before enforcing the MAX_BULK limit and running the transaction. Use the
deduplicated IDs for updateTicket calls and the updated count, while preserving
requiredId validation for every input ID.
In `@src/db.js`:
- Around line 176-186: Raise the project’s minimum Node.js version to 22.16.0 or
newer in package.json and README.md so the tickets_fts migration can rely on
node:sqlite FTS5 support. Keep the existing FTS5 migration unchanged; update all
documented and enforced version constraints consistently.
In `@src/notify.js`:
- Around line 151-168: Update maybeSendDigest to atomically reserve a digest
lease in the meta table before awaiting deliver(), so concurrent calls cannot
both send. Treat unexpired digest lease values as unavailable, expire abandoned
leases using the configured cadence, clear the lease when delivery fails, and
replace it with digest_last_sent only after successful delivery.
- Around line 116-135: Update the ticket-claiming loop in sweepDueSoon around
the mark statement so the UPDATE only succeeds when due_soon_notified_at is
still NULL. Send the ticket.due_soon notification and add its ID to sent only
when mark.run reports one changed row; skip delivery when another overlapping
sweep already claimed it.
---
Nitpick comments:
In `@src/api/devices.js`:
- Around line 61-69: Extract the correlated open-ticket count SQL expression
currently duplicated in SELECT_DEVICE and the dependents query into a shared
constant, then interpolate that constant in both queries. Preserve the existing
device aliases and CLOSED_LIST filtering so both queries remain consistent.
- Around line 170-188: Update assertParentIsSafe to track each cursor in a
visited-ID Set during the parent walk, and stop or reject when an ID repeats so
pre-existing cycles cannot cause an infinite loop. Preserve the existing
self-dependency and new-loop validation behavior.
In `@src/api/events.js`:
- Around line 9-19: Remove the unused EVENT_LABELS export and its entire map
from the events module, including the stale reopened entry; eventLine and
recordChanges do not depend on it.
In `@src/api/metrics.js`:
- Around line 73-78: Update the homelab_warranties_expiring metric query and
help text to use the configured WARRANTY_ALERT_DAYS window instead of hardcoded
30-day wording or SQL. Match the warranty sweep’s boundary semantics so only
non-retired devices with warranties expiring from now through the configured
window are counted, keeping the metric aligned with the sweep.
In `@src/api/tickets.js`:
- Around line 230-233: Update the transaction return in the ticket update flow
around updateTicket so it returns the updated ticket identifiers rather than
hydrated ticket objects. Avoid calling the full ticket hydration path for each
id, while preserving the existing updated count and the client-facing
result.updated field.
In `@src/db.js`:
- Around line 201-213: Add an AFTER UPDATE trigger for the comments table
alongside comments_fts_ai and comments_fts_ad. Recompute tickets_fts.comments
with group_concat(body, ' ') for the updated comment’s ticket_id, using the
existing ticket rowid relationship so edited comment bodies replace stale
indexed text.
In `@test/devices.test.js`:
- Around line 149-161: Extend the “refuses a parent…” test around
assertParentIsSafe/updateDevice with a case that passes String(a.id) as the
device identifier while retaining a.id as parent_id, and assert it rejects with
status 400. Keep the existing numeric self-parent, missing-parent, and loop
assertions unchanged.
In `@test/tickets.test.js`:
- Line 289: Replace the filler assertion in the ticket test with a meaningful
behavioral assertion that verifies searching for a term unique to ticket a
returns only ticket a. Keep the assertion focused on the relevant search result
and use the existing ticket/search symbols in the surrounding test.
- Around line 329-333: Update the test around “a bulk update rejects an empty or
oversized id list” to cover the oversized-list validation branch in
bulkUpdateTickets, using a non-empty ids array larger than the configured
MAX_BULK limit and asserting status 400. Keep the existing empty and missing-ids
assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d77cf83d-7210-40e4-bba1-7a093afe8b08
📒 Files selected for processing (23)
.env.exampleREADME.mdpublic/app.jspublic/styles.csssrc/api/attachments.jssrc/api/calendar.jssrc/api/devices.jssrc/api/events.jssrc/api/export.jssrc/api/metrics.jssrc/api/tickets.jssrc/api/warranty.jssrc/auth.jssrc/config.jssrc/db.jssrc/notify.jssrc/server.jssrc/validate.jstest/devices.test.jstest/hardening.test.jstest/http.test.jstest/ops.test.jstest/tickets.test.js
Fixes verified against the code; low-value nitpicks skipped. - Bulk tagging is now atomic: a new add_tags option on /tickets/bulk merges a label onto each ticket's own set inside the one transaction, replacing the client's sequential per-ticket PATCH loop. - bulkUpdateTickets dedupes ids before the batch limit and the transaction, so a repeated id is neither updated nor counted twice. - Digest sending claims an atomic lease in meta before delivering, releasing it on failure, so an ad-hoc /api/maintenance/run overlapping the timer cannot double-send. The due-soon sweep claims each ticket with a conditional UPDATE for the same reason. - deleteAttachment bumps the ticket's updated_at, matching addAttachment. - assertParentIsSafe coerces the id to a number (a string id no longer slips the self-check) and tracks visited devices so a corrupted pre-existing cycle can't loop forever. - Attachment filenames are percent-encoded into X-Filename and decoded server side, so accented and emoji names survive the Latin-1 header. - The warranty metric uses the configured WARRANTY_ALERT_DAYS window. - Frontend: Activity heading counts rendered entries; device open-ticket list no longer passes the map index as the selection callback. - New migration adds the missing AFTER UPDATE comments FTS trigger; shared the open-ticket count SQL; removed the unused EVENT_LABELS export. - Node floor raised to 22.16.0 (FTS5); attachment delete control reveals on keyboard focus; README fence language and warranty wording fixed. - Tests: bulk oversize/dedupe/add_tags, string-id self-parent, a meaningful search assertion (180 passing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xiY1C9gt2Z14c2zhQHNS8
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/api/metrics.js`:
- Around line 76-80: Update the help text for the homelab_warranties_expiring
metric to state that the gauge includes already expired warranties, matching the
existing SQL query’s behavior; leave the query unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60a6f52b-a8b4-468f-bf5f-a6e92987530c
📒 Files selected for processing (14)
README.mdpackage.jsonpublic/app.jspublic/styles.csssrc/api/attachments.jssrc/api/devices.jssrc/api/events.jssrc/api/metrics.jssrc/api/tickets.jssrc/db.jssrc/notify.jssrc/server.jstest/devices.test.jstest/tickets.test.js
💤 Files with no reviewable changes (1)
- src/api/events.js
🚧 Files skipped from review as they are similar to previous changes (10)
- test/devices.test.js
- public/styles.css
- test/tickets.test.js
- src/api/attachments.js
- src/notify.js
- src/db.js
- src/server.js
- README.md
- src/api/tickets.js
- public/app.js
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
2 participants
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.
Adds activity timelines, device lifecycle and dependency tracking, full-text search, calendar feeds, reminders, Markdown, bulk actions, and attachments while preserving the dependency-free Node.js and single-SQLite-file design.
Requires Node 22.16 or newer. Includes versioned schema migrations and expanded configuration and API documentation.
Validation: all 188 tests pass locally on Windows with Node 22.16.0, 24.21.0, and 26.8.1. Coverage includes Unicode attachment round trips, authentication enforcement at executable startup, authenticated export and metrics, and ticket persistence across a process restart. Frontend JavaScript syntax checks and git diff --check pass. Hosted CI and Docker validation remain separate gates.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation