feat: adds scroll up for multiple container views 🥳

[amir20]

No description provided.

[claude]

Review

Critical Issues:

1. Empty log handling (line 263) - No check for existingLogs.length === 0 before accessing array elements

   • existingLogs[0].date at lines 232, 243 will throw if empty
   • Add guard: if (existingLogs.length === 0) return;

2. Division by zero (line 238) - containers.value.length could be 0

   • Causes Infinity for minPerContainer
   • Should validate containers array before entering branch

3. Container ID missing (line 224) - No guarantee logs have containerID

   • Map may be empty even with logs present
   • Could result in loading duplicate ranges

Bugs:

• Line 243: Fallback existingLogs[0].date already fails if empty (see Support for TLS docker sockets over TCP #1)
• Line 246: Creating computed refs inside .map() - new reactivity for each call, potential memory leak

Performance:

• Line 240: Promise.all() good for parallel fetching
• Consider adding early return if containers.value.length === 0

Code Quality:

• Duplicated logic with single-container branch (lines 202-221)
• Extract delta calculation (lines 232-234 vs 205-207)

Missing:

• No tests for multi-container scroll-up path
• No error handling if loadBetween rejects for subset of containers

eventStreams.ts:197,222,232,238,243,246,263

[claude]

Review: Scroll up for multiple container views

Critical Issues:

• Line 238: Division by zero risk when containers.value.length === 0
  • Check removed at line 108 allows empty array to proceed
  • Should add: if (containers.value.length === 0) return;

Bugs:

• Line 243: Fallback logic may use wrong timestamp
  • Uses existingLogs[0].date when container has no logs yet
  • First request for container could fetch duplicate time range

Logic Issues:

• Lines 223-230: Map tracks earliest log per container but misses edge case
  • If container has lifecycle events only (ContainerEventLogEntry), no containerID set
  • Map will be empty for that container → fallback to wrong timestamp

Suggestions:

• Line 238: Guard against empty containers array before division
• Line 243-244: Consider different fallback strategy (e.g., use delta from global range)
• Line 225: Add check for log entry type or ensure all entries have containerID

eventStreams.ts:238, eventStreams.ts:243

[claude]

Code Review

Critical Issues:

• Race condition (eventStreams.ts:245): Parallel loadBetween() calls can complete out-of-order. If Container A finishes after Container B, older logs may appear after newer ones. Sort merged results by date.

• Empty results edge case (eventStreams.ts:262): When allNewLogs.length === 0, loader remains but no feedback shown. Consider removing loader or showing "no more logs" message.

Bugs:

• Inconsistent lastSeenId tracking (eventStreams.ts:226): When container has no logs in current view (!earliestByContainer.has(c.id)), lastSeenId is undefined. Backend may return duplicates. Fallback to -1 or track separately.

Logic Issues:

• Inefficient search (eventStreams.ts:225-232): O(n*m) loop searches all existingLogs for each container. Break early only when all containers found, but worst-case still processes entire log list. Pre-index by containerID for O(n) performance.

Minor:

• Duplicate computation (eventStreams.ts:233-234): firstDate/lastDate recalculate same values as loop above. Reuse from earliestByContainer.

Code Quality:

• Nested if-else (line 202-270) deeply indented. Extract multi-container logic to separate function loadOlderLogsForMultiple().

• Magic number 300 appears twice (lines 206, 238). Extract constant MAX_LOGS_FOR_DELTA.

Test Coverage:

• No tests for new multi-container scroll logic. Add test cases for:
  • Parallel load with different result sizes
  • Empty earliestByContainer scenarios
  • Out-of-order completion handling

[claude]

PR Review: Scroll up for multiple container views

Critical Issues:

1. Missing multi-container URL handling (loadBetween.ts:22)

   • loadBetween() still uses single-container URL pattern
   • For multi-container views, this will fail since there's no single container context
   • Need URL construction that supports multiple containers or rework the function signature

2. Error handling silent failures (logLoader.ts:33, 78, 98, 120)

   • All catch blocks just log to console without user feedback
   • Users won't know if load failed vs. no logs available

3. Race condition potential (logLoader.ts:46-51)

   • earliestByContainer loop has early break at line 45, but continues checking size
   • If not all containers have logs in existingLogs, some containers will have undefined earliest dates (line 59)

Performance:

• Line 70: Flattening + sorting all logs from Promise.all could be expensive with many containers
• Consider using a merge-sort approach for pre-sorted container logs

Code quality:

• Good refactoring extracting useLogLoader composable
• parseMessage extraction improves reusability
• Type safety maintained with explicit return types (loadBetween.ts:47)

Testing:

• No test coverage visible for multi-container scroll scenarios
• Consider edge cases: empty containers, varying log volumes per container

[claude]

Code Review

Critical Issues:

1. Race condition in multi-container scroll-up (logLoader.ts:38-83)

   • earliestByContainer map may not have entries for all containers if logs are sparse
   • Missing containers will use fallback date but with undefined lastSeenId
   • Could lead to duplicate log entries or incorrect ordering

2. Empty logs array edge case (logLoader.ts:19-23, 86-89)

   • No validation that existingLogs[0] exists before accessing .date and .id
   • Will throw if LoadMoreLogEntry is the only message
   • Add: if (existingLogs.length === 0) return;

3. Fetch error handling incomplete (loadBetween.ts:45)

   • Bare fetch without checking response.ok
   • Network errors or 4xx/5xx responses will fail silently
   • Add: if (!response.ok) throw new Error("HTTP " + response.status);

Performance:

• Promise.all for parallel container fetches is good (logLoader.ts:57, 106)
• Consider skipping containers with no visible logs in earliestByContainer to reduce API calls

Minor:

• parseMessage (loadBetween.ts:5) now exported but only used internally - consider keeping private
• Type annotation as LogEntry<LogMessage>[] (loadBetween.ts:48) redundant given return type

Good patterns:

• Clean separation into loadBetween/logLoader composables
• Proper abort signal handling
• Sort-merge strategy for multi-container logs

[claude]

Critical Issues

logLoader.ts:23 - Potential null pointer when existingLogs is empty:

const id = log.containerID || containerIDs.values().next().value\!;

If no containers exist in the set, .next().value is undefined but you use \! assertion.

logLoader.ts:42 - Fallback breaks when existingLogs is empty:

const to = earliest?.date ?? existingLogs[0].date;

This will throw if no logs exist for a container yet existingLogs is checked empty at line 14.

logLoader.ts:81 - loadSkippedLogs uses lastSeenId from entry but applies it to ALL containers. This breaks multi-container skipped log loading since each container needs its own lastSeenId.

loadBetween.ts:48 - Type annotation redundant, already inferred from return type.

Other Notes

• Good refactoring separating concerns into dedicated composables
• Multi-container parallel fetching looks correct (logLoader.ts:39-52)
• Delta calculation fallback to 60s seems reasonable (logLoader.ts:45)

File references: logLoader.ts:23, logLoader.ts:42, logLoader.ts:81, loadBetween.ts:48

[claude]

Review Summary

Critical bugs to fix:

• logLoader.ts:81 - loadSkippedLogs passes single lastSeenId from entry.lastSkippedLog.id to all containers — this is container-specific and will produce incorrect results in multi-container views. Needs per-container lastSeenId lookup similar to loadOlderLogs.

• logLoader.ts:43 - existingLogs[0].date throws if existingLogs is empty (only LoadMoreLogEntry in messages). Add guard: if (existingLogs.length === 0) return;

• loadBetween.ts:44-45 - No response.ok check before calling .text(). HTTP 4xx/5xx will silently produce empty/error text and be mishandled.

Minor:

• logLoader.ts:21: containerIDs.values().next().value! non-null assertion is unsafe; value can be undefined if set is empty.
• Good refactor overall — separating loadBetween and useLogLoader into dedicated composables is cleaner. Multi-container parallel fetching with Promise.all is correct.

[claude]

Review

Critical bugs:

• logLoader.ts:42: existingLogs[0].date will throw when existingLogs is empty (only LoadMoreLogEntry in messages). The early-return guard at line 13 checks containers.value.length but not existingLogs.length. Add: if (existingLogs.length === 0) return;

• logLoader.ts:80-81: lastSeenId from entry.lastSkippedLog.id is a per-container numeric ID applied to all containers in loadSkippedLogs. For container B, the ID belonging to container A is semantically wrong — backend deduplication will behave incorrectly. Needs per-container lastSeenId lookup like loadOlderLogs does.

• loadBetween.ts:44-45: No response.ok check before .text(). HTTP 4xx/5xx responses will silently produce error text that gets parsed as log lines.

Minor:

• logLoader.ts:23: Logs from LoadMoreLogEntry/SkippedLogsEntry (containerID = "") fall into the fallback branch and get attributed to an arbitrary container. Filter non-data entries before the loop.