Lazy load logs#377
Merged
Merged
Conversation
Implements chunked log fetching with initialize/loadNextChunk/appendPolled/reset API, supporting 300-entry pages, hasMore tracking, and error capture.
Closed
|
Warning Review limit reached
More reviews will be available in 7 minutes and 23 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR refactors analysis log loading and rendering: adds a FlatLogLine model and flattenLogs, implements useLogChunks for chunked pagination, virtualizes AnalysisLogCardContent with VirtualScroller and scroll-based load/auto-scroll behavior, updates ContainerLogs to orchestrate polling/prev-runs, and adjusts related tests and infra. ChangesAnalysis Logs Chunked Loading and Virtualization
Sequence Diagram(s)sequenceDiagram
participant User
participant ContainerLogs
participant LogChunks as useLogChunks
participant Card as AnalysisLogCardContent
participant API
User->>ContainerLogs: mount page
ContainerLogs->>LogChunks: initialize()
LogChunks->>API: GET /logs/{id}?limit=300
API-->>LogChunks: initial chunk
LogChunks->>LogChunks: flatten nginx & analysis lines
LogChunks-->>ContainerLogs: nginxLines, analysisLines, hasOlder
ContainerLogs->>Card: pass lines + onLoadOlderNginx callback
Card->>Card: render virtualized lines
User->>Card: scroll to top
Card->>Card: detect near-top, hasOlder=true
Card->>ContainerLogs: onLoadOlderNginx()
ContainerLogs->>LogChunks: loadOlderChunk()
LogChunks->>API: GET /logs/{id}?start_date=oldest
API-->>LogChunks: older chunk
LogChunks->>LogChunks: prepend older, update hasOlder
LogChunks-->>Card: updated nginxLines
Card->>Card: adjust scrollTop, render new top items
User->>Card: scroll near bottom
ContainerLogs->>API: GET /logs/{id}?start_date=lastFetchedAt
API-->>ContainerLogs: polled logs
ContainerLogs->>LogChunks: appendPolled(newLogs)
LogChunks-->>Card: updated lines (append)
Card->>Card: auto-scroll to bottom (if near-bottom)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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: 3
🧹 Nitpick comments (1)
test/composables/useLogChunks.test.ts (1)
109-151: ⚡ Quick winAdd a regression test for repeated boundary timestamps in
loadOlderChunk().Current tests don’t cover the case where two consecutive chunks share the same oldest timestamp; that’s the failure mode that can duplicate prepended rows.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/composables/useLogChunks.test.ts` around lines 109 - 151, Add a regression test to the describe("loadOlderChunk") suite that simulates two consecutive chunks sharing the same boundary/oldest timestamp and assert no duplicate lines are prepended: use testServer to return an initial chunk (via useLogChunks(...).initialize()) and then a second chunk whose newest/oldest timestamp equals the first chunk's oldest timestamp, call loadOlderChunk(), and assert nginxLines and hasOlder reflect deduplication (no duplicated content at the boundary and total length equals unique lines). Reference loadOlderChunk, useLogChunks, nginxLines, and hasOlder when locating where to add this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/components/analysis/logs/AnalysisLogCardContent.vue`:
- Around line 226-233: The VirtualScroller instances (the components using
:itemSize="20" for nginxLines and the analysis list) assume fixed row heights
but the rendered rows (elements with classes .log-message and
.log-stacktrace-line) use white-space: pre-wrap and can vary in height; either
make each rendered log row a fixed, stable height that matches the itemSize
(e.g., add CSS to .log-message and .log-stacktrace-line to clamp lines, set a
fixed height and overflow:hidden so every item is exactly 20px) or remove the
fixed itemSize and switch to a virtualization approach that measures per-item
heights; apply the change to both VirtualScroller usages (the one with
ref="nginxScrollerRef" and the other analysis VirtualScroller) so virtualization
offsets remain correct.
In `@app/components/analysis/logs/ContainerLogs.vue`:
- Around line 38-40: The polling cursor currently advances lastFetchedAt using
client time (new Date().toISOString()) in the log initialization and polling
code (when checking logChunks.initialized.value and in the fetch/update block
around lines ~88-100); change both places to set lastFetchedAt from the
server-provided timestamp in the returned logs (e.g., the latest log entry's
timestamp or a server cursor field in the fetch response) and only fall back to
client time if no server timestamp is available; update the assignments to
lastFetchedAt.value accordingly and ensure any parsing/formatting matches the
server's ISO timestamp format.
In `@app/composables/useLogChunks.ts`:
- Around line 64-80: loadOlderChunk can prepend duplicate rows when the same
timestamp boundary is reused; to fix, after calling fetchChunk({ end_date:
oldestTimestamp.value }) filter out any returned nginx_logs and analysis_logs
whose timestamp equals the current oldestTimestamp.value (or otherwise treat
end_date as exclusive) before flattening and prepending to nginxLines.value and
analysisLines.value, then compute oldestTimestampOf over the deduplicated
results and set hasOlder based on the deduplicated counts vs CHUNK_SIZE;
reference symbols: loadOlderChunk, fetchChunk, oldestTimestamp.value,
nginx_logs, analysis_logs, nginxLines.value, analysisLines.value,
oldestTimestampOf, CHUNK_SIZE.
---
Nitpick comments:
In `@test/composables/useLogChunks.test.ts`:
- Around line 109-151: Add a regression test to the describe("loadOlderChunk")
suite that simulates two consecutive chunks sharing the same boundary/oldest
timestamp and assert no duplicate lines are prepended: use testServer to return
an initial chunk (via useLogChunks(...).initialize()) and then a second chunk
whose newest/oldest timestamp equals the first chunk's oldest timestamp, call
loadOlderChunk(), and assert nginxLines and hasOlder reflect deduplication (no
duplicated content at the boundary and total length equals unique lines).
Reference loadOlderChunk, useLogChunks, nginxLines, and hasOlder when locating
where to add this test.
🪄 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: a851187b-c7dd-4cce-b0e4-ab81f12925f8
📒 Files selected for processing (13)
.github/actions/build-docker-image/action.yaml.gitignoreapp/components/analysis/logs/AnalysisLogCardContent.vueapp/components/analysis/logs/ContainerLogs.vueapp/components/analysis/logs/RefreshSwitch.vueapp/composables/useLogChunks.tsapp/types/logs.tstest/components/analysis/logs/AnalysisLogCardContent.spec.tstest/components/analysis/logs/ContainerLogs.spec.tstest/components/analysis/logs/RefreshSwitch.spec.tstest/composables/useLogChunks.test.tstest/mockapi/setup.tstest/utils/flatten-logs.test.ts
💤 Files with no reviewable changes (1)
- .github/actions/build-docker-image/action.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/analysis/logs/ContainerLogs.vue (1)
179-184:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUsing one prepend callback for both panes will make the other pane jump.
AnalysisLogCardContentonly preserves the scroll position of the pane that triggeredonLoadOlder*, but both props here point at the samelogChunks.loadOlderChunk, which prepends both arrays at once. Scrolling to the top in one pane will therefore shift the other pane’s viewport without restoring it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/analysis/logs/ContainerLogs.vue` around lines 179 - 184, The two panes share the same prepend callback which causes the non-triggering pane to jump; replace the single logChunks.loadOlderChunk usage with two distinct callbacks (e.g., loadOlderNginxChunk and loadOlderAnalysisChunk or small wrapper functions) and wire those to AnalysisLogCardContent's onLoadOlderNginx and onLoadOlderAnalysis so each callback only prepends to its own array; ensure you still use logChunks.hasOlder.value and logChunks.isLoading.value for each pane but update the underlying loader functions to target nginx vs analysis chunks separately.
♻️ Duplicate comments (1)
app/components/analysis/logs/ContainerLogs.vue (1)
100-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the polling cursor unchanged when a poll returns no rows.
The
fetchTimefallback still advanceslastFetchedAtwith client time on empty responses, so delayed logs with older server timestamps can be skipped permanently. Only move the cursor when the response actually contains a newer log timestamp.💡 Minimal fix
async function refreshLogs() { if (!lastFetchedAt.value) return; - const fetchTime = new Date().toISOString(); const result = await useNuxtApp() .$hubApi<AnalysisLogsResponse>(`/logs/${analysisId}`, { method: "GET", query: { start_date: lastFetchedAt.value }, }) .catch(() => undefined); if (result) { logChunks.appendPolled(result); const allLogs = [...result.nginx_logs, ...result.analysis_logs]; - lastFetchedAt.value = - allLogs.length > 0 - ? allLogs.reduce( - (max, l) => (l.timestamp > max ? l.timestamp : max), - allLogs[0]!.timestamp, - ) - : fetchTime; + if (allLogs.length > 0) { + lastFetchedAt.value = allLogs.reduce( + (max, l) => (l.timestamp > max ? l.timestamp : max), + allLogs[0]!.timestamp, + ); + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/analysis/logs/ContainerLogs.vue` around lines 100 - 116, The current polling logic advances the cursor to fetchTime even when the API returns no logs, which can skip delayed server-side logs; change the update so lastFetchedAt is only modified when the response contains logs: after calling useNuxtApp().$hubApi and logChunks.appendPolled(result), compute allLogs from result.nginx_logs and result.analysis_logs and only set lastFetchedAt to the max timestamp when allLogs.length > 0, otherwise leave lastFetchedAt unchanged; keep fetchTime and logChunks.appendPolled(result) as-is and ensure no assignment to lastFetchedAt occurs on empty result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/composables/useLogChunks.ts`:
- Around line 67-85: The code currently deduplicates both result.nginx_logs and
result.analysis_logs against a shared boundary (oldestTimestamp.value), which
can drop valid rows; change to deduplicate each stream against its own current
first-item timestamp (e.g., compute nginxBoundary =
nginxLines.value[0]?.timestamp and analysisBoundary =
analysisLines.value[0]?.timestamp) and use those when filtering
result.nginx_logs and result.analysis_logs; also determine hasOlder using the
raw page sizes (result.nginx_logs.length === CHUNK_SIZE ||
result.analysis_logs.length === CHUNK_SIZE) instead of the lengths after
deduplication, and keep updating oldestTimestamp.value via
oldestTimestampOf(...) from the deduped arrays as before.
---
Outside diff comments:
In `@app/components/analysis/logs/ContainerLogs.vue`:
- Around line 179-184: The two panes share the same prepend callback which
causes the non-triggering pane to jump; replace the single
logChunks.loadOlderChunk usage with two distinct callbacks (e.g.,
loadOlderNginxChunk and loadOlderAnalysisChunk or small wrapper functions) and
wire those to AnalysisLogCardContent's onLoadOlderNginx and onLoadOlderAnalysis
so each callback only prepends to its own array; ensure you still use
logChunks.hasOlder.value and logChunks.isLoading.value for each pane but update
the underlying loader functions to target nginx vs analysis chunks separately.
---
Duplicate comments:
In `@app/components/analysis/logs/ContainerLogs.vue`:
- Around line 100-116: The current polling logic advances the cursor to
fetchTime even when the API returns no logs, which can skip delayed server-side
logs; change the update so lastFetchedAt is only modified when the response
contains logs: after calling useNuxtApp().$hubApi and
logChunks.appendPolled(result), compute allLogs from result.nginx_logs and
result.analysis_logs and only set lastFetchedAt to the max timestamp when
allLogs.length > 0, otherwise leave lastFetchedAt unchanged; keep fetchTime and
logChunks.appendPolled(result) as-is and ensure no assignment to lastFetchedAt
occurs on empty result.
🪄 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: 57a6c1aa-0f87-49a9-9ed3-1491fb0f5a34
📒 Files selected for processing (3)
app/components/analysis/logs/AnalysisLogCardContent.vueapp/components/analysis/logs/ContainerLogs.vueapp/composables/useLogChunks.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