Parse historical analysis logs from victoria#361
Merged
Conversation
📝 WalkthroughWalkthroughIntroduces structured pod log DTOs and two new endpoints ( Changes
Sequence DiagramsequenceDiagram
participant Container as ContainerLogs
participant API as PodOrc API
participant Card as AnalysisLogCardContent
Container->>+API: GET /logs/{analysis_id}
API-->>-Container: AnalysisLogsResponse (analysis_logs[], nginx_logs[], run_number)
Container->>+API: GET /history/{analysis_id}
API-->>-Container: AnalysisLogHistoryResponse (runs: RunLogs[])
Container->>Card: pass current analysis_logs / nginx_logs
Card->>Card: compute formatted strings (formatTimestamp / formatLogs)
Card-->>Container: render formatted/current logs (with toggle for timestamps)
Container->>Card: pass previous runs' logs (prevRunsList)
Card-->>Container: render historical run logs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/mockapi/handlers.ts (1)
93-123:⚠️ Potential issue | 🟠 MajorUpdate the mocks to return the new log DTOs.
These handlers now match
/logsand/history, but still return the old nested{ status, data: { analysis, nginx } }shape. Consumers now expectAnalysisLogsResponseandAnalysisLogHistoryResponse, so tests using these mocks will render empty logs or miss history entirely.🐛 Proposed mock response update
import { + type AnalysisLogHistoryResponse, + type AnalysisLogsResponse, type BodyKongInitializeKongInitializePost, type BodyPodorcPodsCreatePoPost, type CleanupPodResponse, PodStatus, } from "~/services/Api"; @@ // Analysis logs http.get(`/logs/${fakeAnalysisId}`, () => { - return HttpResponse.json({ - status: 200, - data: { - analysis: { - [fakeAnalysisId]: ["Starting FlameCoreSDK"], - }, - nginx: { - [fakeAnalysisId]: [ - "/docker-entrypoint.sh: /docker-entrypoint.d/ is not empty", - ], - }, - }, - }); + const response: AnalysisLogsResponse = { + analysis_id: fakeAnalysisId, + run_number: 1, + analysis_logs: [ + { + timestamp: "2026-04-22T00:00:00Z", + message: "Starting FlameCoreSDK", + }, + ], + nginx_logs: [ + { + timestamp: "2026-04-22T00:00:00Z", + message: "/docker-entrypoint.sh: /docker-entrypoint.d/ is not empty", + }, + ], + }; + + return HttpResponse.json(response); }), http.get(`/history/${fakeAnalysisId}`, () => { - return HttpResponse.json({ - status: 200, - data: { - analysis: { - [fakeAnalysisId]: ["Starting FlameCoreSDK"], - }, - nginx: { - [fakeAnalysisId]: [ - "/docker-entrypoint.sh: /docker-entrypoint.d/ is not empty", - ], - }, - }, - }); + const response: AnalysisLogHistoryResponse = { + analysis_id: fakeAnalysisId, + runs: [ + { + run_number: 1, + analysis_logs: [ + { + timestamp: "2026-04-22T00:00:00Z", + message: "Starting FlameCoreSDK", + }, + ], + nginx_logs: [ + { + timestamp: "2026-04-22T00:00:00Z", + message: "/docker-entrypoint.sh: /docker-entrypoint.d/ is not empty", + }, + ], + }, + ], + }; + + return HttpResponse.json(response); }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mockapi/handlers.ts` around lines 93 - 123, The mock handlers for http.get(`/logs/${fakeAnalysisId}`) and http.get(`/history/${fakeAnalysisId}`) still return the old {status, data: {analysis, nginx}} shape; update them to return the new AnalysisLogsResponse and AnalysisLogHistoryResponse shapes respectively so tests consume real DTOs. Replace the nested analysis/nginx structure with the DTOs' expected top-level properties (e.g., the response.data.logs or response.data.history arrays/objects per the AnalysisLogsResponse/AnalysisLogHistoryResponse definitions) and populate entries for the fakeAnalysisId with appropriate targets (analysis and nginx) and their log lines so consumers see non-empty logs/history. Ensure you return the same HTTP status (200) but with the new DTO structure from the handlers named in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/analysis/logs/ContainerLogs.vue`:
- Around line 30-36: gatherCurrentLogs currently leaves currentLogs unchanged
for non-403 errors, causing stale live logs to persist when the live endpoint
returns 404; update the error handling in gatherCurrentLogs so that when
status.value === "error" and error.value?.statusCode === 404 you set
currentLogs.value = null (and keep the existing navigateTo("/error/403")
behavior for 403), and apply the same 404-clearing change to the similar block
around lines handling the same logic (the second occurrence referenced in the
review) so both code paths clear currentLogs on 404.
- Around line 45-49: The prevLogs list is including the same run twice because
historyResp.runs contains the current/latest run; update the assignment of
prevLogs (and the equivalent logic used for the “All Previous Runs” rendering
between lines 88-121) to filter out the run that is already shown as the
most-recent/live run by comparing run_number (e.g., exclude historyResp.runs
entries where run_number === currentRunNumber or activeLiveRun?.run_number),
then sort the remaining items descending as before so the UI never renders the
same run in both “Most Recent Run” and “All Previous Runs”.
In `@app/services/hub_adapter_swagger.json`:
- Around line 5894-5909: PodLog.timestamp is declared as a plain string with no
format/contract; update the PodLog schema (PodLog.timestamp) to either add
"format": "date-time" if the hub adapter normalizes timestamps to ISO‑8601 so
generated clients/validators match EventLog.timestamp, or otherwise add a clear
description on PodLog.timestamp indicating the exact format (e.g., epoch ms,
Victoria `_time`, or other) the frontend must parse; ensure the chosen change
keeps PodLog.timestamp consistent with EventLog.timestamp behavior.
- Around line 6175-6185: The Registry.account_secret and
RegistryProject.account_secret fields are exposed in response schemas and should
be treated as sensitive; update their property definitions in the OpenAPI JSON
(the objects named "Registry" and "RegistryProject" in
app/services/hub_adapter_swagger.json) to include "writeOnly": true so the field
is only accepted on input and omitted from responses (this will prevent the
TypeScript client from treating it as a response field once clients are
regenerated), and remove or redact any real secret values from the mock data
file public/analyses.json so tests/examples do not contain credentials.
- Around line 2819-2870: The new path parameters for analysis_id in the
endpoints "logs_analysis_live_get_logs__analysis_id__get" (/logs/{analysis_id})
and the analogous /history/{analysis_id} need to match the existing flexible
schema pattern: update the parameter schema for the "analysis_id" parameter (the
parameter object named analysis_id) to use anyOf with the same options used
elsewhere (e.g., anyOf: [ { type: "string", format: "uuid" }, { type: "string" }
] or anyOf: [ { type: "string", format: "uuid" }, { "type": "string" }, {
"type": "null" } ] depending on which pattern `/po/status/{analysis_id}` and
`/analyses/{analysis_id}` use) so the contract is consistent across endpoints.
---
Outside diff comments:
In `@test/mockapi/handlers.ts`:
- Around line 93-123: The mock handlers for http.get(`/logs/${fakeAnalysisId}`)
and http.get(`/history/${fakeAnalysisId}`) still return the old {status, data:
{analysis, nginx}} shape; update them to return the new AnalysisLogsResponse and
AnalysisLogHistoryResponse shapes respectively so tests consume real DTOs.
Replace the nested analysis/nginx structure with the DTOs' expected top-level
properties (e.g., the response.data.logs or response.data.history arrays/objects
per the AnalysisLogsResponse/AnalysisLogHistoryResponse definitions) and
populate entries for the fakeAnalysisId with appropriate targets (analysis and
nginx) and their log lines so consumers see non-empty logs/history. Ensure you
return the same HTTP status (200) but with the new DTO structure from the
handlers named in the diff.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec0544ef-4f3d-4c13-a3dc-5c73de089714
📒 Files selected for processing (6)
app/components/analysis/logs/AnalysisLogCardContent.vueapp/components/analysis/logs/ContainerLogs.vueapp/composables/useAPIFetch.tsapp/services/Api.tsapp/services/hub_adapter_swagger.jsontest/mockapi/handlers.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/components/analysis/logs/ContainerLogs.vue (1)
61-64:⚠️ Potential issue | 🟡 Minor
refreshLogsnever refreshes history.
gatherPreviousLogs()only runs once at setup. When a run finishes (or a new run starts) while the panel is open, the polling loop updatescurrentLogsbutprevLogsstays frozen — so the completed run never appears under "All Previous Runs" without a full remount, and the run-number filter at line 48 can also become stale.♻️ Suggested fix
async function refreshLogs() { await refresh(); gatherCurrentLogs(); + await gatherPreviousLogs(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/analysis/logs/ContainerLogs.vue` around lines 61 - 64, refreshLogs currently calls refresh() and gatherCurrentLogs() but never updates the historical cache, so prevLogs and the run-number filter become stale when runs start/finish; update refreshLogs to also call gatherPreviousLogs() (or call the function that repopulates prevLogs) after refresh() so previous runs are recomputed when polling occurs, ensuring gatherPreviousLogs(), currentLogs and prevLogs stay in sync with refresh() updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/components/analysis/logs/ContainerLogs.vue`:
- Around line 61-64: refreshLogs currently calls refresh() and
gatherCurrentLogs() but never updates the historical cache, so prevLogs and the
run-number filter become stale when runs start/finish; update refreshLogs to
also call gatherPreviousLogs() (or call the function that repopulates prevLogs)
after refresh() so previous runs are recomputed when polling occurs, ensuring
gatherPreviousLogs(), currentLogs and prevLogs stay in sync with refresh()
updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfca8d32-9ea4-4a24-996a-cb8427f76299
📒 Files selected for processing (5)
app/components/analysis/logs/ContainerLogs.vuetest/components/analysis/constants.tstest/components/analysis/logs/AnalysisLogCardContent.spec.tstest/components/analysis/logs/ContainerLogs.spec.tstest/mockapi/handlers.ts
✅ Files skipped from review due to trivial changes (1)
- test/components/analysis/constants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/mockapi/handlers.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Security
Tests