feat: op insights knowledge center#1167
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds Knowledge Center analytics end-to-end: new DB model and test migration entries, GORM aggregation queries (totals, unique users, avg session minutes, repeat buckets, category views, top content with prior-window deltas), an admin API endpoint with facility/date handling, integration tests, and frontend UI/components plus date-range utilities. ChangesKnowledge Center Analytics Dashboard
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🤖 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 `@backend/src/database/knowledge_center.go`:
- Around line 112-117: In GetKCLibraryViewsByCategory replace the direct join
from open_content_activities to tags with a join through the libraries table so
only library content is counted: first join libraries (e.g., JOIN libraries l ON
l.id = oca.content_id AND l.open_content_provider_id =
oca.open_content_provider_id), then join open_content_tags using the library id
(oct.content_id = l.id AND oct.open_content_provider_id =
l.open_content_provider_id), and finally join tags on oct.tag_id = t.id; update
any selected/filtered references to use the library join to prevent
cross-content ID overlap from inflating category view counts.
- Around line 60-84: GetKCAvgSessionMinutes currently materializes all session
rows and computes durations in Go; instead push the aggregation to the DB by
using kcActivityScope(args, start, end, facilityID) to SELECT the average
session length SQL-side (e.g., AVG(EXTRACT(EPOCH FROM stop_ts - request_ts)/60)
or database-equivalent) with WHERE stop_ts IS NOT NULL AND stop_ts > request_ts,
then Scan the resulting avg value into a float64, return 0,nil if NULL, and
preserve error wrapping via newGetRecordsDBError on query failure; update
GetKCAvgSessionMinutes to use that single aggregate query rather than loading
rows and iterating.
In `@backend/src/handlers/dashboard.go`:
- Around line 287-292: The parsed facility ID is cast to uint without rejecting
non-positive values, which turns 0 or negatives into invalid/wrapped uints; in
the code around strconv.Atoi(facility) (variables facilityIdInt and ref), add a
check after parsing: if facilityIdInt <= 0 { return nil,
newInvalidIdServiceError(fmt.Errorf("invalid facility id: %d", facilityIdInt),
"facility") } (or use an appropriate error value) so you reject zero and
negative IDs before converting to uint and returning &ref.
In `@frontend/src/pages/insights/insightsRange.ts`:
- Around line 37-49: priorParams currently assumes params.start_date and
params.end_date are valid and ordered; update priorParams to validate and
normalize inputs before computing the prior range: parse params.start_date and
params.end_date into Date objects and if either is missing/invalid return the
original params (or a safe default), if start > end swap them (or set start =
end) so the range is non-negative, compute durationDays as Math.max(1,
Math.round((end.getTime() - start.getTime()) / 86400000) + 1), then compute
priorStart/priorEnd from the normalized start and duration, and finally return
formatted dates via formatDate; reference function priorParams, type
InsightsDateParams and helper formatDate to locate changes.
In `@frontend/src/pages/insights/KCContentTable.tsx`:
- Around line 55-57: The TableRow key uses only row.title which can collide;
update the rows.map key to a collision-safe stable identifier by using a unique
property on the row (e.g., row.id) or, if no unique id exists, derive a stable
composite key (e.g., combine a unique field and the map index) instead of plain
title; change the TableRow key in the rows.map callback to reference that unique
identifier so React can reliably track rows.
In `@frontend/src/pages/insights/OverviewTab.tsx`:
- Around line 60-62: The current changePct function masks real increases by
returning 0 when prior === 0; update changePct to explicitly handle three cases:
if prior === 0 and current === 0 return 0, if prior === 0 and current > 0 return
null (or another sentinel) to indicate an undefined/infinite percent-change,
otherwise compute and return the rounded percent as before; then update
consumers of changePct (the usages around the locations referenced in the
review) to handle the null sentinel for display/formatting (e.g., show "—" or
"New" instead of 0) so increases from a zero baseline are not misrepresented.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a75f6663-8147-42b1-87f9-6fce24d68ec4
📒 Files selected for processing (13)
backend/src/database/DB.gobackend/src/database/knowledge_center.gobackend/src/handlers/dashboard.gobackend/src/models/open_content.gobackend/src/models/tags.gobackend/tests/integration/knowledge_center_test.gofrontend/src/pages/insights/KCContentTable.tsxfrontend/src/pages/insights/KnowledgeCenterTab.tsxfrontend/src/pages/insights/OperationalInsights.tsxfrontend/src/pages/insights/OverviewTab.tsxfrontend/src/pages/insights/insightsRange.tsfrontend/src/styles/globals.cssfrontend/src/types/insights.ts
Comment on lines
+60
to
+84
| func (db *DB) GetKCAvgSessionMinutes(args *models.QueryContext, start, end *time.Time, facilityID *uint) (float64, error) { | ||
| type sessionRow struct { | ||
| RequestTS time.Time | ||
| StopTS time.Time | ||
| } | ||
| rows := make([]sessionRow, 0) | ||
| if err := db.kcActivityScope(args, start, end, facilityID). | ||
| Select("request_ts, stop_ts"). | ||
| Where("stop_ts IS NOT NULL"). | ||
| Find(&rows).Error; err != nil { | ||
| return 0, newGetRecordsDBError(err, "open_content_activities") | ||
| } | ||
| var totalMinutes float64 | ||
| var counted int | ||
| for _, row := range rows { | ||
| if row.StopTS.After(row.RequestTS) { | ||
| totalMinutes += row.StopTS.Sub(row.RequestTS).Minutes() | ||
| counted++ | ||
| } | ||
| } | ||
| if counted == 0 { | ||
| return 0, nil | ||
| } | ||
| return totalMinutes / float64(counted), nil | ||
| } |
There was a problem hiding this comment.
Compute average session duration in SQL instead of materializing all rows.
Lines [66]-[79] fetch every session row into memory and iterate in Go. For large ranges (especially all-time), this creates avoidable DB->API transfer and memory pressure on a request path.
Suggested direction
func (db *DB) GetKCAvgSessionMinutes(args *models.QueryContext, start, end *time.Time, facilityID *uint) (float64, error) {
- type sessionRow struct {
- RequestTS time.Time
- StopTS time.Time
- }
- rows := make([]sessionRow, 0)
- if err := db.kcActivityScope(args, start, end, facilityID).
- Select("request_ts, stop_ts").
- Where("stop_ts IS NOT NULL").
- Find(&rows).Error; err != nil {
- return 0, newGetRecordsDBError(err, "open_content_activities")
- }
- var totalMinutes float64
- var counted int
- for _, row := range rows {
- if row.StopTS.After(row.RequestTS) {
- totalMinutes += row.StopTS.Sub(row.RequestTS).Minutes()
- counted++
- }
- }
- if counted == 0 {
- return 0, nil
- }
- return totalMinutes / float64(counted), nil
+ type avgRow struct {
+ AvgMinutes float64
+ }
+ var out avgRow
+ tx := db.kcActivityScope(args, start, end, facilityID).
+ Where("stop_ts IS NOT NULL AND stop_ts > request_ts")
+
+ switch db.Dialector.Name() {
+ case "postgres":
+ tx = tx.Select("COALESCE(AVG(EXTRACT(EPOCH FROM (stop_ts - request_ts))/60.0), 0) AS avg_minutes")
+ case "sqlite":
+ tx = tx.Select("COALESCE(AVG((julianday(stop_ts) - julianday(request_ts)) * 24 * 60), 0) AS avg_minutes")
+ default:
+ return 0, nil
+ }
+
+ if err := tx.Scan(&out).Error; err != nil {
+ return 0, newGetRecordsDBError(err, "open_content_activities")
+ }
+ return out.AvgMinutes, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (db *DB) GetKCAvgSessionMinutes(args *models.QueryContext, start, end *time.Time, facilityID *uint) (float64, error) { | |
| type sessionRow struct { | |
| RequestTS time.Time | |
| StopTS time.Time | |
| } | |
| rows := make([]sessionRow, 0) | |
| if err := db.kcActivityScope(args, start, end, facilityID). | |
| Select("request_ts, stop_ts"). | |
| Where("stop_ts IS NOT NULL"). | |
| Find(&rows).Error; err != nil { | |
| return 0, newGetRecordsDBError(err, "open_content_activities") | |
| } | |
| var totalMinutes float64 | |
| var counted int | |
| for _, row := range rows { | |
| if row.StopTS.After(row.RequestTS) { | |
| totalMinutes += row.StopTS.Sub(row.RequestTS).Minutes() | |
| counted++ | |
| } | |
| } | |
| if counted == 0 { | |
| return 0, nil | |
| } | |
| return totalMinutes / float64(counted), nil | |
| } | |
| func (db *DB) GetKCAvgSessionMinutes(args *models.QueryContext, start, end *time.Time, facilityID *uint) (float64, error) { | |
| type avgRow struct { | |
| AvgMinutes float64 | |
| } | |
| var out avgRow | |
| tx := db.kcActivityScope(args, start, end, facilityID). | |
| Where("stop_ts IS NOT NULL AND stop_ts > request_ts") | |
| switch db.Dialector.Name() { | |
| case "postgres": | |
| tx = tx.Select("COALESCE(AVG(EXTRACT(EPOCH FROM (stop_ts - request_ts))/60.0), 0) AS avg_minutes") | |
| case "sqlite": | |
| tx = tx.Select("COALESCE(AVG((julianday(stop_ts) - julianday(request_ts)) * 24 * 60), 0) AS avg_minutes") | |
| default: | |
| return 0, nil | |
| } | |
| if err := tx.Scan(&out).Error; err != nil { | |
| return 0, newGetRecordsDBError(err, "open_content_activities") | |
| } | |
| return out.AvgMinutes, nil | |
| } |
🤖 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 `@backend/src/database/knowledge_center.go` around lines 60 - 84,
GetKCAvgSessionMinutes currently materializes all session rows and computes
durations in Go; instead push the aggregation to the DB by using
kcActivityScope(args, start, end, facilityID) to SELECT the average session
length SQL-side (e.g., AVG(EXTRACT(EPOCH FROM stop_ts - request_ts)/60) or
database-equivalent) with WHERE stop_ts IS NOT NULL AND stop_ts > request_ts,
then Scan the resulting avg value into a float64, return 0,nil if NULL, and
preserve error wrapping via newGetRecordsDBError on query failure; update
GetKCAvgSessionMinutes to use that single aggregate query rather than loading
rows and iterating.
Comment on lines
+112
to
+117
| func (db *DB) GetKCLibraryViewsByCategory(args *models.QueryContext, start, end *time.Time, facilityID *uint) ([]models.CategoryViews, error) { | ||
| views := make([]models.CategoryViews, 0) | ||
| tx := db.WithContext(args.Ctx).Table("open_content_activities oca"). | ||
| Select("t.name as category, count(oca.id) as views"). | ||
| Joins("JOIN open_content_tags oct ON oct.content_id = oca.content_id AND oct.open_content_provider_id = oca.open_content_provider_id"). | ||
| Joins("JOIN tags t ON t.id = oct.tag_id") |
There was a problem hiding this comment.
Filter category aggregation through libraries to avoid cross-content miscounts.
Line [116] joins tags directly from open_content_activities. If IDs overlap across content tables for the same provider, non-library activity can be counted in “library views by category”. Join through libraries first to enforce content type.
Proposed fix
tx := db.WithContext(args.Ctx).Table("open_content_activities oca").
Select("t.name as category, count(oca.id) as views").
- Joins("JOIN open_content_tags oct ON oct.content_id = oca.content_id AND oct.open_content_provider_id = oca.open_content_provider_id").
+ Joins("JOIN libraries l ON l.id = oca.content_id AND l.open_content_provider_id = oca.open_content_provider_id AND l.deleted_at IS NULL").
+ Joins("JOIN open_content_tags oct ON oct.content_id = l.id AND oct.open_content_provider_id = l.open_content_provider_id").
Joins("JOIN tags t ON t.id = oct.tag_id")🤖 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 `@backend/src/database/knowledge_center.go` around lines 112 - 117, In
GetKCLibraryViewsByCategory replace the direct join from open_content_activities
to tags with a join through the libraries table so only library content is
counted: first join libraries (e.g., JOIN libraries l ON l.id = oca.content_id
AND l.open_content_provider_id = oca.open_content_provider_id), then join
open_content_tags using the library id (oct.content_id = l.id AND
oct.open_content_provider_id = l.open_content_provider_id), and finally join
tags on oct.tag_id = t.id; update any selected/filtered references to use the
library join to prevent cross-content ID overlap from inflating category view
counts.
Comment on lines
287
to
+292
| facilityIdInt, err := strconv.Atoi(facility) | ||
| if err != nil { | ||
| return newInvalidIdServiceError(err, "facility") | ||
| return nil, newInvalidIdServiceError(err, "facility") | ||
| } | ||
| ref := uint(facilityIdInt) | ||
| facilityID = &ref | ||
| return &ref, nil |
There was a problem hiding this comment.
Reject non-positive facility values before casting to uint.
Line [291] casts parsed values directly; -1 or 0 should be invalid, but currently become invalid uint IDs (including wraparound for negatives).
Proposed fix
facilityIdInt, err := strconv.Atoi(facility)
if err != nil {
return nil, newInvalidIdServiceError(err, "facility")
}
+ if facilityIdInt <= 0 {
+ return nil, newInvalidIdServiceError(fmt.Errorf("facility must be a positive integer"), "facility")
+ }
ref := uint(facilityIdInt)
return &ref, nil🤖 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 `@backend/src/handlers/dashboard.go` around lines 287 - 292, The parsed
facility ID is cast to uint without rejecting non-positive values, which turns 0
or negatives into invalid/wrapped uints; in the code around
strconv.Atoi(facility) (variables facilityIdInt and ref), add a check after
parsing: if facilityIdInt <= 0 { return nil,
newInvalidIdServiceError(fmt.Errorf("invalid facility id: %d", facilityIdInt),
"facility") } (or use an appropriate error value) so you reject zero and
negative IDs before converting to uint and returning &ref.
Comment on lines
+37
to
+49
| export function priorParams(params: InsightsDateParams): InsightsDateParams { | ||
| const start = new Date(`${params.start_date}T00:00:00`); | ||
| const end = new Date(`${params.end_date}T00:00:00`); | ||
| const durationDays = | ||
| Math.round((end.getTime() - start.getTime()) / 86400000) + 1; | ||
| const priorEnd = new Date(start); | ||
| priorEnd.setDate(start.getDate() - 1); | ||
| const priorStart = new Date(start); | ||
| priorStart.setDate(start.getDate() - durationDays); | ||
| return { | ||
| start_date: formatDate(priorStart), | ||
| end_date: formatDate(priorEnd) | ||
| }; |
There was a problem hiding this comment.
Validate and normalize prior-range inputs before computing dates.
priorParams assumes valid, ordered dates. With empty/invalid custom values (or start_date > end_date), this returns malformed dates and propagates invalid query params downstream.
Proposed fix
export function priorParams(params: InsightsDateParams): InsightsDateParams {
const start = new Date(`${params.start_date}T00:00:00`);
const end = new Date(`${params.end_date}T00:00:00`);
+ if (Number.isNaN(start.getTime()) || Number.isNaN(end.getTime())) {
+ return params;
+ }
+ if (start > end) {
+ return params;
+ }
const durationDays =
Math.round((end.getTime() - start.getTime()) / 86400000) + 1;🤖 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 `@frontend/src/pages/insights/insightsRange.ts` around lines 37 - 49,
priorParams currently assumes params.start_date and params.end_date are valid
and ordered; update priorParams to validate and normalize inputs before
computing the prior range: parse params.start_date and params.end_date into Date
objects and if either is missing/invalid return the original params (or a safe
default), if start > end swap them (or set start = end) so the range is
non-negative, compute durationDays as Math.max(1, Math.round((end.getTime() -
start.getTime()) / 86400000) + 1), then compute priorStart/priorEnd from the
normalized start and duration, and finally return formatted dates via
formatDate; reference function priorParams, type InsightsDateParams and helper
formatDate to locate changes.
Comment on lines
+55
to
+57
| rows.map((row) => ( | ||
| <TableRow key={row.title}> | ||
| <TableCell className="text-muted-foreground"> |
There was a problem hiding this comment.
Use a collision-safe row key for mapped table rows.
Using only row.title as key is unsafe when duplicate titles exist; React can reuse the wrong row state/render.
Proposed fix
- rows.map((row) => (
- <TableRow key={row.title}>
+ rows.map((row, idx) => (
+ <TableRow key={`${row.title}-${row.visits}-${idx}`}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| rows.map((row) => ( | |
| <TableRow key={row.title}> | |
| <TableCell className="text-muted-foreground"> | |
| rows.map((row, idx) => ( | |
| <TableRow key={`${row.title}-${row.visits}-${idx}`}> | |
| <TableCell className="text-muted-foreground"> |
🤖 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 `@frontend/src/pages/insights/KCContentTable.tsx` around lines 55 - 57, The
TableRow key uses only row.title which can collide; update the rows.map key to a
collision-safe stable identifier by using a unique property on the row (e.g.,
row.id) or, if no unique id exists, derive a stable composite key (e.g., combine
a unique field and the map index) instead of plain title; change the TableRow
key in the rows.map callback to reference that unique identifier so React can
reliably track rows.
Comment on lines
+60
to
+62
| function changePct(current: number, prior: number): number { | ||
| return prior > 0 ? Math.round(((current - prior) / prior) * 100) : 0; | ||
| } |
There was a problem hiding this comment.
Handle zero prior baseline explicitly in percent-change calculation.
Returning 0 when prior === 0 masks real increases and produces misleading deltas for “New Residents Added”.
Proposed fix
function changePct(current: number, prior: number): number {
- return prior > 0 ? Math.round(((current - prior) / prior) * 100) : 0;
+ if (prior === 0) {
+ return current === 0 ? 0 : 100;
+ }
+ return Math.round(((current - prior) / prior) * 100);
}Also applies to: 174-177, 244-244
🤖 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 `@frontend/src/pages/insights/OverviewTab.tsx` around lines 60 - 62, The
current changePct function masks real increases by returning 0 when prior === 0;
update changePct to explicitly handle three cases: if prior === 0 and current
=== 0 return 0, if prior === 0 and current > 0 return null (or another sentinel)
to indicate an undefined/infinite percent-change, otherwise compute and return
the rounded percent as before; then update consumers of changePct (the usages
around the locations referenced in the review) to handle the null sentinel for
display/formatting (e.g., show "—" or "New" instead of 0) so increases from a
zero baseline are not misrepresented.
corypride
approved these changes
Jun 10, 2026
corypride
left a comment
corypride
left a comment
Contributor
There was a problem hiding this comment.
Approving. Pulled the branch and tested the Knowledge Center tab end-to-end (seeded activity/tag/video data, then drove the UI + API as system_admin, facility_admin, and a student).
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.
Pre-Submission PR Checklist
NOTE: Per @CK-7vn's PR
Description of the change
This is the second PR for Operational Insights. This PR pulls more indepth charts and information to the knowledge center side of Operational Insights as well as adds prior-period deltas that compare each metric in the selected time range against the immediately preceeding window of equal length.
Screenshot(s)
Additional context
If any core features or components were removed with this PR, please note them here so that they can be added to the wiki (see Deprecated features and Components).