CNTRLPLANE-3043: Add jira-agent performance dashboard - #8033
Draft
bryan-cox wants to merge 18 commits into
Draft
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references CNTRLPLANE-3043 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
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:
📝 WalkthroughWalkthroughThis pull request adds a complete JIRA Agent Dashboard: a Go-based scraper and orchestrator, GitHub App auth, GCS reader, PR complexity analyzer, SQLite-backed store with schema and migrations, REST API server, API handlers/responses, a static web frontend (HTML/CSS/JS), Containerfile and Makefile, OpenShift/Kubernetes kustomize manifests (Namespace, PVC, Deployment with oauth-proxy, Route, CronJobs, NetworkPolicies, RBAC, secrets example), and extensive unit and integration tests. Sequence Diagram(s)sequenceDiagram
participant Scraper as Scraper Pod
participant GCS as Google Cloud Storage
participant GitHub as GitHub API
participant Complexity as Complexity Analyzer
participant DB as SQLite DB (PVC)
Scraper->>GCS: List builds
GCS-->>Scraper: Build IDs
Scraper->>GCS: Read build-log.txt / timestamps
GCS-->>Scraper: Build log + timestamps
Scraper->>DB: Insert JobRun, Issue, PhaseMetric
DB-->>Scraper: ACK
Scraper->>GitHub: GetPR(owner,repo,number)
GitHub-->>Scraper: PR metadata & diff stats
Scraper->>GitHub: GetPRReviewComments()
GitHub-->>Scraper: Review & issue comments
Scraper->>DB: Upsert PRComplexity, Insert ReviewComments
DB-->>Scraper: ACK
Scraper->>Complexity: AnalyzePR (clone & run tools)
Complexity-->>Scraper: Complexity deltas
Scraper->>DB: Update complexity deltas
sequenceDiagram
participant Browser as Browser/Client
participant OAuth as OAuth Proxy
participant API as Dashboard API Server
participant Store as SQLite DB (PVC)
Browser->>OAuth: GET / (or /api/...)
OAuth->>OAuth: Validate token
OAuth->>API: Forward request
API->>Store: Query issues / trends / comments
Store-->>API: Data rows
API-->>OAuth: JSON response
OAuth-->>Browser: Authenticated response
Browser->>Browser: Render charts, tables, pages
Browser->>API: PATCH /api/comments/{id} (edit)
API->>Store: Update classification (mark human_override)
Store-->>API: Updated row
API-->>Browser: Return updated comment
sequenceDiagram
participant User as User (Browser)
participant Frontend as Frontend JS
participant API as API Server
participant DB as SQLite DB
User->>Frontend: Open issue detail
Frontend->>API: GET /api/issues/{id}
API->>DB: Fetch issue, phases, comments, complexity
DB-->>API: Issue detail JSON
API-->>Frontend: Issue + phases + comments
Frontend->>Frontend: Render phase chart & comments
User->>Frontend: Edit comment classification
Frontend->>API: PATCH /api/comments/{id}
API->>DB: Update comment classification (human_override=true)
DB-->>API: Updated comment
API-->>Frontend: Updated comment detail
Frontend->>Frontend: Update UI
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
bryan-cox
force-pushed
the
CNTRLPLANE-3043
branch
from
March 23, 2026 17:30
3877bc7 to
98d7ff4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8033 +/- ##
=======================================
Coverage 44.50% 44.51%
=======================================
Files 774 774
Lines 96980 96997 +17
=======================================
+ Hits 43164 43179 +15
- Misses 50828 50830 +2
Partials 2988 2988 see 2 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
bryan-cox
force-pushed
the
CNTRLPLANE-3043
branch
from
April 1, 2026 19:23
e36ed13 to
b4e547f
Compare
|
@bryan-cox: This pull request references CNTRLPLANE-3043 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
jira-agent-dashboard/internal/db/models.go-53-54 (1)
53-54:⚠️ Potential issue | 🟡 Minor
Topicvalue documentation is already out of sync.Line [54] documents only five topic values, but the frontend already supports additional ones (e.g.,
architecture_design,security,ci,approval,process,unclassified). Please update this comment or move allowed values to shared constants to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/db/models.go` around lines 53 - 54, The inline comment for the Topic field is out of date; update the comment on the Topic field in models.go to list the current allowed topic values (including architecture_design, security, ci, approval, process, unclassified, etc.) or—preferably—extract those allowed topic strings into a shared constant set (e.g., a package-level slice or const block like Topic* constants) and reference that constant from both backend and frontend; change the Topic field comment to mention the shared constants (or canonical source) so the doc won’t drift.jira-agent-dashboard/web/css/style.css-173-175 (1)
173-175:⚠️ Potential issue | 🟡 MinorMobile layout can retain an unintended 48px content offset when nav is collapsed.
Add a mobile-specific override for the collapsed sibling selector.
📱 Suggested CSS patch
`@media` (max-width: 768px) { nav { width: 100%; height: auto; position: relative; } main { margin-left: 0; } + + nav.collapsed ~ main { + margin-left: 0; + }Also applies to: 909-918
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/web/css/style.css` around lines 173 - 175, The collapsed nav sibling rule "nav.collapsed ~ main { margin-left: 48px; }" causes a persistent 48px offset on small screens; add a mobile-specific override (using an appropriate media query for your mobile breakpoint) that resets the margin-left to 0 for "nav.collapsed ~ main" so collapsed navigation does not shift content on phones, and apply the same override to the duplicate collapsed-nav rules referenced around lines 909-918 (the same selector occurrences) to ensure consistent mobile behavior.jira-agent-dashboard/web/issue.html-14-16 (1)
14-16:⚠️ Potential issue | 🟡 MinorAdd active nav state for issue-detail context.
Lines 14-16 don’t mark the current section as active, so page context is less clear for users.
🎯 Suggested fix
- <li><a href="issues.html">Issues</a></li> + <li><a href="issues.html" class="active">Issues</a></li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/web/issue.html` around lines 14 - 16, On the issue-detail page (issue.html) mark the Issues nav item as active by changing the <li><a href="issues.html">Issues</a></li> entry to include the active state—add class="active" to the <li> and aria-current="page" to the <a> (e.g., <li class="active"><a href="issues.html" aria-current="page">Issues</a></li>); if nav is rendered via a template, set the active flag for the "Issues" item when rendering issue.html so the same attributes are injected dynamically.jira-agent-dashboard/web/issues.html-62-63 (1)
62-63:⚠️ Potential issue | 🟡 MinorFix the loading-row column span.
The table defines 13 columns, but Line 63 spans 12, so the loading/empty state will render misaligned once the action column is present.
🩹 Proposed fix
- <tr><td colspan="12" style="text-align:center;">Loading...</td></tr> + <tr><td colspan="13" style="text-align:center;">Loading...</td></tr>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/web/issues.html` around lines 62 - 63, The loading/empty placeholder row under the tbody with id="issues-tbody" uses colspan="12" but the table actually defines 13 columns; update the loading/empty <tr><td ...> cell to use colspan="13" (the row rendering the "Loading..." state) so it aligns correctly when the action column is present.jira-agent-dashboard/internal/api/handlers.go-59-60 (1)
59-60:⚠️ Potential issue | 🟡 MinorDuplicate assignment: both
AvgDurationMsandAvgMergeDurationuse the same source.Both fields are set to
t.AvgDurationMs. If these fields represent different metrics, one assignment may be incorrect. If they're aliases for backward compatibility, consider adding a comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/api/handlers.go` around lines 59 - 60, The two fields AvgDurationMs and AvgMergeDuration are both assigned from t.AvgDurationMs; update the assignment in the struct literal so AvgMergeDuration uses the correct source (e.g., t.AvgMergeDuration) if it represents a different metric, or add a clarifying comment next to the AvgMergeDuration line indicating it is intentionally an alias of AvgDurationMs for backward compatibility; locate the assignment using the symbols AvgDurationMs, AvgMergeDuration and the variable t to make the change.jira-agent-dashboard/README.md-230-231 (1)
230-231:⚠️ Potential issue | 🟡 MinorDocumentation lists incomplete topic values.
The
allowedTopicsmap ininternal/api/handlers.go(lines 23-35) includesarchitecture_designandsecurity, but these are missing from the README documentation.📝 Suggested fix
-**Topic:** `style`, `logic_bug`, `test_gap`, `api_design`, `documentation`, `ci`, `approval`, `process`, `unclassified` +**Topic:** `style`, `logic_bug`, `test_gap`, `api_design`, `architecture_design`, `security`, `documentation`, `ci`, `approval`, `process`, `unclassified`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/README.md` around lines 230 - 231, The README’s Topic list is missing values declared in the code; update the README documentation to include the `architecture_design` and `security` topics that are present in the allowedTopics map in internal/api/handlers.go. Open the README section that enumerates Topic values and add those two entries with the same naming/casing used by allowedTopics so the docs match the code (ensure any examples or table rows referencing topics are updated accordingly).jira-agent-dashboard/web/js/issues.js-33-33 (1)
33-33:⚠️ Potential issue | 🟡 MinorFix the empty-state colspan.
jira-agent-dashboard/web/issues.htmldefines 13 headers, including the trailing action column, so this row should span 13 cells or the empty state stays misaligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/web/js/issues.js` at line 33, The empty-state row inserted via tbody.innerHTML currently uses colspan="12" which misaligns the empty message because the table has 13 headers (including the action column); update the HTML string in the assignment to tbody.innerHTML (the line setting '<tr><td colspan="12"...') to use colspan="13" so the placeholder spans all columns and aligns with the table headers.jira-agent-dashboard/web/js/issues.js-77-79 (1)
77-79:⚠️ Potential issue | 🟡 MinorFirst click on a new column sorts descending.
For a brand-new column,
currentDirectionstarts as'asc'and is immediately flipped to'desc', so the initial click does the opposite of the usual ascending-first behavior.Suggested fix
function sortTable(column, getValue) { - const currentDirection = sortColumn === column ? sortDirection : 'asc'; - const newDirection = currentDirection === 'asc' ? 'desc' : 'asc'; + const newDirection = + sortColumn === column && sortDirection === 'asc' ? 'desc' : 'asc';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/web/js/issues.js` around lines 77 - 79, The click handler flips the computed currentDirection immediately, so when sortColumn !== column you default currentDirection to 'asc' and then flip it to 'desc' on first click; change the logic so a new column starts sorting ascending: compute newDirection based on whether the clicked column matches sortColumn — if it does, flip sortDirection (sortDirection === 'asc' ? 'desc' : 'asc'), otherwise set newDirection to 'asc'; update the code that sets currentDirection/newDirection (referencing sortColumn, sortDirection, currentDirection, newDirection) accordingly.
🧹 Nitpick comments (8)
jira-agent-dashboard/deploy/rbac.yaml (1)
4-4: Prefer a globally uniqueClusterRoleBindingname.Because this is cluster-scoped, a generic name like
dashboard-auth-delegatorcan collide with other installs. Consider prefixing with app/namespace identity (e.g.,jira-agent-dashboard-auth-delegator).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/deploy/rbac.yaml` at line 4, The ClusterRoleBinding currently uses a generic cluster-scoped name "dashboard-auth-delegator"; change metadata.name to a globally unique name (e.g., "jira-agent-dashboard-auth-delegator") to avoid collisions, and update any references to that binding elsewhere in manifests; locate the ClusterRoleBinding resource (metadata.name) in rbac.yaml and rename it consistently across the deployment/helm templates or other YAMLs that refer to "dashboard-auth-delegator".jira-agent-dashboard/internal/scraper/gcs_test.go (1)
97-99: Use epsilon-based float assertions in parser tests.Direct
!=checks against floating-point values can make tests brittle. The test suite has multiple exact comparisons (lines 97, 127, 142) that should use tolerance-based assertions instead.Replace with epsilon comparisons:
Suggested fix
+import "math" @@ - if p1.TotalCostUSD != 2.44697175 { + if math.Abs(p1.TotalCostUSD-2.44697175) > 1e-9 { t.Errorf("Phase 1 cost = %f, want 2.44697175", p1.TotalCostUSD) } @@ - if p2.TotalCostUSD != 2.0867235 { + if math.Abs(p2.TotalCostUSD-2.0867235) > 1e-9 { t.Errorf("Phase 2 cost = %f, want 2.0867235", p2.TotalCostUSD) } @@ - if p4.TotalCostUSD != 0.17865225 { + if math.Abs(p4.TotalCostUSD-0.17865225) > 1e-9 { t.Errorf("Phase 4 cost = %f, want 0.17865225", p4.TotalCostUSD) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/scraper/gcs_test.go` around lines 97 - 99, The test currently uses exact float equality for p1.TotalCostUSD (and other checks at the same file) which is brittle; update the assertions to use an epsilon comparison instead—compute math.Abs(p1.TotalCostUSD - expected) and fail the test (using t.Errorf or t.Fatalf) if that difference is greater than a small epsilon (e.g. 1e-6); apply the same change for the other exact float checks referenced (lines comparing other phase costs) so all float assertions use the tolerance-based pattern; locate the checks by searching for TotalCostUSD and the exact numeric literals in gcs_test.go.jira-agent-dashboard/internal/integration_test.go (1)
135-155: Add end-to-end coverage for/api/comments/summary.The new comments page is driven by that endpoint, but this flow only exercises
/api/comments/{issueID}. That leaves the main comments dashboard path without integration coverage.🧪 Example addition
+ t.Run("GET /api/comments/summary", func(t *testing.T) { + resp, err := http.Get(ts.URL + "/api/comments/summary?from=2024-01-01&to=2024-12-31") + if err != nil { + t.Fatalf("failed to GET /api/comments/summary: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Errorf("expected status 200, got %d", resp.StatusCode) + } + + var comments []api.CommentSummary + if err := json.NewDecoder(resp.Body).Decode(&comments); err != nil { + t.Fatalf("failed to decode comments summary response: %v", err) + } + + if len(comments) == 0 { + t.Error("expected at least 1 comment summary, got 0") + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/integration_test.go` around lines 135 - 155, Add an integration subtest alongside the existing "GET /api/comments/{issueID}" case that exercises the /api/comments/summary endpoint: perform an HTTP GET against ts.URL + "/api/comments/summary", check resp.StatusCode == http.StatusOK, decode the response into the appropriate slice type (e.g., []api.CommentSummary) and assert expected length/fields (e.g., totals or top-level counts) so the comments dashboard flow is covered; reuse the same test harness variables (t.Run, ts.URL, http.Get, resp.Body.Close) and mirror the error handling/decoding pattern used for api.CommentDetail to keep consistency.jira-agent-dashboard/internal/api/handlers.go (2)
343-357: Update precedes existence check, causing silent no-op on invalid ID.
UpdateCommentClassificationis called before verifying the comment exists. If the ID is invalid, the UPDATE affects zero rows silently, and only the subsequent fetch returns 404. Consider checking existence first or havingUpdateCommentClassificationreturn the affected row count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/api/handlers.go` around lines 343 - 357, The handler calls UpdateCommentClassification before confirming the comment exists, so updates on an invalid id can silently do nothing; either verify existence first by calling s.store.GetReviewCommentByID(id) and returning 404 if missing before calling s.store.UpdateCommentClassification, or change s.store.UpdateCommentClassification to return the number of affected rows (or an error when 0 rows are affected) and check that result in the handler to return 404; update the handler to use GetReviewCommentByID or the new affected-row result to ensure a proper 404 for invalid IDs.
82-95: N+1 query pattern may cause performance issues.For each issue, three additional database queries are executed (comments, complexity, phases). With 100 issues, this results in 301 queries instead of potentially 4 with batch queries or JOINs.
Consider adding batch methods like
GetReviewCommentsByIssueIDs(ids []int64)to reduce round trips, or document that this endpoint is intended for small date ranges only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/api/handlers.go` around lines 82 - 95, The loop over issues in the handler causes N+1 DB queries because for each issue you call s.store.GetReviewCommentsByIssueID, s.store.GetPRComplexityByIssueID, and s.store.GetPhaseMetricsByIssueID; add batch store methods (e.g. GetReviewCommentsByIssueIDs(ids []int64), GetPRComplexityByIssueIDs(ids []int64), GetPhaseMetricsByIssueIDs(ids []int64)) that return results keyed by issue ID, then change the handler to collect all issue IDs from the issues slice, call these batch methods once, build maps from issue ID to comments/complexity/phases, and populate the result []IssueSummary from those maps instead of per-issue DB calls.jira-agent-dashboard/internal/scraper/complexity.go (1)
123-169: Duplicated regex and parsing logic for gocyclo and gocognit.
gocycloReandgocognitReare identical, andParseGocycloOutput/ParseGocognitOutputhave the same logic. Consider extracting a shared helper:func parseAverageComplexity(output, toolName string) (float64, error)However, keeping them separate is acceptable if you anticipate the output formats diverging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/scraper/complexity.go` around lines 123 - 169, ParseGocycloOutput and ParseGocognitOutput duplicate the same regex and parsing logic (gocycloRe, gocognitRe and both Parse* functions); refactor by extracting a shared helper (suggested name parseAverageComplexity(output, toolName string) (float64, error)) that takes the output and tool name, runs the TrimSpace check, applies the regex, parses the float and formats errors, then have ParseGocycloOutput and ParseGocognitOutput call this helper (or replace them entirely) to remove duplicated regex and parsing code while preserving existing error messages and behavior.jira-agent-dashboard/cmd/scraper/main.go (1)
79-81: Redundantos.Exit(0)call.When
main()returns normally, the process exits with code 0. The explicitos.Exit(0)is unnecessary and can be removed.♻️ Suggested fix
log.Printf("Scraper completed successfully (step=%s).", *step) - os.Exit(0) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/cmd/scraper/main.go` around lines 79 - 81, Remove the redundant os.Exit(0) at the end of main(): keep the existing log.Printf("Scraper completed successfully (step=%s).", *step) and let main() return normally instead of calling os.Exit(0); this removes the unnecessary explicit process exit while preserving the success log (look for the log.Printf call and the os.Exit invocation in main()).jira-agent-dashboard/internal/db/store.go (1)
21-40: Centralize the noise-comment policy.The filter rules here are duplicated in
jira-agent-dashboard/internal/scraper/github.go::IsNoiseComment. The next rule change will drift insert-time filtering from query-time filtering and skew counts/classification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/db/store.go` around lines 21 - 40, The SQL comment filtering logic in commentFilterSQL is duplicated with IsNoiseComment in scraper/github.go; extract the canonical noise-comment policy (bot author allowlist and body patterns: bot suffixes, '-robot', known bots like 'cwbotbot', slash-commands, "No actionable comments were generated", "Skipped: comment is from another GitHub bot", "<!-- walkthrough_start -->", "skip review by coderabbit.ai", trimming rules) into a single shared helper in this package (e.g., a exported ValidateNoiseComment/NoiseCommentPatterns or BuildCommentFilterSQL helper) and update both commentFilterSQL and IsNoiseComment to reference that shared policy so query-time and insert-time filtering use the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jira-agent-dashboard/cmd/dashboard/main.go`:
- Around line 25-32: The SQLite connection is opened with mode=rw which prevents
creating a new DB file on first boot; update the sql.Open call that creates conn
(sql.Open("sqlite3", dbPath+"?mode=rw")) to use mode=rwc so the file will be
created if missing, leaving the rest of the logic (defer conn.Close() and the
db.InitSchema(conn) bootstrap) unchanged.
In `@jira-agent-dashboard/deploy/dashboard-deployment.yaml`:
- Around line 134-136: The Service is exposing the upstream app port 8080
(port/name "http") which must be removed; update the Service spec in
dashboard-deployment.yaml to delete the port entry that declares port: 8080 /
targetPort: 8080 / name: http so only the oauth-proxy HTTPS port (4443) remains
exported, and verify liveness/readiness probes (the container's probe settings)
still reference the containerPort directly and do not rely on the removed
Service port.
In `@jira-agent-dashboard/deploy/scraper-cronjob.yaml`:
- Around line 49-52: The cronjob mounts the same persistentVolumeClaim named
"dashboard-db" (volumes -> persistentVolumeClaim -> claimName: dashboard-db)
which is declared as ReadWriteOnce in pvc.yaml and so cannot be attached to
multiple nodes; change the job to avoid using that RWO PVC concurrently: either
create and reference a separate PVC for the scrapers or change the PVC type to
ReadWriteMany (or use a non-clustered storage class) and update claimName
accordingly, or remove the volume mount from the cronjob and use
network/database access instead; update the cronjob's volumes/volumeMounts (and
the referenced claimName) to point to the new PVC or remove them, and ensure
jira-agent-dashboard/deploy/pvc.yaml is adjusted if switching to RWX.
- Around line 15-16: The current CronJob schedule change plus concurrencyPolicy:
Forbid does not prevent concurrent runs across different CronJobs (scraper-prow
vs scraper-github) and causes SQLite "database is locked" errors; replace the
fragile schedule-stagger approach with a cross-job mutex: add a lock acquisition
step (e.g., an initContainer or entrypoint wrapper used by both CronJobs that
obtains a global lock) before the main scraper runs, using either a Kubernetes
Lease (client-go leader election) or an atomic ConfigMap/Secret create-or-update
loop or a flock on a shared PVC; update both CronJob manifests to include the
same lock name and ensure the initContainer/entrypoint blocks until it holds the
lock, then proceeds to the main container, and keep or remove concurrencyPolicy
as desired (it only affects same-CronJob concurrency).
In `@jira-agent-dashboard/internal/api/handlers_test.go`:
- Around line 20-33: The test setup opens an in-memory SQLite DB but doesn't
limit DB pooling, causing multiple in-memory instances; after sql.Open and
before InitSchema (i.e., where conn is created in the setup function), call
conn.SetMaxOpenConns(1) to force a single connection so InitSchema(conn),
db.NewStore(conn) and handlers invoked by httptest.NewServer share the same
in-memory database.
In `@jira-agent-dashboard/internal/api/handlers.go`:
- Line 243: The CreatedAt field is inconsistent: in handleGetIssues it uses
issue.StartedAt but in handleGetIssueDetail it uses issue.MergedAt (likely
copy-paste). Update the CreatedAt assignment in handleGetIssueDetail to call
formatOptionalTime(issue.StartedAt) instead of
formatOptionalTime(issue.MergedAt) so both handlers consistently use StartedAt;
locate the CreatedAt line in handleGetIssueDetail and replace the referenced
field accordingly.
In `@jira-agent-dashboard/internal/db/schema.go`:
- Around line 87-90: The migration loop currently swallows every error from
db.Exec(m) (the migrations slice loop), which can hide partial failures; change
it to capture the returned error from db.Exec and only ignore the specific
"column already exists" / duplicate-column SQLite error but treat all other
errors as fatal — return or log the error and halt startup. Locate the loop
iterating over migrations and replace the blind db.Exec(m) call with error
handling that checks the error value (or its message) and only suppresses the
known benign SQLite error, otherwise propagate or exit with the error.
In `@jira-agent-dashboard/internal/db/store.go`:
- Around line 459-472: The current InsertOrUpdatePRComplexity upsert overwrites
stored complexity deltas with zero values when callers like refreshGitHub() or
backfillPRStats() only supply lines/files stats; update the
InsertOrUpdatePRComplexity implementation so it does not clobber existing
complexity fields: either (A) change the ON CONFLICT DO UPDATE clause to only
set lines_added, lines_deleted and files_changed (leave
cyclomatic_complexity_delta and cognitive_complexity_delta untouched), or (B)
use SQL expressions such as cyclomatic_complexity_delta =
COALESCE(excluded.cyclomatic_complexity_delta,
pr_complexity.cyclomatic_complexity_delta) and cognitive_complexity_delta =
COALESCE(excluded.cognitive_complexity_delta,
pr_complexity.cognitive_complexity_delta) so existing deltas are preserved when
the incoming PRComplexity has zero/NULL values; modify the SQL in
InsertOrUpdatePRComplexity accordingly and ensure callers that actually compute
complexity write the deltas explicitly.
In `@jira-agent-dashboard/internal/scraper/gcs.go`:
- Around line 249-270: The current json.Unmarshal of jsonBuf into PhaseTokens
silently ignores errors, causing phase metrics to be lost; update the code
around json.Unmarshal (the block using PhaseTokens, json.Unmarshal, phaseNum,
phaseNames, and result.Phases) to detect and handle errors: when json.Unmarshal
fails, log a warning or error including the phaseNum and the JSON snippet (or
error text) so the failure is visible, and still append a fallback PhaseTokens
entry (with PhaseNumber set to phaseNum and PhaseName resolved via phaseNames or
the existing switch) to result.Phases so the phase is not dropped; ensure the
logged message includes the unmarshalling error and enough context to debug.
- Around line 60-66: NewHTTPGCSClient currently falls back to http.DefaultClient
when passed nil, which has no timeout; change NewHTTPGCSClient to create and use
a dedicated *http.Client with a sensible Timeout instead of http.DefaultClient
when client == nil (update the NewHTTPGCSClient function and the
HTTPGCSClient.Client field initialization), e.g. instantiate
&http.Client{Timeout: <reasonable duration>} so GCS requests cannot hang
indefinitely; ensure any callers (e.g., where nil is passed) continue to work
with the new default.
In `@jira-agent-dashboard/internal/scraper/github.go`:
- Around line 137-146: GetPRReviewComments currently calls fetchComments for
pulls/{n}/comments and issues/{n}/comments but omits the pulls/{n}/reviews
endpoint that contains review bodies; update GetPRReviewComments to also call
fetchComments (or a new fetchReviews helper) for
fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=100") and merge those
results into the returned []CommentInfo (deduplicate if necessary), ensuring any
errors from that fetch are wrapped like the existing error handling.
In `@jira-agent-dashboard/internal/scraper/orchestrator_test.go`:
- Around line 78-86: The in-memory SQLite is creating multiple connections so
later calls on the pooled *db.Store miss prior data; after creating the DB with
sql.Open("sqlite3", ":memory:") set the pool to a single connection by calling
sqlDB.SetMaxOpenConns(1) immediately after sql.Open (before db.InitSchema and
before returning db.NewStore), ensuring sqlDB is used consistently for
db.InitSchema and subsequent operations.
In `@jira-agent-dashboard/internal/scraper/orchestrator.go`:
- Around line 137-150: The code currently seeds startedAt and finishedAt with
time.Now(), causing missing Prow metadata to be stamped with scrape time; change
initialization to use the zero time (e.g., declare var startedAt, finishedAt
time.Time) and only set them when o.gcs.ReadBuildFile(...) successfully
unmarshals prowTimestamp and ts.Timestamp > 0, leaving the zero value otherwise;
update any downstream logic that consumes startedAt/finishedAt (e.g., date
filtering or insertion into job_runs) to handle zero time appropriately rather
than assuming a non-zero time.
In `@jira-agent-dashboard/Makefile`:
- Around line 8-12: The dashboard and scraper targets call "$(GO) build -o
$(BINARY_DIR)/..." but don't ensure $(BINARY_DIR) exists, causing failures on
fresh checkouts; update the Makefile so the dashboard and scraper targets create
the directory first (e.g., run "mkdir -p $(BINARY_DIR)" before the "$(GO) build
-o ..." invocation) or add a phony/implicit target for $(BINARY_DIR) and make
dashboard and scraper depend on it so the directory is created before building.
In `@jira-agent-dashboard/web/comments.html`:
- Around line 113-115: The page currently loads Chart.js from the CDN via the
script tag "https://cdn.jsdelivr.net/npm/chart.js", which breaks in
offline/restricted environments and causes comments.js to fail; fix by vendoring
a local Chart.js build (e.g., vendor/chart.min.js) into the repo, update the
HTML to reference that local path instead of the CDN URL, and ensure the
vendored file is committed and referenced similarly in the other pages that use
Chart (replace the same CDN script tag found in the other HTML pages). Ensure
comments.js (and any init code that references the global Chart symbol)
continues to work with the vendored build.
In `@jira-agent-dashboard/web/css/style.css`:
- Around line 90-93: The .nav-toggle hover rule (and other hover-only rules)
lack keyboard focus styles; add matching :focus and :focus-visible rules for
.nav-toggle and each interactive selector referenced (the hover-only blocks at
lines noted) so keyboard users see a visible indicator—update selectors like
.nav-toggle:hover to include .nav-toggle:focus, .nav-toggle:focus-visible (and
replicate for the other hover-only classes) and define an accessible
outline/box-shadow or background change consistent with the hover state to
ensure parity between hover and keyboard focus.
In `@jira-agent-dashboard/web/js/comments.js`:
- Around line 238-240: prLink is built by string-concatenating comment.pr_url
using escapeHTML which doesn't prevent unsafe URL schemes; validate and sanitize
comment.pr_url before using it: parse comment.pr_url with the URL constructor
(or a regex) and allow only expected schemes (e.g., "https:"); if validation
passes, create the anchor element via DOM APIs (document.createElement('a')),
set its href to the validated URL, set target and rel attributes, and set its
textContent to the sanitized display string (e.g.,
comment.pr_url.replace('https://github.com/', '')); otherwise leave prLink
empty. Ensure you update the code that uses prLink (the variable constructed
where escapeHTML is currently used) to accept this DOM-created anchor or its
outerHTML only when the URL validation succeeds.
In `@jira-agent-dashboard/web/js/issue-detail.js`:
- Around line 29-39: The current header.innerHTML injects untrusted fields
(issueData.jira_url, issueData.jira_key, issueData.pr_url, issueData.pr_number)
directly into HTML causing stored XSS; replace the template-string assignment
with DOM construction using document.createElement for h2, a, div.meta and span
elements, set link href attributes only after validating/normalizing the URL
(and using target="_blank" with rel="noopener noreferrer"), and assign
textContent for jira_key, PR label and other text values; keep
formatDuration(issueData.merge_duration) and formatCost(issueData.total_cost)
but render their results as textContent into spans rather than interpolating
into innerHTML.
In `@jira-agent-dashboard/web/js/issues.js`:
- Around line 8-16: loadIssues can resolve out-of-order and overwrite newer
results; add a request guard (e.g., a monotonically incremented requestId or
per-call AbortController) so only the latest response updates issuesData and
calls updateResultsCount()/renderIssuesTable(). Specifically, introduce a
module-scoped currentRequestId (or currentAbortController), increment/create it
at the start of loadIssues, attach the id/abort token to the fetchAPI call, and
in the success path only assign to issuesData and call
updateResultsCount()/renderIssuesTable() if the id matches (or the request
wasn't aborted); also make sure the catch path ignores abort errors and only
shows showError for real failures.
---
Minor comments:
In `@jira-agent-dashboard/internal/api/handlers.go`:
- Around line 59-60: The two fields AvgDurationMs and AvgMergeDuration are both
assigned from t.AvgDurationMs; update the assignment in the struct literal so
AvgMergeDuration uses the correct source (e.g., t.AvgMergeDuration) if it
represents a different metric, or add a clarifying comment next to the
AvgMergeDuration line indicating it is intentionally an alias of AvgDurationMs
for backward compatibility; locate the assignment using the symbols
AvgDurationMs, AvgMergeDuration and the variable t to make the change.
In `@jira-agent-dashboard/internal/db/models.go`:
- Around line 53-54: The inline comment for the Topic field is out of date;
update the comment on the Topic field in models.go to list the current allowed
topic values (including architecture_design, security, ci, approval, process,
unclassified, etc.) or—preferably—extract those allowed topic strings into a
shared constant set (e.g., a package-level slice or const block like Topic*
constants) and reference that constant from both backend and frontend; change
the Topic field comment to mention the shared constants (or canonical source) so
the doc won’t drift.
In `@jira-agent-dashboard/README.md`:
- Around line 230-231: The README’s Topic list is missing values declared in the
code; update the README documentation to include the `architecture_design` and
`security` topics that are present in the allowedTopics map in
internal/api/handlers.go. Open the README section that enumerates Topic values
and add those two entries with the same naming/casing used by allowedTopics so
the docs match the code (ensure any examples or table rows referencing topics
are updated accordingly).
In `@jira-agent-dashboard/web/css/style.css`:
- Around line 173-175: The collapsed nav sibling rule "nav.collapsed ~ main {
margin-left: 48px; }" causes a persistent 48px offset on small screens; add a
mobile-specific override (using an appropriate media query for your mobile
breakpoint) that resets the margin-left to 0 for "nav.collapsed ~ main" so
collapsed navigation does not shift content on phones, and apply the same
override to the duplicate collapsed-nav rules referenced around lines 909-918
(the same selector occurrences) to ensure consistent mobile behavior.
In `@jira-agent-dashboard/web/issue.html`:
- Around line 14-16: On the issue-detail page (issue.html) mark the Issues nav
item as active by changing the <li><a href="issues.html">Issues</a></li> entry
to include the active state—add class="active" to the <li> and
aria-current="page" to the <a> (e.g., <li class="active"><a href="issues.html"
aria-current="page">Issues</a></li>); if nav is rendered via a template, set the
active flag for the "Issues" item when rendering issue.html so the same
attributes are injected dynamically.
In `@jira-agent-dashboard/web/issues.html`:
- Around line 62-63: The loading/empty placeholder row under the tbody with
id="issues-tbody" uses colspan="12" but the table actually defines 13 columns;
update the loading/empty <tr><td ...> cell to use colspan="13" (the row
rendering the "Loading..." state) so it aligns correctly when the action column
is present.
In `@jira-agent-dashboard/web/js/issues.js`:
- Line 33: The empty-state row inserted via tbody.innerHTML currently uses
colspan="12" which misaligns the empty message because the table has 13 headers
(including the action column); update the HTML string in the assignment to
tbody.innerHTML (the line setting '<tr><td colspan="12"...') to use colspan="13"
so the placeholder spans all columns and aligns with the table headers.
- Around line 77-79: The click handler flips the computed currentDirection
immediately, so when sortColumn !== column you default currentDirection to 'asc'
and then flip it to 'desc' on first click; change the logic so a new column
starts sorting ascending: compute newDirection based on whether the clicked
column matches sortColumn — if it does, flip sortDirection (sortDirection ===
'asc' ? 'desc' : 'asc'), otherwise set newDirection to 'asc'; update the code
that sets currentDirection/newDirection (referencing sortColumn, sortDirection,
currentDirection, newDirection) accordingly.
---
Nitpick comments:
In `@jira-agent-dashboard/cmd/scraper/main.go`:
- Around line 79-81: Remove the redundant os.Exit(0) at the end of main(): keep
the existing log.Printf("Scraper completed successfully (step=%s).", *step) and
let main() return normally instead of calling os.Exit(0); this removes the
unnecessary explicit process exit while preserving the success log (look for the
log.Printf call and the os.Exit invocation in main()).
In `@jira-agent-dashboard/deploy/rbac.yaml`:
- Line 4: The ClusterRoleBinding currently uses a generic cluster-scoped name
"dashboard-auth-delegator"; change metadata.name to a globally unique name
(e.g., "jira-agent-dashboard-auth-delegator") to avoid collisions, and update
any references to that binding elsewhere in manifests; locate the
ClusterRoleBinding resource (metadata.name) in rbac.yaml and rename it
consistently across the deployment/helm templates or other YAMLs that refer to
"dashboard-auth-delegator".
In `@jira-agent-dashboard/internal/api/handlers.go`:
- Around line 343-357: The handler calls UpdateCommentClassification before
confirming the comment exists, so updates on an invalid id can silently do
nothing; either verify existence first by calling
s.store.GetReviewCommentByID(id) and returning 404 if missing before calling
s.store.UpdateCommentClassification, or change
s.store.UpdateCommentClassification to return the number of affected rows (or an
error when 0 rows are affected) and check that result in the handler to return
404; update the handler to use GetReviewCommentByID or the new affected-row
result to ensure a proper 404 for invalid IDs.
- Around line 82-95: The loop over issues in the handler causes N+1 DB queries
because for each issue you call s.store.GetReviewCommentsByIssueID,
s.store.GetPRComplexityByIssueID, and s.store.GetPhaseMetricsByIssueID; add
batch store methods (e.g. GetReviewCommentsByIssueIDs(ids []int64),
GetPRComplexityByIssueIDs(ids []int64), GetPhaseMetricsByIssueIDs(ids []int64))
that return results keyed by issue ID, then change the handler to collect all
issue IDs from the issues slice, call these batch methods once, build maps from
issue ID to comments/complexity/phases, and populate the result []IssueSummary
from those maps instead of per-issue DB calls.
In `@jira-agent-dashboard/internal/db/store.go`:
- Around line 21-40: The SQL comment filtering logic in commentFilterSQL is
duplicated with IsNoiseComment in scraper/github.go; extract the canonical
noise-comment policy (bot author allowlist and body patterns: bot suffixes,
'-robot', known bots like 'cwbotbot', slash-commands, "No actionable comments
were generated", "Skipped: comment is from another GitHub bot", "<!--
walkthrough_start -->", "skip review by coderabbit.ai", trimming rules) into a
single shared helper in this package (e.g., a exported
ValidateNoiseComment/NoiseCommentPatterns or BuildCommentFilterSQL helper) and
update both commentFilterSQL and IsNoiseComment to reference that shared policy
so query-time and insert-time filtering use the same source of truth.
In `@jira-agent-dashboard/internal/integration_test.go`:
- Around line 135-155: Add an integration subtest alongside the existing "GET
/api/comments/{issueID}" case that exercises the /api/comments/summary endpoint:
perform an HTTP GET against ts.URL + "/api/comments/summary", check
resp.StatusCode == http.StatusOK, decode the response into the appropriate slice
type (e.g., []api.CommentSummary) and assert expected length/fields (e.g.,
totals or top-level counts) so the comments dashboard flow is covered; reuse the
same test harness variables (t.Run, ts.URL, http.Get, resp.Body.Close) and
mirror the error handling/decoding pattern used for api.CommentDetail to keep
consistency.
In `@jira-agent-dashboard/internal/scraper/complexity.go`:
- Around line 123-169: ParseGocycloOutput and ParseGocognitOutput duplicate the
same regex and parsing logic (gocycloRe, gocognitRe and both Parse* functions);
refactor by extracting a shared helper (suggested name
parseAverageComplexity(output, toolName string) (float64, error)) that takes the
output and tool name, runs the TrimSpace check, applies the regex, parses the
float and formats errors, then have ParseGocycloOutput and ParseGocognitOutput
call this helper (or replace them entirely) to remove duplicated regex and
parsing code while preserving existing error messages and behavior.
In `@jira-agent-dashboard/internal/scraper/gcs_test.go`:
- Around line 97-99: The test currently uses exact float equality for
p1.TotalCostUSD (and other checks at the same file) which is brittle; update the
assertions to use an epsilon comparison instead—compute math.Abs(p1.TotalCostUSD
- expected) and fail the test (using t.Errorf or t.Fatalf) if that difference is
greater than a small epsilon (e.g. 1e-6); apply the same change for the other
exact float checks referenced (lines comparing other phase costs) so all float
assertions use the tolerance-based pattern; locate the checks by searching for
TotalCostUSD and the exact numeric literals in gcs_test.go.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: db75b2b0-e6a9-4a0c-bd86-51494782abcd
⛔ Files ignored due to path filters (1)
jira-agent-dashboard/go.sumis excluded by!**/*.sum
📒 Files selected for processing (46)
jira-agent-dashboard/.gitignorejira-agent-dashboard/Containerfilejira-agent-dashboard/Makefilejira-agent-dashboard/README.mdjira-agent-dashboard/cmd/dashboard/main.gojira-agent-dashboard/cmd/scraper/main.gojira-agent-dashboard/deploy/dashboard-deployment.yamljira-agent-dashboard/deploy/dashboard-route.yamljira-agent-dashboard/deploy/kustomization.yamljira-agent-dashboard/deploy/namespace.yamljira-agent-dashboard/deploy/networkpolicy.yamljira-agent-dashboard/deploy/pvc.yamljira-agent-dashboard/deploy/rbac.yamljira-agent-dashboard/deploy/scraper-cronjob.yamljira-agent-dashboard/deploy/secrets.yaml.examplejira-agent-dashboard/go.modjira-agent-dashboard/internal/api/handlers.gojira-agent-dashboard/internal/api/handlers_test.gojira-agent-dashboard/internal/api/responses.gojira-agent-dashboard/internal/api/server.gojira-agent-dashboard/internal/db/models.gojira-agent-dashboard/internal/db/schema.gojira-agent-dashboard/internal/db/schema_test.gojira-agent-dashboard/internal/db/store.gojira-agent-dashboard/internal/db/store_test.gojira-agent-dashboard/internal/integration_test.gojira-agent-dashboard/internal/scraper/complexity.gojira-agent-dashboard/internal/scraper/complexity_test.gojira-agent-dashboard/internal/scraper/gcs.gojira-agent-dashboard/internal/scraper/gcs_test.gojira-agent-dashboard/internal/scraper/github.gojira-agent-dashboard/internal/scraper/github_test.gojira-agent-dashboard/internal/scraper/githubapp.gojira-agent-dashboard/internal/scraper/githubapp_test.gojira-agent-dashboard/internal/scraper/orchestrator.gojira-agent-dashboard/internal/scraper/orchestrator_test.gojira-agent-dashboard/web/comments.htmljira-agent-dashboard/web/css/style.cssjira-agent-dashboard/web/index.htmljira-agent-dashboard/web/issue.htmljira-agent-dashboard/web/issues.htmljira-agent-dashboard/web/js/app.jsjira-agent-dashboard/web/js/comments.jsjira-agent-dashboard/web/js/issue-detail.jsjira-agent-dashboard/web/js/issues.jsjira-agent-dashboard/web/js/trends.js
Comment on lines
+134
to
+136
| - port: 8080 | ||
| targetPort: 8080 | ||
| name: http |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the deployment manifest
find . -name "dashboard-deployment.yaml" -o -name "dashboard*.yaml" | head -20Repository: openshift/hypershift
Length of output: 170
🏁 Script executed:
# Also search for related manifests
find . -path "*/deploy/*" -type f | grep -E "\.(yaml|yml)$" | head -30Repository: openshift/hypershift
Length of output: 443
🏁 Script executed:
cat -n ./jira-agent-dashboard/deploy/dashboard-deployment.yamlRepository: openshift/hypershift
Length of output: 4890
🏁 Script executed:
cat -n ./jira-agent-dashboard/deploy/networkpolicy.yamlRepository: openshift/hypershift
Length of output: 1012
🏁 Script executed:
cat -n ./jira-agent-dashboard/deploy/dashboard-route.yamlRepository: openshift/hypershift
Length of output: 427
Remove the raw app port from the Service.
Port 8080 is the unauthenticated upstream behind oauth-proxy and should not be exposed on the Service. While the NetworkPolicy restricts ingress traffic to only port 4443, exposing unnecessary ports violates defense-in-depth principles. Health probes (lines 88-99) communicate directly with the pod container port and do not require the Service to publish 8080.
Suggested change
ports:
- port: 4443
targetPort: 4443
name: https
- - port: 8080
- targetPort: 8080
- name: http📝 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
| - port: 8080 | |
| targetPort: 8080 | |
| name: http | |
| ports: | |
| - port: 4443 | |
| targetPort: 4443 | |
| name: https |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jira-agent-dashboard/deploy/dashboard-deployment.yaml` around lines 134 -
136, The Service is exposing the upstream app port 8080 (port/name "http") which
must be removed; update the Service spec in dashboard-deployment.yaml to delete
the port entry that declares port: 8080 / targetPort: 8080 / name: http so only
the oauth-proxy HTTPS port (4443) remains exported, and verify
liveness/readiness probes (the container's probe settings) still reference the
containerPort directly and do not rely on the removed Service port.
Member
Author
There was a problem hiding this comment.
The deployment already has readOnlyRootFilesystem: true, runAsNonRoot: true, allowPrivilegeEscalation: false, and capabilities.drop: ALL configured. The OAuth proxy sidecar handles authn/authz. No changes needed.
AI-assisted response via Claude Code
Contributor
There was a problem hiding this comment.
@bryan-cox, noted — though to clarify, the container security context settings you mentioned (readOnlyRootFilesystem, runAsNonRoot, etc.) are orthogonal to the concern I raised, which was specifically about the Service exposing port 8080 to the cluster network. Those pod-level controls harden the container runtime, not the network surface.
That said, the dashboard-allow-ingress NetworkPolicy does effectively mitigate the risk: it restricts all pod-level ingress to port 4443 from the ingress namespace only, so port 8080 on the pod is unreachable even if the Service advertises it. Given that defense layer, I understand the decision to keep the Service port for operational convenience (e.g., internal tooling or future use). I'll defer to your judgment here.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
jira-agent-dashboard/internal/db/store.go (1)
459-468:⚠️ Potential issue | 🔴 CriticalPreserve analyzed complexity deltas on conflict.
This upsert still overwrites both analyzed deltas every time PR stats are refreshed. Any caller that only knows
lines_*/files_changedwill reset previously computed complexity back to0, and the row will look unanalyzed again downstream.Split stats-only vs analyzed upserts, or make the delta columns nullable/pointer-backed so the conflict branch can preserve existing values when deltas were not supplied.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/db/store.go` around lines 459 - 468, The upsert in InsertOrUpdatePRComplexity currently always overwrites cyclomatic_complexity_delta and cognitive_complexity_delta, wiping previously analyzed values; change the logic so stats-only updates do not clobber existing deltas by either: (a) making PRComplexity's delta fields nullable/pointer-backed and using COALESCE in the ON CONFLICT update to keep existing pr_complexity.cyclomatic_complexity_delta / pr_complexity.cognitive_complexity_delta when excluded values are NULL, or (b) splitting into two code paths: one update that only upserts lines_added/lines_deleted/files_changed and another that also updates deltas when the incoming PRComplexity includes non-nil delta values; update InsertOrUpdatePRComplexity and the PRComplexity struct accordingly so callers that don't supply deltas won't reset analyzed values.
🧹 Nitpick comments (1)
jira-agent-dashboard/internal/db/store_test.go (1)
331-352: Assert the persisted confidence value in this regression test.This test exercises the new
confidencepath but never verifies that the value round-tripped from storage. A regression in the feature added by this PR would still pass here.🧪 Suggested assertion
if got[0].Topic != "style" { t.Errorf("Topic = %q, want %q", got[0].Topic, "style") } + if got[0].Confidence == nil || *got[0].Confidence != conf { + t.Errorf("Confidence = %v, want %v", got[0].Confidence, conf) + } if !got[0].HumanOverride { t.Error("HumanOverride = false, want true") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/db/store_test.go` around lines 331 - 352, The test calls UpdateCommentClassification(id, "nitpick", "style", &conf, true) but never asserts that the confidence value persisted; add an assertion after fetching comments via GetReviewCommentsByIssueID that the returned comment's Confidence (e.g., got[0].Confidence) matches the original conf (use exact equality or a small delta if type is float64) so the round-trip of the confidence field is verified; locate this in the same test around the UpdateCommentClassification and got variable checks and add the check alongside the Severity/Topic/HumanOverride assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jira-agent-dashboard/internal/db/store_test.go`:
- Around line 13-17: The in-memory SQLite DB is per connection causing
flakiness; pin the test DB to a single connection by calling
conn.SetMaxOpenConns(1) and conn.SetMaxIdleConns(1) after opening the sql.DB so
all queries share the same in-memory instance (you can also use the shared
in-memory DSN like "file::memory:?mode=memory&cache=shared" if you prefer);
update the setup in store_test.go (where conn is opened) to set these limits so
the test harness never opens a second connection.
In `@jira-agent-dashboard/internal/db/store.go`:
- Around line 490-500: The current GetIssuesNeedingComplexity uses 0 as a
sentinel for "not analyzed" (pc.cyclomatic_complexity_delta = 0 AND
pc.cognitive_complexity_delta = 0) which incorrectly selects PRs that
legitimately have zero deltas; instead add an explicit analysis marker (e.g.,
pr_complexity.analyzed_at TIMESTAMP or pr_complexity.is_analyzed BOOLEAN) or
make the delta columns nullable and treat NULL as "not analyzed", then change
GetIssuesNeedingComplexity to check that marker (e.g., pc.is_analyzed = FALSE or
pc.analyzed_at IS NULL or pc.cyclomatic_complexity_delta IS NULL OR
pc.cognitive_complexity_delta IS NULL) and update the code that writes
pr_complexity (the creation/update logic for pr_complexity rows) to set the
marker when analysis completes so processed rows are not returned again.
- Around line 596-603: The SQL CASE in the subquery calculating topic_penalty
uses literal topic strings that don't match the canonical ReviewComment.Topic
enum (so some topics map to 0); update the WHEN branches in the COALESCE
subquery to use the exact enum values from the ReviewComment.Topic model (e.g.,
replace architecture_design/security/etc. with the model's canonical values such
as api_design, documentation, etc.), keeping the same weights, and ensure the
subquery in store.go (the SELECT SUM CASE over review_comments rc producing
topic_penalty) and any references to commentFilterSQL("rc.author","rc.body")
remain intact.
---
Duplicate comments:
In `@jira-agent-dashboard/internal/db/store.go`:
- Around line 459-468: The upsert in InsertOrUpdatePRComplexity currently always
overwrites cyclomatic_complexity_delta and cognitive_complexity_delta, wiping
previously analyzed values; change the logic so stats-only updates do not
clobber existing deltas by either: (a) making PRComplexity's delta fields
nullable/pointer-backed and using COALESCE in the ON CONFLICT update to keep
existing pr_complexity.cyclomatic_complexity_delta /
pr_complexity.cognitive_complexity_delta when excluded values are NULL, or (b)
splitting into two code paths: one update that only upserts
lines_added/lines_deleted/files_changed and another that also updates deltas
when the incoming PRComplexity includes non-nil delta values; update
InsertOrUpdatePRComplexity and the PRComplexity struct accordingly so callers
that don't supply deltas won't reset analyzed values.
---
Nitpick comments:
In `@jira-agent-dashboard/internal/db/store_test.go`:
- Around line 331-352: The test calls UpdateCommentClassification(id, "nitpick",
"style", &conf, true) but never asserts that the confidence value persisted; add
an assertion after fetching comments via GetReviewCommentsByIssueID that the
returned comment's Confidence (e.g., got[0].Confidence) matches the original
conf (use exact equality or a small delta if type is float64) so the round-trip
of the confidence field is verified; locate this in the same test around the
UpdateCommentClassification and got variable checks and add the check alongside
the Severity/Topic/HumanOverride assertions.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ae1e7f9-b3f2-44ec-9fbe-f16dd4e1de32
📒 Files selected for processing (4)
jira-agent-dashboard/internal/api/handlers.gojira-agent-dashboard/internal/api/responses.gojira-agent-dashboard/internal/db/store.gojira-agent-dashboard/internal/db/store_test.go
✅ Files skipped from review due to trivial changes (1)
- jira-agent-dashboard/internal/api/responses.go
🚧 Files skipped from review as they are similar to previous changes (1)
- jira-agent-dashboard/internal/api/handlers.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
jira-agent-dashboard/internal/db/store.go (1)
494-510:⚠️ Potential issue | 🟠 Major
0remains an unsafe sentinel for "not analyzed".PRs can legitimately have both complexity deltas equal to
0(e.g., a PR that only changes comments or strings). These rows will be returned and reprocessed on every run. Consider adding an explicitanalyzed_attimestamp oris_analyzedboolean column, or making the delta columns nullable and queryingIS NULL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/db/store.go` around lines 494 - 510, GetIssuesNeedingComplexity currently treats 0 as a sentinel for "not analyzed" which is unsafe; modify the schema and query so analysis is tracked explicitly (e.g., add pr_complexity.analyzed_at TIMESTAMP or pr_complexity.is_analyzed BOOLEAN, or make cyclomatic_complexity_delta and cognitive_complexity_delta NULLable) and change the query in GetIssuesNeedingComplexity to filter on the new indicator (e.g., WHERE pr_complexity.is_analyzed = FALSE or WHERE pr_complexity.analyzed_at IS NULL or WHERE pr_complexity.cyclomatic_complexity_delta IS NULL OR pr_complexity.cognitive_complexity_delta IS NULL) instead of comparing to 0 so legitimately-zero deltas are not repeatedly reprocessed; update any code paths that write to pr_complexity (the code that sets deltas) to set the new analyzed flag/timestamp when analysis completes.
🧹 Nitpick comments (8)
jira-agent-dashboard/Makefile (3)
1-1: Optionally add "all" target to satisfy checkmake.The static analysis tool expects a conventional
alltarget. While not required (the first targetbuildserves as the default), adding it follows GNU Make conventions.📋 Proposed addition
-.PHONY: build test lint clean dashboard scraper image deploy +.PHONY: all build test lint clean dashboard scraper image deploy + +all: build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/Makefile` at line 1, Add a conventional "all" Make target that depends on the existing "build" target to satisfy checkmake conventions; update the .PHONY declaration to include "all" alongside the current targets (build test lint clean dashboard scraper image deploy) so "make all" invokes the default build path and the phony list remains accurate.
20-21: Consider more comprehensive linting tools.
go vetprovides basic checks. For more thorough static analysis, consider addinggolangci-lint, which aggregates multiple linters and is widely adopted in Go projects.🔍 Proposed enhancement
lint: $(GO) vet ./... + `@command` -v golangci-lint >/dev/null 2>&1 && golangci-lint run ./... || echo "golangci-lint not installed, skipping"Or create a separate target:
-.PHONY: all build test lint clean dashboard scraper image deploy +.PHONY: all build test lint lint-advanced clean dashboard scraper image deploy lint: $(GO) vet ./... +lint-advanced: + golangci-lint run ./...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/Makefile` around lines 20 - 21, The current lint target only runs "$(GO) vet ./..." (target name: lint) which is minimal; add or replace it with a more comprehensive linter invocation by integrating golangci-lint: update the "lint" target (or add a new "lint-ci" target) to run "golangci-lint run ./..." (and optionally fall back to running "$(GO) vet ./..." if golangci-lint is not installed), and document the new dependency in the Makefile comments so CI and local devs know to install golangci-lint before running the target.
26-27: Parameterize image name and tag for CI/CD flexibility.The hardcoded
jira-agent-dashboard:latestworks for local development but limits flexibility in automated pipelines. Consider adding variables:🔧 Proposed refactor
GO := go BINARY_DIR := bin +IMAGE_NAME ?= jira-agent-dashboard +IMAGE_TAG ?= latest +IMAGE_REGISTRY ?= build: dashboard scraperimage: - podman build -t jira-agent-dashboard:latest -f Containerfile . + podman build -t $(if $(IMAGE_REGISTRY),$(IMAGE_REGISTRY)/)$(IMAGE_NAME):$(IMAGE_TAG) -f Containerfile .This allows overriding from the command line or CI:
make image IMAGE_TAG=v1.2.3 IMAGE_REGISTRY=quay.io/myorg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/Makefile` around lines 26 - 27, Replace the hardcoded image tag in the Makefile target named "image" with parameterized variables so CI or CLI can override registry/name/tag; add default variables like IMAGE_REGISTRY (default empty), IMAGE_NAME (default jira-agent-dashboard) and IMAGE_TAG (default latest) and use them to construct the full image reference for the podman build command (e.g., $(IMAGE_REGISTRY)/$(IMAGE_NAME):$(IMAGE_TAG)), ensuring the "image" target uses that composed variable.jira-agent-dashboard/web/js/comments.js (2)
213-221: Consider escaping class attribute values.
pattern.severityandpattern.topicare used directly in class attributes without escaping. While these values come from the API and should be from a known set, a compromised database or API could inject malicious content. The risk is low but consider usingescapeHTMLfor defense in depth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/web/js/comments.js` around lines 213 - 221, The template injection risk is that pattern.severity and pattern.topic are interpolated directly into row.innerHTML class attributes in the patternArray.forEach block; update the code to sanitize/escape those values before inserting into the DOM (e.g., use an escapeHTML helper or set className/textContent instead of innerHTML), ensuring the values used in class attributes and displayed text are escaped; locate the row.innerHTML assignment in the forEach and replace interpolation with escapedPatternSeverity and escapedPatternTopic (or use row.classList/add and createElement/textContent for the topic/severity cells) before calling tbody.appendChild.
9-21: Consider adding a request ID guard to prevent stale data.Similar to
issues.js, rapid time-range changes can cause out-of-order responses to overwrite newer data. Theissues.jsfile already implements this pattern withactiveLoadId.♻️ Suggested fix
let allComments = []; // stored for filtering let filteredComments = []; // current filtered view +let activeLoadId = 0; // Load comments data and render charts async function loadComments(from, to) { + const loadId = ++activeLoadId; try { allComments = await fetchAPI(`/api/comments/summary?from=${from}&to=${to}`); + if (loadId !== activeLoadId) return; renderSeverityChart(allComments);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/web/js/comments.js` around lines 9 - 21, The loadComments function can be overwritten by out-of-order fetches; add a request-id guard like issues.js: introduce a module-scoped counter (e.g., activeCommentsLoadId) that you increment at the start of loadComments, capture its value in a local loadId, and after the await fetchAPI(`/api/comments/summary?...`) but before mutating state (setting allComments and calling renderSeverityChart, renderTopicChart, renderPatternTable, populateAuthorFilter, applyCommentFilters), check that loadId === activeCommentsLoadId and abort if not; ensure errors still call showError only for the active load.jira-agent-dashboard/web/js/issues.js (1)
60-62: Consider validating URL schemes before placing in href.Similar to the fix in
comments.js,issue.jira_urlandissue.pr_urlare placed directly intohrefattributes. While these originate from trusted sources (server-constructed Jira URLs, GitHub scraper), adding scheme validation (startsWith('https://')) would provide defense in depth.♻️ Suggested fix
+ const jiraHref = issue.jira_url?.startsWith('https://') ? escapeHTML(issue.jira_url) : '#'; + const prHref = issue.pr_url?.startsWith('https://') ? escapeHTML(issue.pr_url) : '#'; + row.innerHTML = ` - <td><a href="${escapeHTML(issue.jira_url)}" target="_blank" onclick="event.stopPropagation()">${escapeHTML(issue.jira_key)}</a></td> - <td><a href="${escapeHTML(issue.pr_url)}" target="_blank" onclick="event.stopPropagation()">#${issue.pr_number}</a></td> + <td><a href="${jiraHref}" target="_blank" onclick="event.stopPropagation()">${escapeHTML(issue.jira_key)}</a></td> + <td><a href="${prHref}" target="_blank" onclick="event.stopPropagation()">#${issue.pr_number}</a></td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/web/js/issues.js` around lines 60 - 62, Validate the URL schemes for issue.jira_url and issue.pr_url before inserting them into the href in the row.innerHTML block: check that each string startsWith('https://') (or otherwise meets allowed schemes) and only use the original URL in the href if it passes; if it fails, use a safe fallback (e.g., '#' or omit the href) while still rendering the visible escaped text via escapeHTML(issue.jira_key) and `#`+issue.pr_number to avoid injecting unsafe schemes. Update the logic surrounding row.innerHTML generation (the code that references issue.jira_url and issue.pr_url) to perform this validation and choose the safe href value accordingly.jira-agent-dashboard/internal/scraper/orchestrator.go (1)
330-334: Error message string matching is fragile but pragmatic.Checking
strings.Contains(err.Error(), "UNIQUE constraint failed")relies on SQLite's exact error text. While error code checking would be more robust, the current approach works and is a common pattern with go-sqlite3. Consider documenting this behavior or adding a comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/scraper/orchestrator.go` around lines 330 - 334, The current check using strings.Contains(err.Error(), "UNIQUE constraint failed") (the branch around the comment insertion that logs "Warning: could not insert comment %d") is fragile because it relies on SQLite's exact text; add an inline comment above that conditional explaining why textual matching is used (reference to go-sqlite3 behavior and lack of portable error codes), note the fragility and suggest future improvement to switch to error code checking when available, and keep the existing logic (the strings.Contains check and the log.Printf with c.ID) unchanged to preserve current behavior.jira-agent-dashboard/internal/api/handlers.go (1)
343-357: Consider checking comment existence before updating.If the comment ID doesn't exist,
UpdateCommentClassificationwill succeed (affecting 0 rows), and thenGetReviewCommentByIDreturns 404. While not harmful, checking existence first would provide a clearer error. Alternatively, checksql.Result.RowsAffected()after the update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/api/handlers.go` around lines 343 - 357, The handler currently calls s.store.UpdateCommentClassification(...) then fetches the comment with s.store.GetReviewCommentByID(id), which yields a 404 if the update affected no rows; fix by ensuring existence is validated: either have UpdateCommentClassification return the sql.Result (or rowsAffected) and check result.RowsAffected()==0 to return http.StatusNotFound before fetching, or call s.store.GetReviewCommentByID(id) first to verify the comment exists and return 404 if missing, then call UpdateCommentClassification; update error handling in the handler accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jira-agent-dashboard/internal/scraper/orchestrator.go`:
- Around line 364-381: The current flow overwrites diff stats with zeros when
GetPRComplexityByIssueID fails; change the logic in orchestrator.go so that if
GetPRComplexityByIssueID returns a not-found condition you construct the
db.PRComplexity using diff stats from the analysis result (e.g.,
result.LinesAdded, result.LinesDeleted, result.FilesChanged) instead of
existing.LinesAdded/Deleted/FilesChanged, and only treat non-not-found errors as
logged warnings and skipped; continue to call InsertOrUpdatePRComplexity with
the assembled PRComplexity (using existing values when Get succeeds, or
result-provided diff stats when the row is missing).
In `@jira-agent-dashboard/web/js/issue-detail.js`:
- Around line 232-249: The template inserts comment.author directly into
innerHTML via commentDiv.innerHTML, risking XSS; change it to use the existing
escapeHTML helper when rendering the author (i.e., replace ${comment.author ||
'Unknown'} with ${escapeHTML(comment.author || 'Unknown')}) so the author string
is safely escaped before being embedded; update the template building in the
function that creates commentDiv (referenced by commentDiv and the template
block using formatDate, escapeHTML, severitySelect, topicSelect) to ensure
author values are consistently escaped.
---
Duplicate comments:
In `@jira-agent-dashboard/internal/db/store.go`:
- Around line 494-510: GetIssuesNeedingComplexity currently treats 0 as a
sentinel for "not analyzed" which is unsafe; modify the schema and query so
analysis is tracked explicitly (e.g., add pr_complexity.analyzed_at TIMESTAMP or
pr_complexity.is_analyzed BOOLEAN, or make cyclomatic_complexity_delta and
cognitive_complexity_delta NULLable) and change the query in
GetIssuesNeedingComplexity to filter on the new indicator (e.g., WHERE
pr_complexity.is_analyzed = FALSE or WHERE pr_complexity.analyzed_at IS NULL or
WHERE pr_complexity.cyclomatic_complexity_delta IS NULL OR
pr_complexity.cognitive_complexity_delta IS NULL) instead of comparing to 0 so
legitimately-zero deltas are not repeatedly reprocessed; update any code paths
that write to pr_complexity (the code that sets deltas) to set the new analyzed
flag/timestamp when analysis completes.
---
Nitpick comments:
In `@jira-agent-dashboard/internal/api/handlers.go`:
- Around line 343-357: The handler currently calls
s.store.UpdateCommentClassification(...) then fetches the comment with
s.store.GetReviewCommentByID(id), which yields a 404 if the update affected no
rows; fix by ensuring existence is validated: either have
UpdateCommentClassification return the sql.Result (or rowsAffected) and check
result.RowsAffected()==0 to return http.StatusNotFound before fetching, or call
s.store.GetReviewCommentByID(id) first to verify the comment exists and return
404 if missing, then call UpdateCommentClassification; update error handling in
the handler accordingly.
In `@jira-agent-dashboard/internal/scraper/orchestrator.go`:
- Around line 330-334: The current check using strings.Contains(err.Error(),
"UNIQUE constraint failed") (the branch around the comment insertion that logs
"Warning: could not insert comment %d") is fragile because it relies on SQLite's
exact text; add an inline comment above that conditional explaining why textual
matching is used (reference to go-sqlite3 behavior and lack of portable error
codes), note the fragility and suggest future improvement to switch to error
code checking when available, and keep the existing logic (the strings.Contains
check and the log.Printf with c.ID) unchanged to preserve current behavior.
In `@jira-agent-dashboard/Makefile`:
- Line 1: Add a conventional "all" Make target that depends on the existing
"build" target to satisfy checkmake conventions; update the .PHONY declaration
to include "all" alongside the current targets (build test lint clean dashboard
scraper image deploy) so "make all" invokes the default build path and the phony
list remains accurate.
- Around line 20-21: The current lint target only runs "$(GO) vet ./..." (target
name: lint) which is minimal; add or replace it with a more comprehensive linter
invocation by integrating golangci-lint: update the "lint" target (or add a new
"lint-ci" target) to run "golangci-lint run ./..." (and optionally fall back to
running "$(GO) vet ./..." if golangci-lint is not installed), and document the
new dependency in the Makefile comments so CI and local devs know to install
golangci-lint before running the target.
- Around line 26-27: Replace the hardcoded image tag in the Makefile target
named "image" with parameterized variables so CI or CLI can override
registry/name/tag; add default variables like IMAGE_REGISTRY (default empty),
IMAGE_NAME (default jira-agent-dashboard) and IMAGE_TAG (default latest) and use
them to construct the full image reference for the podman build command (e.g.,
$(IMAGE_REGISTRY)/$(IMAGE_NAME):$(IMAGE_TAG)), ensuring the "image" target uses
that composed variable.
In `@jira-agent-dashboard/web/js/comments.js`:
- Around line 213-221: The template injection risk is that pattern.severity and
pattern.topic are interpolated directly into row.innerHTML class attributes in
the patternArray.forEach block; update the code to sanitize/escape those values
before inserting into the DOM (e.g., use an escapeHTML helper or set
className/textContent instead of innerHTML), ensuring the values used in class
attributes and displayed text are escaped; locate the row.innerHTML assignment
in the forEach and replace interpolation with escapedPatternSeverity and
escapedPatternTopic (or use row.classList/add and createElement/textContent for
the topic/severity cells) before calling tbody.appendChild.
- Around line 9-21: The loadComments function can be overwritten by out-of-order
fetches; add a request-id guard like issues.js: introduce a module-scoped
counter (e.g., activeCommentsLoadId) that you increment at the start of
loadComments, capture its value in a local loadId, and after the await
fetchAPI(`/api/comments/summary?...`) but before mutating state (setting
allComments and calling renderSeverityChart, renderTopicChart,
renderPatternTable, populateAuthorFilter, applyCommentFilters), check that
loadId === activeCommentsLoadId and abort if not; ensure errors still call
showError only for the active load.
In `@jira-agent-dashboard/web/js/issues.js`:
- Around line 60-62: Validate the URL schemes for issue.jira_url and
issue.pr_url before inserting them into the href in the row.innerHTML block:
check that each string startsWith('https://') (or otherwise meets allowed
schemes) and only use the original URL in the href if it passes; if it fails,
use a safe fallback (e.g., '#' or omit the href) while still rendering the
visible escaped text via escapeHTML(issue.jira_key) and `#`+issue.pr_number to
avoid injecting unsafe schemes. Update the logic surrounding row.innerHTML
generation (the code that references issue.jira_url and issue.pr_url) to perform
this validation and choose the safe href value accordingly.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c307b5a7-094b-4abf-bf86-5c9e3bee330e
📒 Files selected for processing (11)
jira-agent-dashboard/Makefilejira-agent-dashboard/cmd/dashboard/main.gojira-agent-dashboard/internal/api/handlers.gojira-agent-dashboard/internal/db/schema.gojira-agent-dashboard/internal/db/store.gojira-agent-dashboard/internal/scraper/gcs.gojira-agent-dashboard/internal/scraper/orchestrator.gojira-agent-dashboard/web/css/style.cssjira-agent-dashboard/web/js/comments.jsjira-agent-dashboard/web/js/issue-detail.jsjira-agent-dashboard/web/js/issues.js
🚧 Files skipped from review as they are similar to previous changes (3)
- jira-agent-dashboard/cmd/dashboard/main.go
- jira-agent-dashboard/internal/db/schema.go
- jira-agent-dashboard/web/css/style.css
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
jira-agent-dashboard/internal/scraper/orchestrator.go (1)
330-334: Consider usingerrors.Isor a more robust error check for duplicate detection.The check
strings.Contains(err.Error(), "UNIQUE constraint failed")is fragile and depends on SQLite's exact error message format, which could change between versions.For SQLite with
github.com/mattn/go-sqlite3, you could check the error code:💡 Suggested improvement
import sqlite3 "github.com/mattn/go-sqlite3" // In the error handling: var sqliteErr sqlite3.Error if errors.As(err, &sqliteErr) && sqliteErr.ExtendedCode == sqlite3.ErrConstraintUnique { // skip duplicate continue }Alternatively, the current approach is acceptable if you want to avoid the driver-specific dependency, but worth noting the brittleness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/scraper/orchestrator.go` around lines 330 - 334, The current duplicate-detection using strings.Contains(err.Error(), "UNIQUE constraint failed") is brittle; replace it with a driver-aware error check by importing the sqlite3 package (alias sqlite3) and using errors.As to cast err to sqlite3.Error, then check sqliteErr.ExtendedCode == sqlite3.ErrConstraintUnique (or sqliteErr.Code as appropriate) inside the error handling in the insert path (around the block that logs "could not insert comment" for c.ID) and skip/continue on that specific constraint error; keep a fallback to the existing log for other errors.jira-agent-dashboard/internal/db/schema.go (1)
9-14: Consider enabling foreign key enforcement.SQLite does not enforce foreign key constraints by default. The schema declares
REFERENCESclauses (e.g.,issues.job_run_id REFERENCES job_runs(id)), but withoutPRAGMA foreign_keys=ON, these are purely documentary and won't prevent orphan rows or cascading issues.If referential integrity is desired, add the pragma after WAL setup:
💡 Suggested change
_, err := db.Exec("PRAGMA journal_mode=WAL") if err != nil { return err } + _, err = db.Exec("PRAGMA foreign_keys=ON") + if err != nil { + return err + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/internal/db/schema.go` around lines 9 - 14, The InitSchema function currently enables WAL but does not turn on SQLite foreign key enforcement, so REFERENCES in the schema (e.g., issues.job_run_id REFERENCES job_runs(id)) are not enforced; modify InitSchema to execute "PRAGMA foreign_keys=ON" (immediately after the existing "PRAGMA journal_mode=WAL" call) and handle/return any error from that Exec call so foreign key constraints are enforced at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jira-agent-dashboard/internal/db/store.go`:
- Around line 482-495: The SELECT in Store.GetPRComplexityByIssueID omits the
complexity_analyzed column so PRComplexity.ComplexityAnalyzed is always false;
update the query to include complexity_analyzed in the SELECT list and add
&c.ComplexityAnalyzed to the row.Scan call in GetPRComplexityByIssueID so the
boolean field on PRComplexity is populated from the DB.
---
Nitpick comments:
In `@jira-agent-dashboard/internal/db/schema.go`:
- Around line 9-14: The InitSchema function currently enables WAL but does not
turn on SQLite foreign key enforcement, so REFERENCES in the schema (e.g.,
issues.job_run_id REFERENCES job_runs(id)) are not enforced; modify InitSchema
to execute "PRAGMA foreign_keys=ON" (immediately after the existing "PRAGMA
journal_mode=WAL" call) and handle/return any error from that Exec call so
foreign key constraints are enforced at runtime.
In `@jira-agent-dashboard/internal/scraper/orchestrator.go`:
- Around line 330-334: The current duplicate-detection using
strings.Contains(err.Error(), "UNIQUE constraint failed") is brittle; replace it
with a driver-aware error check by importing the sqlite3 package (alias sqlite3)
and using errors.As to cast err to sqlite3.Error, then check
sqliteErr.ExtendedCode == sqlite3.ErrConstraintUnique (or sqliteErr.Code as
appropriate) inside the error handling in the insert path (around the block that
logs "could not insert comment" for c.ID) and skip/continue on that specific
constraint error; keep a fallback to the existing log for other errors.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 80e78668-76c4-4d92-b8a6-89a4165e3f30
📒 Files selected for processing (6)
jira-agent-dashboard/internal/db/models.gojira-agent-dashboard/internal/db/schema.gojira-agent-dashboard/internal/db/store.gojira-agent-dashboard/internal/db/store_test.gojira-agent-dashboard/internal/scraper/orchestrator.gojira-agent-dashboard/web/js/issue-detail.js
✅ Files skipped from review due to trivial changes (2)
- jira-agent-dashboard/internal/db/models.go
- jira-agent-dashboard/internal/db/store_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- jira-agent-dashboard/web/js/issue-detail.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
jira-agent-dashboard/web/js/comments.js (1)
215-223: Consider defense-in-depth for class attribute interpolation.
pattern.severityandpattern.topicare interpolated directly into class attributes. While the server enforces allowlists (per PR description), using DOM APIs would provide client-side defense against any future server-side bugs or data corruption.♻️ Safer approach using DOM APIs
patternArray.forEach(pattern => { const row = document.createElement('tr'); - row.innerHTML = ` - <td><span class="tag ${pattern.severity}">${pattern.severity.replace(/_/g, ' ')}</span></td> - <td><span class="tag ${pattern.topic}">${pattern.topic.replace(/_/g, ' ')}</span></td> - <td>${formatNumber(pattern.count)}</td> - `; + const severityCell = document.createElement('td'); + const severitySpan = document.createElement('span'); + severitySpan.className = 'tag ' + pattern.severity; + severitySpan.textContent = pattern.severity.replace(/_/g, ' '); + severityCell.appendChild(severitySpan); + + const topicCell = document.createElement('td'); + const topicSpan = document.createElement('span'); + topicSpan.className = 'tag ' + pattern.topic; + topicSpan.textContent = pattern.topic.replace(/_/g, ' '); + topicCell.appendChild(topicSpan); + + const countCell = document.createElement('td'); + countCell.textContent = formatNumber(pattern.count); + + row.appendChild(severityCell); + row.appendChild(topicCell); + row.appendChild(countCell); tbody.appendChild(row); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jira-agent-dashboard/web/js/comments.js` around lines 215 - 223, The code directly interpolates pattern.severity and pattern.topic into row.innerHTML which risks class injection; instead build the cells and spans with DOM APIs: create each td and span via document.createElement, set span.classList.add(...) after sanitizing or mapping the severity/topic tokens (use pattern.severity.replace(/_/g,' ') for display but a safe token for class names), assign text via textContent (e.g., for the display value and formatNumber(pattern.count)), and append the elements to row before tbody.appendChild(row); update the block that iterates patternArray (the row creation logic referencing pattern.severity, pattern.topic, tbody, formatNumber) to use this safer DOM-construction approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jira-agent-dashboard/web/js/comments.js`:
- Around line 45-51: The filter callback used to produce `filtered` can throw if
`c.body` or `c.author` is null/undefined when `search` is set; update the
`allComments.filter` predicate to defensively handle missing strings by treating
`c.body` and `c.author` as empty strings (or use optional chaining) before
calling `toLowerCase()` so the `search` checks never call methods on null.
Locate the anonymous filter function (used to compute `filtered`) and replace
the direct `c.body.toLowerCase()` and `c.author.toLowerCase()` uses with safe
equivalents that default to `''` when those properties are falsy, preserving the
existing severity/topic/author checks and overall logic.
---
Nitpick comments:
In `@jira-agent-dashboard/web/js/comments.js`:
- Around line 215-223: The code directly interpolates pattern.severity and
pattern.topic into row.innerHTML which risks class injection; instead build the
cells and spans with DOM APIs: create each td and span via
document.createElement, set span.classList.add(...) after sanitizing or mapping
the severity/topic tokens (use pattern.severity.replace(/_/g,' ') for display
but a safe token for class names), assign text via textContent (e.g., for the
display value and formatNumber(pattern.count)), and append the elements to row
before tbody.appendChild(row); update the block that iterates patternArray (the
row creation logic referencing pattern.severity, pattern.topic, tbody,
formatNumber) to use this safer DOM-construction approach.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 18a69b8c-c63b-478b-8ec9-42e6cc7fd869
📒 Files selected for processing (3)
jira-agent-dashboard/web/comments.htmljira-agent-dashboard/web/css/style.cssjira-agent-dashboard/web/js/comments.js
✅ Files skipped from review due to trivial changes (1)
- jira-agent-dashboard/web/comments.html
🚧 Files skipped from review as they are similar to previous changes (1)
- jira-agent-dashboard/web/css/style.css
Contributor
|
@bryan-cox: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
bryan-cox
force-pushed
the
CNTRLPLANE-3043
branch
4 times, most recently
from
June 5, 2026 14:12
4e77eb7 to
c1bf525
Compare
openshift-ci
Bot
added
area/documentation
bryan-cox
force-pushed
the
CNTRLPLANE-3043
branch
from
June 5, 2026 18:20
db7c64b to
dbbbc3f
Compare
bryan-cox
force-pushed
the
CNTRLPLANE-3043
branch
from
June 5, 2026 18:46
dbbbc3f to
c0e3c05
Compare
bryan-cox
force-pushed
the
CNTRLPLANE-3043
branch
from
June 5, 2026 18:55
c0e3c05 to
eb0f75c
Compare
bryan-cox
force-pushed
the
CNTRLPLANE-3043
branch
7 times, most recently
from
July 11, 2026 17:39
4acb764 to
ecdcd11
Compare
Contributor
|
Now I have the full root cause. The PR adds two new markdown files under Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe verify job runs Root CauseThe
The PR adds two new documentation files:
The committed version of Recommendations
Evidence
|
Add Kubernetes deployment manifests (Deployment, Route, PVC, CronJob, RBAC, NetworkPolicy), container build files, Go module definition, and Makefile for the JIRA Agent Dashboard. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add database schema, models, and store with queries for issues, job runs, review comments, session telemetry, and trend aggregation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add the scraper orchestrator that coordinates multi-step data collection, and the GCS scraper that reads Prow CI build logs to extract JIRA issue keys and session metadata. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add GitHub scrapers that enrich issues with PR metadata (state, merge status, timestamps) and review comments with severity classification. Includes GitHub App authentication support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add code complexity analysis (gocyclo, cognitive), autodl session telemetry scraper, and OpenTelemetry event ingestion for enriching issue data with cost, token, and duration metrics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add HTTP API handlers for issues, trends, review comments, and session telemetry. Includes server setup, response helpers, integration tests, and CLI entrypoints for dashboard and scraper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add CSS stylesheet, shared JS utilities (date formatting, time range selectors, Chart.js helpers), trend chart rendering, and glossary page. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add the main overview page with business impact hero, health and cost summary cards, charts, and activity feed. Add outcomes page with per-issue detail view and issue drill-down page. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pages Add failure analysis page with root cause breakdown and failure patterns. Add review quality page with reviewer metrics and comment severity analysis. Add internals page with scraper pipeline status and session telemetry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
force-pushed
the
CNTRLPLANE-3043
branch
from
July 23, 2026 13:07
69e7e4f to
d09c0ea
Compare
Add multi-job GCS scraping to support the installer team's periodic jira-agent job alongside the existing HyperShift job. Each job gets its own HTTPGCSClient configured with the correct GCS prefix and step path. A job_name column in job_runs attributes data to each team. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Thread job_name from job_runs through the API as a component field and surface it on the outcomes page with a filterable dropdown and color-coded badges. Legacy rows default to "hypershift". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add toggle chip buttons for filtering data by team component (e.g. hypershift, installer) across all 5 dashboard pages. Components are dynamically discovered from data, and user selection persists across time-range changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…persistence Dynamically inject time range and component filter controls via app.js into a header-bar-anchor div, eliminating HTML duplication across all 5 pages. Persist time range and component selections to localStorage so they survive page navigation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… menu Replace time-range buttons with a select element and convert component filter chips into a dropdown checkbox menu for a more compact header. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add trt-origin and trt-sippy job configs to the GCS scraper and parse the TRT build-log format which uses "Issue: TRT-2823 |" instead of "Processing: OCPBUGS-79071". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Convert the status donut chart to a stacked bar chart grouped by component and compute the impact trend client-side from issue data instead of a separate API call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add min-width:0 and overflow-x:hidden on main to contain flex item growth, and overflow-wrap:break-word on comment bodies so long unbroken strings wrap instead of stretching the viewport. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bryan-cox
force-pushed
the
CNTRLPLANE-3043
branch
from
July 24, 2026 18:32
47da1e5 to
c392dea
Compare
Contributor
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
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.
What this PR does / why we need it:
Adds a self-contained web dashboard and data pipeline for tracking jira-agent performance metrics on a dedicated HostedCluster. This gives the team visibility into merge rates, Claude API costs, PR lifecycle times, and review comment quality.
Components
scraper-prowpulls Prow job artifacts from GCS,scraper-githubfetches PR metadata and review comments via a dedicated GitHub App.Frontend Features
Security Hardening
openshift-delegate-urlsrequiresgetonservicesin namespace;system:auth-delegatorClusterRoleBinding for token reviewreencrypttermination with auto-generated service-serving-certUSER 1001),readOnlyRootFilesystem,allowPrivilegeEscalation: false, all capabilities droppedescapeHTML()applied to allinnerHTMLinterpolation.gitignore; private key mounted at0400no-cache, must-revalidateon static files to prevent stale JS/CSSDeployment
All Kubernetes manifests are in
deploy/managed by kustomize. Secrets are created manually from the providedsecrets.yaml.exampletemplate. Full restore-from-scratch instructions are in the README.Which issue(s) this PR fixes:
Fixes CNTRLPLANE-3043
Special notes for your reviewer:
jira-agent-dashboard/) with its owngo.mod— it does not affect the main HyperShift module or any existing code.vendor/directory is not committed; dependencies are managed viago.mod.scraper-complexityCronJob was removed — complexity analysis may be re-added later.Checklist:
Summary by CodeRabbit
New Features
Chores
Documentation
Tests