feat: add resolver for logging #1222
Conversation
WalkthroughThis pull request introduces a new log file management capability across the backend and frontend systems. The API now includes a new pub/sub channel and enhanced GraphQL schema for log queries and subscriptions. A fresh NestJS Logs module—with corresponding resolver and service classes—handles log file operations and real-time file watching. On the frontend, new Vue components integrate GraphQL queries, subscriptions, and a dedicated Unraid log viewer page. Additional configuration updates, dependency additions, and tests support the expanded functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant GQL as GraphQL Endpoint
participant R as LogsResolver
participant S as LogsService
C->>GQL: Send logFiles query
GQL->>R: Forward query to resolver
R->>S: Call listLogFiles()
S-->>R: Return list of log files
R-->>GQL: Return logFiles response
GQL-->>C: Deliver data
sequenceDiagram
participant C as Client
participant GQL as GraphQL Endpoint
participant R as LogsResolver
participant S as LogsService
participant FS as File System Watcher
C->>GQL: Subscribe to logFile updates
GQL->>R: Forward subscription
R->>S: Start watching log file
S->>FS: Initiate file watcher (chokidar)
FS-->>S: File update event
S->>R: Publish updated log content via pub/sub
R-->>GQL: Push update to subscriber
GQL-->>C: Deliver live update
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
api/src/unraid-api/graph/resolvers/logs/logs.module.ts (1)
1-9: Module structure looks good, but please fix formatting issues.The NestJS module structure follows conventions with correct providers and exports. However, the static analysis is flagging several formatting issues:
- Use absolute paths for imports
- Add a blank line after imports
- Fix indentation with proper spaces
- Add a newline at the end of the file
-import { Module } from '@nestjs/common'; -import { LogsResolver } from './logs.resolver.js'; -import { LogsService } from './logs.service.js'; +import { Module } from '@nestjs/common'; + +import { LogsResolver } from 'src/unraid-api/graph/resolvers/logs/logs.resolver.js'; +import { LogsService } from 'src/unraid-api/graph/resolvers/logs/logs.service.js'; @Module({ - providers: [LogsResolver, LogsService], - exports: [LogsService], + providers: [LogsResolver, LogsService], + exports: [LogsService], }) -export class LogsModule {} +export class LogsModule {}🧰 Tools
🪛 GitHub Check: Test API
[failure] 2-2:
Insert⏎
[failure] 2-2:
import statements should have an absolute path
[failure] 3-3:
import statements should have an absolute path
[failure] 6-6:
Insert··
[failure] 7-7:
Insert··
[failure] 9-9:
Replace·with⏎🪛 GitHub Check: Build API
[failure] 2-2:
Insert⏎
[failure] 2-2:
import statements should have an absolute path
[failure] 3-3:
import statements should have an absolute path
[failure] 6-6:
Insert··
[failure] 7-7:
Insert··
[failure] 9-9:
Replace·with⏎api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts (1)
15-33: Optional lines argument usage.
You might want to explicitly default thelinesargument to a sensible value in the GraphQL schema or handle edge cases (e.g., negative values) on the service layer.api/src/graphql/schema/types/logs/logs.graphql (1)
15-66: Consider renaming the subscription field for clarity.
Using a distinct field name likelogFileUpdatedmay help differentiate it from thelogFilequery.Here’s an example diff:
- logFile(path: String!): LogFileContent! + logFileUpdated(path: String!): LogFileContent!api/src/unraid-api/graph/resolvers/logs/logs.service.ts (2)
71-91: Potential performance concern with large files.
Reading and counting lines for very large log files may be slow. Caching or chunked reads could improve performance.
112-185: Watch for error scenarios in file watching.
The code correctly handles file truncation, but if a file is moved or locked while reading, consider additional logging or retry strategies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
api/src/graphql/generated/api/operations.tsis excluded by!**/generated/**api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (8)
api/src/core/pubsub.ts(1 hunks)api/src/graphql/schema/types/logs/logs.graphql(1 hunks)api/src/store/modules/paths.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/logs/logs.module.ts
[failure] 2-2:
Insert ⏎
[failure] 2-2:
import statements should have an absolute path
[failure] 3-3:
import statements should have an absolute path
[failure] 6-6:
Insert ··
[failure] 7-7:
Insert ··
[failure] 9-9:
Replace · with ⏎
api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts
[failure] 2-2:
Insert ⏎
[failure] 2-2:
import statements should have an absolute path
[failure] 5-5:
Insert ··
[failure] 7-7:
Insert ··
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/logs/logs.module.ts
[failure] 2-2:
Insert ⏎
[failure] 2-2:
import statements should have an absolute path
[failure] 3-3:
import statements should have an absolute path
[failure] 6-6:
Insert ··
[failure] 7-7:
Insert ··
[failure] 9-9:
Replace · with ⏎
api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts
[failure] 2-2:
Insert ⏎
[failure] 2-2:
import statements should have an absolute path
[failure] 5-5:
Insert ··
[failure] 7-7:
Insert ··
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (9)
api/src/core/pubsub.ts (1)
21-21: New LOG_FILE channel looks good!This addition aligns with the PR's purpose of adding log file management features.
api/src/store/modules/paths.ts (1)
63-63: Path addition looks good!Adding the 'unraid-log-base' path pointing to the standard Linux log location is appropriate for the log file management feature.
api/src/unraid-api/graph/resolvers/resolvers.module.ts (2)
22-23: Imports look good!
These additions properly integrate the new logging components.
46-47: Confirm whether to export logs functionality.
IfLogsResolverandLogsServiceare required by other modules, consider exporting them here for broader availability. Otherwise, this is fine.api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts (2)
1-13: Good overall structure for the new resolver.
The constructor injection ofLogsServiceappears clean and maintainable. Make sure there is test coverage for each method in this resolver.
35-61: Validate subscription teardown logic.
Overriding thereturnmethod effectively halts file monitoring, but please double-check concurrency scenarios if multiple subscriptions exist for the same file.api/src/graphql/schema/types/logs/logs.graphql (1)
1-13: Clear documentation for queries.
The docstring forlogFileclarifies how many lines default to 100 if none is provided, which is consistent with the implementation.api/src/unraid-api/graph/resolvers/logs/logs.service.ts (2)
28-35: Ensure secure path handling.
Usingbasenamehelps, but consider further checks to guard against path traversal or symbolic links that might lead outside the intended directory.
40-64: Efficient implementation for listing logs.
The approach is correct. Handling errors and returning an empty list is reasonable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts (1)
23-31: Consider validating the 'lines' parameter.While the implementation is clean, consider adding validation for the 'lines' parameter to ensure it's a positive number.
- async logFile(@Args('path') path: string, @Args('lines') lines?: number) { + async logFile(@Args('path') path: string, @Args('lines') lines?: number) { + if (lines !== undefined && lines <= 0) { + throw new Error('The lines parameter must be a positive number'); + } return this.logsService.getLogFileContent(path, lines); }api/src/unraid-api/graph/resolvers/logs/logs.service.ts (1)
207-232: Consider more efficient line counting for large files.The current implementation reads the entire file line by line, which could be inefficient for very large log files.
Consider using a more efficient approach for large files, such as reading in chunks and counting newlines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
api/src/unraid-api/graph/resolvers/logs/logs.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/src/unraid-api/graph/resolvers/logs/logs.module.ts
- api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts
- api/src/unraid-api/graph/resolvers/resolvers.module.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (13)
api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts (4)
5-7: Good import organization.The imports are well-organized, with a clear separation between external libraries and internal modules.
9-11: Clean resolver implementation with dependency injection.The resolver is properly decorated and uses constructor injection for the LogsService.
13-21: Permission protected query implementation looks good.The logFiles query is properly protected with permission checks and the implementation is straightforward.
33-61: Well-implemented subscription with proper resource cleanup.The subscription implementation is excellent. You've appropriately:
- Started watching the file
- Created the async iterator
- Overridden the return method to clean up resources
- Ensured proper error handling
This approach properly cleans up resources when clients disconnect from the subscription.
api/src/unraid-api/graph/resolvers/logs/logs.service.ts (9)
1-11: Good import organization and dependency management.The imports are well-organized, and you've properly imported the necessary modules for file operations.
12-23: Well-defined interfaces for log file data.The interfaces for
LogFileandLogFileContentare clear and comprehensive.
25-36: Good service initialization and configuration access.The injectable service is well-structured with proper logger initialization and a clean getter for the log base path.
38-65: Good error handling in the listLogFiles method.The implementation properly handles errors by logging them and gracefully returning an empty array instead of crashing.
67-94: Path validation is implemented correctly for security.Good job normalizing the path to prevent directory traversal attacks with
join(this.logBasePath, basename(path)).
96-109: Clean subscription channel management.The method correctly normalizes paths and initiates file watching only when needed.
111-191: Robust file watching implementation with good error handling.The file watching implementation handles both file growth and truncation scenarios well. The error handling is comprehensive with detailed logging.
193-205: Proper resource cleanup in stopWatchingLogFile.The method correctly closes watchers and removes them from the tracking map.
234-268: Reading last N lines may be optimized.Currently, every line is read to compute the final N lines. For large files, consider a reverse seek approach.
A more efficient approach would be to read from the end of the file in chunks and count backwards until the desired number of lines is reached.
private async readLastLines(filePath: string, lineCount: number): Promise<string> { - const totalLines = await this.countFileLines(filePath); - const linesToSkip = Math.max(0, totalLines - lineCount); - - return new Promise((resolve, reject) => { - let currentLine = 0; - let content = ''; - - const stream = createReadStream(filePath); - const rl = createInterface({ - input: stream, - crlfDelay: Infinity, - }); - - rl.on('line', (line) => { - currentLine++; - if (currentLine > linesToSkip) { - content += line + '\n'; - } - }); - - rl.on('close', () => { - resolve(content); - }); - - rl.on('error', (err) => { - reject(err); - }); - }); + return new Promise(async (resolve, reject) => { + try { + const { size } = await stat(filePath); + const fileBuffer = Buffer.alloc(size); + + // Open file for reading + const fileHandle = await open(filePath, 'r'); + + // Read the entire file + await fileHandle.read(fileBuffer, 0, size, 0); + await fileHandle.close(); + + // Convert buffer to string and split by newlines + const lines = fileBuffer.toString().split('\n'); + + // Get last N lines + const lastLines = lines.slice(-lineCount).join('\n'); + resolve(lastLines + (lastLines.endsWith('\n') ? '' : '\n')); + } catch (err) { + reject(err); + } + }); }Note: For extremely large files, you might want to implement a more sophisticated solution that reads chunks from the end of the file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
web/components/Logs/LogViewer.ce.vue (2)
30-38: Consider extracting utility functionThe
formatFileSizefunction is well-implemented but consider moving it to a shared utility file if it might be used elsewhere in the application.- // Format file size for display - const formatFileSize = (bytes: number): string => { - if (bytes === 0) return '0 Bytes'; - - const k = 1024; - const sizes = ['Bytes', 'KB', 'MB', 'GB']; - const i = Math.floor(Math.log(bytes) / Math.log(k)); - - return parseFloat((bytes / Math.pow(k, i)).toFixed(2)) + ' ' + sizes[i]; - };Consider creating a utility file:
// utils/format.ts export const formatFileSize = (bytes: number): string => { if (bytes === 0) return '0 Bytes'; const k = 1024; const sizes = ['Bytes', 'KB', 'MB', 'GB']; const i = Math.floor(Math.log(bytes) / Math.log(k)); return parseFloat((bytes / Math.pow(k, i)).toFixed(2)) + ' ' + sizes[i]; };Then import it:
import { formatFileSize } from '~/utils/format';
111-113: Empty style sectionThe style section contains only a comment about importing UI globals but doesn't actually import anything. If you're not using any custom styles, consider removing this section.
- <style lang="postcss"> - /* Import unraid-ui globals first */ - </style>api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts (1)
30-30: Potential extraneous whitespace or character detected.
Static analysis reported “Delete·” at line 30. Please verify and remove any hidden or trailing characters.- @Args('path') path: string, + @Args('path') path: string,🧰 Tools
🪛 GitHub Check: Build API
[failure] 30-30:
Delete·🪛 GitHub Check: Test API
[failure] 30-30:
Delete·api/src/graphql/schema/types/logs/logs.graphql (2)
24-47: Add an optional field for log file type or location metadata.
Currently,LogFilecontains basic file info. If multi-directory or different log categories are needed in the future, you may consider extending this type with optional metadata or tags to enhance sorting and filtering.
49-72: Check line count defaults for large files.
TheLogFileContentfields work fine for typical logs, but be mindful that returning large logs entirely might impact performance. Consider pagination or chunking for extremely large files, if necessary.api/src/unraid-api/graph/resolvers/logs/logs.service.ts (7)
68-108: Handle out-of-range values forlinesorstartLine.
getLogFileContent()should gracefully handle negative or zero line counts, as well asstartLinevalues exceeding the file size. This can prevent unexpected slices or empty content results.🧰 Tools
🪛 GitHub Check: Build API
[failure] 75-75:
Delete·
[failure] 76-76:
Delete·
[failure] 87-87:
Delete············
[failure] 100-100:
Insert,🪛 GitHub Check: Test API
[failure] 75-75:
Delete·
[failure] 76-76:
Delete·
[failure] 87-87:
Delete············
[failure] 100-100:
Insert,
125-205: Prevent potential memory overflow when files grow rapidly.
startWatchingLogFile()reads updates into a string. For extremely large bursts of new data, consider streaming chunk by chunk or limiting buffer sizes to reduce memory usage.
207-219: Add usage metrics instrumentation (optional).
stopWatchingLogFile()is a good housekeeping method. For logs with high churn, you might also track usage metrics or add scheduling logic to close watchers automatically after a certain period of inactivity.
221-246: Potential performance bottleneck for massive files.
countFileLines()iterates over all lines, which can be expensive for huge logs. If line counts are frequently requested, consider indexing or caching the line count if feasible.
248-282: Leverage a more efficient approach for reading last N lines.
Reading the entire file to retrieve the last lines can be slow for large logs. Consider a reverse seek or other specialized approach to read from the end without scanning the entire file.
284-330: Optimize partial reads from large files.
readLinesFromPosition()works but may still require linear parsing up tostartLine. If logs are extremely large, you could implement skip logic or approximate offsets.🧰 Tools
🪛 GitHub Check: Build API
[failure] 291-291:
Delete·
[failure] 292-292:
Delete·
[failure] 308-308:
Delete················🪛 GitHub Check: Test API
[failure] 291-291:
Delete·
[failure] 292-292:
Delete·
[failure] 308-308:
Delete················
75-76: Remove extra whitespace to fix pipeline lint failures.
The static analysis hints indicate that there are extraneous whitespace characters on these lines. Removing them ensures formatting compliance and avoids build failures.-·· - // or other extraneous spaces +// remove trailing or extra indentationAlso applies to: 87-87, 100-100, 291-292, 308-308
🧰 Tools
🪛 GitHub Check: Build API
[failure] 75-75:
Delete·
[failure] 76-76:
Delete·🪛 GitHub Check: Test API
[failure] 75-75:
Delete·
[failure] 76-76:
Delete·
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (10)
api/src/graphql/schema/types/logs/logs.graphql(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.service.ts(1 hunks)web/components/Logs/LogViewer.ce.vue(1 hunks)web/components/Logs/SingleLogViewer.vue(1 hunks)web/components/Logs/log.query.ts(1 hunks)web/components/Logs/log.subscription.ts(1 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(9 hunks)web/pages/index.vue(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts
[failure] 30-30:
Delete ·
api/src/unraid-api/graph/resolvers/logs/logs.service.ts
[failure] 75-75:
Delete ·
[failure] 76-76:
Delete ·
[failure] 87-87:
Delete ············
[failure] 100-100:
Insert ,
[failure] 291-291:
Delete ·
[failure] 292-292:
Delete ·
[failure] 308-308:
Delete ················
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts
[failure] 30-30:
Delete ·
api/src/unraid-api/graph/resolvers/logs/logs.service.ts
[failure] 75-75:
Delete ·
[failure] 76-76:
Delete ·
[failure] 87-87:
Delete ············
[failure] 100-100:
Insert ,
[failure] 291-291:
Delete ·
[failure] 292-292:
Delete ·
[failure] 308-308:
Delete ················
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (31)
web/components/Logs/log.subscription.ts (1)
1-11: Looks good: GraphQL subscription for log file updatesThe GraphQL subscription is well-structured and includes all necessary fields for tracking log file updates in real-time. This will work well with the pubsub system for delivering log file changes to the frontend.
web/components/Logs/log.query.ts (2)
1-12: Well-structured query for retrieving log filesThe
GET_LOG_FILESquery retrieves all necessary metadata for log files including name, path, size, and modification timestamp. This provides a complete picture for displaying log files in the UI.
14-23: Solid implementation of log file content queryThe
GET_LOG_FILE_CONTENTquery is well-designed with optional parameters for pagination (lines and startLine). This provides flexibility for viewing different portions of large log files.web/pages/index.vue (2)
11-11: Good component importThe LogViewerCe component is properly imported using the correct path.
187-191: Clean integration of Log Viewer componentThe Log Viewer component is integrated consistently with the existing UI patterns, maintaining the same structure as other component sections with appropriate headers and styling.
web/components/Logs/LogViewer.ce.vue (3)
17-21: Well-designed component stateThe reactive state for managing the log viewer is clearly defined with appropriate defaults. The auto-scroll default being enabled is a good choice for log viewing.
22-27: Effective GraphQL query integrationThe component correctly uses the Apollo composable to fetch log files while exposing loading and error states for proper UI handling.
83-107: Excellent error handling and conditional renderingThe component handles all possible states appropriately: loading, error, empty log files list, and no selection. This creates a robust user experience with clear feedback.
api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts (4)
1-4: Clean and well-structured imports.
Everything looks standard and appropriate for a NestJS GraphQL Resolver.
13-21: Query method uses appropriate authorization checks.
ThelogFilesquery method correctly delegates directory scanning toLogsService, and the use of@UsePermissionsis consistent with security best practices.
23-35: Consider handling invalid arguments forlogFile.
Ifpathis invalid or points to a non-existent file, ensureLogsService.getLogFileContentgracefully handles errors. You may want to wrap and rethrow with user-friendly messages.🧰 Tools
🪛 GitHub Check: Build API
[failure] 30-30:
Delete·🪛 GitHub Check: Test API
[failure] 30-30:
Delete·
37-65: Subscription logic is robust with cleanup.
Overriding theasyncIterator.returnmethod to stop file watching ensures resources are released on unsubscribe. Excellent approach to preventing resource leaks.web/components/Logs/SingleLogViewer.vue (7)
1-8: Imports and setup look correct.
All necessary composables and UI elements are imported, including DOMPurify for safe HTML rendering. Great job ensuring sanitized log output.
13-17: Prop definitions are explicit and typed.
DefininglogFilePath,lineCount, andautoScrollfosters clarity and helps prevent runtime errors.
19-28: State structure is comprehensive.
The reactivestateobject neatly tracks loading status, chunk data, and scrolling details to manage large log files effectively.
50-57: Smart scrolling method.
UsingnextTickto scroll after DOM updates helps ensure the viewport is scrolled correctly. This is a good approach for log viewers.
71-92: Real-time subscription approach is well done.
Appending new log content on subscription updates keeps the viewer in sync. No concurrency concerns are apparent.
139-166: Infinite scroll implementation.
Loading older content chunks on scroll ensures usability for large logs. The toggling ofcanLoadMoreandisLoadingMoreis carefully handled.
168-181: Effective refresh strategy.
Resetting chunks and refetching the log ensures the view stays consistent if the user clicks Refresh or the file changes drastically.web/composables/gql/graphql.ts (4)
600-624: Schema definitions for log files are clear.
DeclaringLogFileandLogFileContentwith fields for size, path, content, etc. is straightforward and well-documented.
1099-1108: Exposing new queries in the schema.
AddinglogFileandlogFilesto theQuerytype is a clean approach to retrieving logs.
1355-1359: Subscription fields integrate seamlessly.
The subscription forlogFilecomplementsQuery.logFile, enabling real-time updates on file changes. Good job matching the arguments with the query.Also applies to: 1390-1392
1799-1820: GraphQL operations for logs.
Queries and subscriptions (LogFilesQuery,LogFileContentQuery,LogFileSubscriptionSubscription) are well-typed with clear variables.Also applies to: 1946-1948
api/src/graphql/schema/types/logs/logs.graphql (2)
1-14: Consider validating query parameters in the resolver.
While this schema is clean, ensure in your resolver logic that user inputs (e.g., negative values forlinesor invalidstartLine) are properly handled or validated to avoid errors or inconsistent data.
16-22: Evaluate error handling for subscription-based user inputs.
Review your subscription resolver to confirm that it gracefully handles invalidpathinputs and other potential user errors, such as non-existent files or restricted access.api/src/unraid-api/graph/resolvers/logs/logs.service.ts (3)
26-38: Service encapsulation looks solid.
Defining a dedicatedLogsServiceis clear and maintains good separation of concerns.
39-66: Ensure consistent error handling strategy.
listLogFiles()returns an empty array on error, which is fine, but be sure the calling resolver or any higher-level logic is aware of this possibility and can surface it appropriately to clients.
110-123: Enforce path normalization.
getLogFileSubscriptionChannelnormalizes the path before watching. Double-check that errors and security constraints are handled, e.g., ifbasename(path)differs from user input.web/composables/gql/gql.ts (3)
16-19: Validate new queries in the generated types.
The new entries inDocumentsalign well with your newly added operations. Verify that these references accurately match the schema definitions and no further transformations are needed.
40-43: Ensure consistent usage of the newly added documents.
TheLogFiles,LogFileContent, andLogFileSubscriptionentries indocumentsappear correct. However, double-check that they’re used consistently in the consuming components.
79-90: Confirm the correctness of overload signatures.
The newly added function overloads forgraphqlproperly map query strings to typed documents. Make sure the extended approach for large scale queries or future expansions is workable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/components/Logs/LogViewer.ce.vue (1)
82-88: Minor suggestion for auto-detection approach.
Consider using a concise mapping to associate file extensions with languages for easier maintenance, rather than multiple conditionals.-const autoDetectLanguage = (filePath: string): string => { - const fileName = filePath.split('/').pop() || ''; - if (fileName.endsWith('.sh') || fileName.endsWith('.bash') || fileName.includes('syslog')) { - return 'bash'; - } else if (fileName.endsWith('.conf') || ...) - ... - return 'plaintext'; -}; +const extensionToLanguageMap: Record<string, string> = { + '.sh': 'bash', + '.bash': 'bash', + '.conf': 'ini', + '.ini': 'ini', + '.cfg': 'ini', + // ... +}; + +function autoDetectLanguage(filePath: string): string { + const fileName = filePath.split('/').pop() || ''; + for (const ext in extensionToLanguageMap) { + if (fileName.endsWith(ext) || fileName.includes(ext.replace('.', ''))) { + return extensionToLanguageMap[ext]; + } + } + return 'plaintext'; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
web/components/Logs/LogViewer.ce.vue(1 hunks)web/components/Logs/SingleLogViewer.vue(1 hunks)web/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (9)
web/package.json (1)
95-95: No immediate concerns with the new highlight.js dependency.
This library is well-suited for syntax highlighting.web/components/Logs/LogViewer.ce.vue (4)
1-36: Solid setup logic and references.
The reactive approach for selected files, line count, and auto-scroll is straightforward and maintainable.
37-80: Computed properties and helpers look great.
YourlogFilescomputation andformatFileSizeutility are well-organized and clear.
90-173: Comprehensive template structure.
The approach for displaying file selection, line count, syntax highlight options, and error states provides a good user experience.
175-179: Styles placeholder noted.
No issues here; the partial style block is understandable for future expansions.web/components/Logs/SingleLogViewer.vue (4)
1-43: Good setup for highlight.js and theming.
Registering languages and referencing the theme store for dark mode is a nice touch.
44-179: Robust GraphQL integration.
Using query + subscription ensures real-time updates. The chunk-based loading strategy is well-structured.
181-351: Effective syntax highlighting strategy.
Kudos for including sanitation (DOMPurify), error/warning highlighting, and advanced markers for IPs and timestamps. These additions significantly enhance readability.
353-572: Well-organized layout and custom themes.
Your light/dark theme CSS variables provide a clean, maintainable approach for color-coding logs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
web/components/Logs/SingleLogViewer.vue (6)
54-68: Consider initializing currentStartLine with a specific value.Instead of using
undefined, it might be clearer to initializecurrentStartLinewith a specific value likenullor1to make the intent more explicit.
180-247: Consider extracting highlight patterns to constants.The regex patterns used for highlighting timestamps and IP addresses could be extracted to named constants to improve readability and maintainability.
+// Regex patterns for log highlighting +const TIMESTAMP_PATTERN = /\b(\d{4}-\d{2}-\d{2}[T ]\d{2}:\d{2}:\d{2}(?:\.\d+)?(?:Z|[+-]\d{2}:?\d{2})?)\b/g; +const IP_ADDRESS_PATTERN = /\b(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})\b/g; +const ERROR_PATTERN = /\b(error|exception|fail|failed|failure)\b/gi; +const WARNING_PATTERN = /\b(warning|warn)\b/gi; +const SUCCESS_PATTERN = /\b(success|successful|completed|done)\b/gi; const highlightLog = (content: string): string => { try { // Determine which language to use for highlighting const language = props.highlightLanguage || defaultLanguage; // Apply syntax highlighting let highlighted = hljs.highlight(content, { language }).value; // Apply additional custom highlighting for common log patterns // Highlight timestamps (various formats) highlighted = highlighted.replace( - /\b(\d{4}-\d{2}-\d{2}[T ]\d{2}:\d{2}:\d{2}(?:\.\d+)?(?:Z|[+-]\d{2}:?\d{2})?)\b/g, + TIMESTAMP_PATTERN, '<span class="hljs-timestamp">$1</span>' ); // Highlight IP addresses highlighted = highlighted.replace( - /\b(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})\b/g, + IP_ADDRESS_PATTERN, '<span class="hljs-ip">$1</span>' );
208-235: Consider optimizing the highlighting logic.The current implementation tests each line multiple times with different regex patterns. This could be optimized by using a single pass approach or by checking for the most common patterns first.
// Process each line to add error, warning, and success highlighting lines = lines.map(line => { - if (/(error|exception|fail|failed|failure)/i.test(line)) { + // Get line type with a single regex test + const isError = ERROR_PATTERN.test(line); + const isWarning = !isError && WARNING_PATTERN.test(line); + const isSuccess = !isError && !isWarning && SUCCESS_PATTERN.test(line); + + if (isError) { // Highlight error keywords line = line.replace( - /\b(error|exception|fail|failed|failure)\b/gi, + ERROR_PATTERN, '<span class="hljs-error-keyword">$1</span>' ); // Wrap the entire line return `<span class="hljs-error">${line}</span>`; - } else if (/(warning|warn)/i.test(line)) { + } else if (isWarning) { // Highlight warning keywords line = line.replace( - /\b(warning|warn)\b/gi, + WARNING_PATTERN, '<span class="hljs-warning-keyword">$1</span>' ); // Wrap the entire line return `<span class="hljs-warning">${line}</span>`; - } else if (/(success|successful|completed|done)/i.test(line)) { + } else if (isSuccess) { // Highlight success keywords line = line.replace( - /\b(success|successful|completed|done)\b/gi, + SUCCESS_PATTERN, '<span class="hljs-success-keyword">$1</span>' ); // Wrap the entire line return `<span class="hljs-success">${line}</span>`; } return line; });
288-333: Consider adding a file size check before downloading.For very large log files, it might be helpful to warn users before downloading to prevent browser issues. Consider adding a size check or limiting the download size.
const downloadLogFile = async () => { if (!props.logFilePath || state.isDownloading) return; try { state.isDownloading = true; // Get the filename from the path const fileName = props.logFilePath.split('/').pop() || 'log.txt'; + // First check file size + const sizeResult = await client.query({ + query: GET_LOG_FILE_SIZE, // You would need to create this query + variables: { + path: props.logFilePath, + }, + fetchPolicy: 'network-only', + }); + + const fileSizeBytes = sizeResult.data?.logFile?.sizeBytes || 0; + const fileSizeMB = fileSizeBytes / (1024 * 1024); + + // Warn user if file is large + if (fileSizeMB > 10) { // 10MB threshold + const confirmDownload = window.confirm( + `The log file is ${fileSizeMB.toFixed(2)}MB in size. Do you want to continue with the download?` + ); + if (!confirmDownload) { + state.isDownloading = false; + return; + } + } + // Query for the entire log file content const result = await client.query({ query: GET_LOG_FILE_CONTENT,
82-82: Consider providing a default value for startLine.When
state.currentStartLineis undefined, it's passed directly to the GraphQL query. It might be clearer to use a nullish coalescing operator to provide a default value.- startLine: state.currentStartLine, + startLine: state.currentStartLine ?? undefined,
169-169: Consider explaining the purpose of the setTimeout delay.The 300ms timeout might not be immediately clear to someone reading the code. A comment explaining why this delay is necessary would improve maintainability.
- setTimeout(() => (state.canLoadMore = true), 300); + // Add a small delay before allowing more content to be loaded to prevent + // accidental double-loading and ensure the UI has stabilized + setTimeout(() => (state.canLoadMore = true), 300);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/LogViewer.page(1 hunks)web/components/Logs/SingleLogViewer.vue(1 hunks)web/nuxt.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (10)
web/nuxt.config.ts (1)
145-148: Custom element registration looks good.The new
UnraidLogViewercustom element is properly registered following the project's established pattern.plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/LogViewer.page (2)
1-4: Page metadata looks good.The menu placement, title, icon, and tag are properly defined for the Log Viewer page.
17-19: Well-structured component usage.The page correctly wraps the log viewer component with the i18n host for internationalization support.
web/components/Logs/SingleLogViewer.vue (7)
1-27: Well-structured imports and setup.The component has a clear organization of imports, properly separating framework utilities from custom components and domain-specific code.
28-43: Good language support for syntax highlighting.The component registers multiple languages for highlighting, which provides good coverage for various log file formats.
44-53: Props are well-defined with appropriate types.The TypeScript interface for props is clear and includes documentation comments. The optional highlightLanguage prop provides good flexibility.
98-142: Observer setup logic looks good.Good use of MutationObserver for tracking content changes and implementing auto-scroll functionality. The subscription setup is also well implemented.
259-285: Solid implementation of loading older content.The implementation correctly handles the loading state, preserves scroll position, and sets appropriate flags. Good use of async/await for handling asynchronous operations.
354-401: UI layout is well-structured and provides good user feedback.The template has a clear structure with appropriate loading indicators, error states, and user controls. Good use of infinite scrolling directive.
403-570: Well-organized CSS with theming support.The CSS uses variables effectively to support both light and dark themes. The highlighting styles are comprehensive and visually distinguishable.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts (2)
9-21: No additional error handling on query.
Currently, thelogFilesquery delegates error handling tolistLogFiles(). This is acceptable; however, if the service throws unexpected exceptions, the error may bubble up here and cause a generic error response. Consider adding a try/catch for more fine-grained control over errors and a clearer user-facing message.
23-35: Add optional input validation for large or invalid line parameters.
When clients request too many lines or pass invalid values forlinesorstartLine, it may degrade performance or cause edge-case errors. You could add basic bounds checks (e.g., no negative lines, upper limit constraints) to ensure stable behavior.api/src/unraid-api/graph/resolvers/logs/logs.service.ts (2)
42-66: Potential performance overhead when listing a large number of logs.
The current logic loops through every file in the directory and callsstaton each. For directories containing thousands of logs, this might be slow. Consider batching file stats or using OS-level aggregated info (if available) for better performance.
125-205: Multiple subscriptions handled by a single watcher.
If multiple watchers are unavoidably started for the same file, concurrency or double-publishing could occur. Alternatively, if you rely on one watcher per file, consider locking or carefully synchronizing the read and publish steps to avoid race conditions.web/components/Logs/SingleLogViewer.vue (4)
33-43: Consider optimizing language registrations with lazy-loading.
Registering all languages at once can increase the bundle size. Especially if users rarely view certain file types, consider dynamically importing only the languages needed at runtime.
57-65: Monitor memory usage when storing large logs in memory.
When dealing with very large logs, repeatedly storing chunks inloadedContentChunkscould grow quickly and impact memory usage. Consider an upper limit or periodic truncation if logs are extremely large.
174-177: Relying on a 300ms timeout for enablingcanLoadMorecan be brittle.
Using a timeout-based approach for re-enablingcanLoadMoremay fail in some edge cases. Consider hooking into a reliable event or a post-fetch callback to manage state changes more deterministically.
335-337: Enhance user feedback by replacing browser alerts with a UI-based error display.
Relying onalert()can disrupt the user flow and feels inconsistent with modern web UX patterns. Transitioning to a dedicated error toast or modal would improve user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
api/src/__test__/store/modules/paths.test.ts(2 hunks)api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.service.ts(1 hunks)web/components/Logs/SingleLogViewer.vue(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Test API
api/src/__test__/store/modules/paths.test.ts
[failure] 3-3:
Replace ⏎⏎import·{·store·}·from·'@app/store/index.js';⏎⏎⏎⏎ with import·{·store·}·from·'@app/store/index.js';
[failure] 3-3:
More than 1 blank line not allowed
[failure] 7-7:
More than 1 blank line not allowed
[failure] 40-40:
Insert ⏎
🪛 GitHub Check: Build API
api/src/__test__/store/modules/paths.test.ts
[failure] 3-3:
Replace ⏎⏎import·{·store·}·from·'@app/store/index.js';⏎⏎⏎⏎ with import·{·store·}·from·'@app/store/index.js';
[failure] 3-3:
More than 1 blank line not allowed
[failure] 7-7:
More than 1 blank line not allowed
[failure] 40-40:
Insert ⏎
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
api/src/__test__/store/modules/paths.test.ts (1)
34-34: LGTM! Addition of "unraid-log-base" path is appropriate.This addition correctly aligns with the new configuration path that was added for log file management, as mentioned in the PR objectives.
api/src/unraid-api/graph/resolvers/logs/logs.service.ts (1)
253-282: Reading the last N lines may be optimized.
This approach reads and counts every line from the beginning. As previously noted, a reverse seek or chunk-based approach could drastically reduce I/O for large logs.web/components/Logs/SingleLogViewer.vue (1)
104-109: Be aware of performance implications ofMutationObserverin large logs.
The observer will fire for every DOM change in the log content, which might impact performance heavily if logs grow or update frequently. Throttling or batching updates could help.
| @Subscription('logFile') | ||
| @UsePermissions({ | ||
| action: AuthActionVerb.READ, | ||
| resource: Resource.LOGS, | ||
| possession: AuthPossession.ANY, | ||
| }) | ||
| async logFileSubscription(@Args('path') path: string) { | ||
| // Start watching the file | ||
| this.logsService.getLogFileSubscriptionChannel(path); | ||
|
|
||
| // Create the async iterator | ||
| const asyncIterator = createSubscription(PUBSUB_CHANNEL.LOG_FILE); | ||
|
|
||
| // Store the original return method to wrap it | ||
| const originalReturn = asyncIterator.return; | ||
|
|
||
| // Override the return method to clean up resources | ||
| asyncIterator.return = async () => { | ||
| // Stop watching the file when subscription ends | ||
| this.logsService.stopWatchingLogFile(path); | ||
|
|
||
| // Call the original return method | ||
| return originalReturn | ||
| ? originalReturn.call(asyncIterator) | ||
| : Promise.resolve({ value: undefined, done: true }); | ||
| }; | ||
|
|
||
| return asyncIterator; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Watchers for multiple subscriptions may conflict.
Each new subscription starts a file watcher if one doesn't already exist. When the subscription ends, you unconditionally stop watching. This may disrupt other simultaneous subscribers to the same file. Consider implementing a reference counter or subscription tracking to ensure the file watcher remains active until all consumers have unsubscribed.
There was a problem hiding this comment.
I've fixed this inside of the stopWatchingLogFile function
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Let's examine the `stopWatchingLogFile` function in LogsService
cat api/src/unraid-api/graph/resolvers/logs/logs.service.ts | grep -A 30 "stopWatchingLogFile"
Length of output: 1309
@elibosley I see you've addressed the subscription issue by implementing a reference counting mechanism in the stopWatchingLogFile function. The function now decrements a counter and only closes the watcher when all subscribers have unsubscribed (count <= 0). This is a good solution that maintains the watcher for multiple concurrent subscribers.
✏️ Learnings added
Learnt from: elibosley
PR: unraid/api#1222
File: api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts:37-65
Timestamp: 2025-03-17T13:41:52.251Z
Learning: The LogsService in Unraid API uses a reference counting mechanism to track log file subscriptions, ensuring file watchers remain active until all subscribers have unsubscribed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts (1)
11-13:⚠️ Potential issueMissing LogsService dependency in test setup.
Based on the AI summary and previous review, the LogsResolver depends on LogsService, but it's not included in the test module. This will likely cause the test to fail as the resolver won't be able to be properly instantiated.
Update the test module to include a mock for LogsService:
const module: TestingModule = await Test.createTestingModule({ - providers: [LogsResolver], + providers: [ + LogsResolver, + { + provide: LogsService, + useValue: { + // Add mock implementations for service methods used by resolver + } + } + ], }).compile();Don't forget to add the import:
import { Test, TestingModule } from '@nestjs/testing'; import { beforeEach, describe, expect, it } from 'vitest'; import { LogsResolver } from '@app/unraid-api/graph/resolvers/logs/logs.resolver.js'; +import { LogsService } from '@app/unraid-api/graph/resolvers/logs/logs.service.js';
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts (2)
7-21: Test suite lacks coverage for resolver methods.The test suite only verifies that the resolver can be instantiated but doesn't test any of its actual functionality. Consider adding tests for each resolver method to ensure they work as expected.
Example test cases you might want to add:
- Tests for log file listing functionality
- Tests for log file content retrieval
- Tests for subscription handlers
For each test, you'll need to set up appropriate mocks for the LogsService methods and verify the resolver correctly processes and returns the data.
1-3: Separate imports by source type.Consider organizing imports by grouping them according to their source: external libraries first, then internal modules, and finally relative imports.
import { Test, TestingModule } from '@nestjs/testing'; - import { beforeEach, describe, expect, it } from 'vitest'; + import { LogsResolver } from '@app/unraid-api/graph/resolvers/logs/logs.resolver.js';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts (1)
5-5: Verify import file extension.The import statement includes the
.jsextension, which is unusual for TypeScript files. Make sure this is intentional and follows your project's module resolution configuration.If your project uses ESM modules with TypeScript, the
.jsextension might be necessary. Otherwise, you might want to remove it:-import { LogsResolver } from '@app/unraid-api/graph/resolvers/logs/logs.resolver.js'; +import { LogsResolver } from '@app/unraid-api/graph/resolvers/logs/logs.resolver';
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/src/unraid-api/graph/resolvers/logs/logs.service.ts (3)
45-69: Rethink silent failure approach.
listLogFiles()logs the error but returns an empty array, which can mask critical filesystem issues from upstream callers. Consider rethrowing or returning a specialized error value so callers can handle the situation.
137-219: Gracefully handle file rename or removal.
Currently, if the file is removed or renamed, you may continue watching a non-existent path. Monitoring forunlinkevents (or verifying file existence upon change events) could enhance robustness.
308-354: Consider a more efficient strategy for readLinesFromPosition.
While functional, scanning line-by-line from the start is expensive for largestartLinevalues. A partial seek or chunk-based approach may help optimize performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
api/src/unraid-api/graph/resolvers/logs/logs.service.ts (5)
1-11: Consider validating symlinks for added security.
While usingbasename(path)prevents direct directory traversal, attackers could create symlinks in your log directory that point outside. Consider resolving real paths or disallowing symlinks to strengthen security.Would you like a follow-up script to search for symbolic link usage in your codebase?
77-111: Ensure usage alignment when startLine exceeds total lines.
IfstartLineis larger than the total line count, the returnedcontentis empty. Verify this behavior is intended and that callers correctly handle edge cases.
117-135: Looks good for subscription management.
This approach to maintain watchers in a map with subscription counts is well-organized and prevents multiple watchers on the same file.
245-270: Reading the entire file to count lines can be expensive.
This was raised previously: for large files, consider a more optimal approach (e.g., reading in reverse).
272-306: Large-file performance caution (repeat suggestion).
LikecountFileLines,readLastLinesprocesses the entire file, which can become costly. A reverse-seek approach could significantly reduce I/O overhead.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/components/Logs/LogViewer.ce.vue (2)
56-80: Consider refactoring the language detection functionThe auto-detection logic works but could be more maintainable with a map-based approach rather than a long series of conditionals.
-const autoDetectLanguage = (filePath: string): string => { - const fileName = filePath.split('/').pop() || ''; - - if (fileName.endsWith('.sh') || fileName.endsWith('.bash') || fileName.includes('syslog')) { - return 'bash'; - } else if (fileName.endsWith('.conf') || fileName.endsWith('.ini') || fileName.endsWith('.cfg')) { - return 'ini'; - } else if (fileName.endsWith('.xml') || fileName.endsWith('.html')) { - return 'xml'; - } else if (fileName.endsWith('.json')) { - return 'json'; - } else if (fileName.endsWith('.yml') || fileName.endsWith('.yaml')) { - return 'yaml'; - } else if (fileName.includes('nginx')) { - return 'nginx'; - } else if (fileName.includes('apache') || fileName.includes('httpd')) { - return 'apache'; - } else if (fileName.endsWith('.js')) { - return 'javascript'; - } else if (fileName.endsWith('.php')) { - return 'php'; - } - - return 'plaintext'; -}; +const autoDetectLanguage = (filePath: string): string => { + const fileName = filePath.split('/').pop() || ''; + + const extensionMap: Record<string, string> = { + '.sh': 'bash', + '.bash': 'bash', + '.conf': 'ini', + '.ini': 'ini', + '.cfg': 'ini', + '.xml': 'xml', + '.html': 'xml', + '.json': 'json', + '.yml': 'yaml', + '.yaml': 'yaml', + '.js': 'javascript', + '.php': 'php' + }; + + const keywordMap: Record<string, string> = { + 'syslog': 'bash', + 'nginx': 'nginx', + 'apache': 'apache', + 'httpd': 'apache' + }; + + // Check for file extension matches + for (const [ext, lang] of Object.entries(extensionMap)) { + if (fileName.endsWith(ext)) { + return lang; + } + } + + // Check for keyword matches in filename + for (const [keyword, lang] of Object.entries(keywordMap)) { + if (fileName.includes(keyword)) { + return lang; + } + } + + return 'plaintext'; +};
110-120: Consider adding validation for line countWhile you have min and max attributes on the input, consider adding explicit validation to ensure the value remains within bounds even when manually edited.
<div> <Label for="line-count">Lines</Label> <Input id="line-count" - v-model.number="lineCount" + v-model.number="lineCount" + @update:modelValue="val => lineCount = Math.min(1000, Math.max(10, val))" type="number" min="10" max="1000" class="w-24" /> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
web/components/Logs/LogViewer.ce.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (8)
web/components/Logs/LogViewer.ce.vue (8)
17-21: State management looks goodThe component state is well-defined with appropriate defaults. The use of Vue refs for reactive state management follows best practices.
23-35: Good selection of highlight languagesThe list of supported syntax highlighting languages covers most common log file formats. This provides good flexibility for users.
37-42: Apollo query implementation is correctGood use of Apollo composables with proper handling of loading state, error state, and results.
45-53: File size formatting implementation is solidThe utility function handles all edge cases and properly formats file sizes with appropriate units.
82-87: Good use of watch effectThe watch effect for auto-detecting language is well implemented and properly handles selection changes.
90-144: Well-structured UI controlsThe header section is well-organized with appropriate controls for file selection, line count, syntax highlighting, and auto-scroll options.
146-172: Good handling of different statesExcellent job implementing conditional rendering for various states (loading, error, empty, etc.). This provides clear feedback to the user.
175-180: Styles are properly importedGood practice to import UI framework styles before custom styles.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web/components/Logs/LogViewer.ce.vue (4)
44-53: Consider Supporting Larger File Sizes
YourformatFileSizefunction currently stops at gigabytes. If there's a scenario where logs can exceed gigabytes, you might want to extend support to terabytes or beyond.
55-80: Refine Filename Matching for Language Detection
You are using partial substring checks (e.g.fileName.includes('apache')). Consider a stricter approach like checking file extensions or known patterns to avoid false matches.
97-107: Match 'for' and 'id' for Better Accessibility
You have<Label for="log-file-select">but haven't definedid="log-file-select"on the corresponding<Select>. Consider adding anidto improve screen reader support.
151-153: Review Error Message Exposure
Rendering the full error message can inadvertently disclose stack traces or internal details in production. Verify that this is the intended behavior, or sanitize the error content if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
web/components/Logs/LogViewer.ce.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
web/components/Logs/LogViewer.ce.vue (1)
1-13: Solid Composition API Setup
This section cleanly sets up Composition API with TypeScript and uses Apollo integrators in a straightforward manner. Good job!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
web/components/Logs/LogViewer.ce.vue (3)
25-37: Consider externalizing highlight languages for maintainability.Listing them directly in the code can be unwieldy if you add more syntaxes or need to update labels. Extracting them into a shared config or utility can make the code more scalable.
125-135: Reconsider the line count limits for heavy log usage.Restricting the user to 1,000 lines might be too limiting in some scenarios. Allowing dynamic pagination or higher limits can enhance usability for large logs.
187-194: Consider partial loading or pagination for huge logs.Loading the entire file at once can lead to performance and memory issues with very large logs. Introducing pagination or smaller chunk retrieval will improve responsiveness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
web/components/Logs/LogViewer.ce.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
web/components/Logs/LogViewer.ce.vue (1)
61-86: Add more patterns or fallback for unrecognized file extensions.Currently, files like “.txt” or “.log” default to
plaintext, which may not be optimal for scenarios involving specialized logs. Please verify whether you expect other file types and adjust the detection logic accordingly.
elibosley
force-pushed
the
feat/logs-in-gql
branch
from
23132b4 to
f3f84e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web/components/Logs/LogViewer.ce.vue (4)
1-1: Consider adding a test suite for the new LogViewer component.
While the code appears solid, implementing a small integration or end-to-end test would help ensure reliability, especially when future changes impact log viewing functionality.Do you want me to open a new issue or draft a test example for you?
52-60: Consider handling file sizes larger than 1 GB.
Your currentformatFileSizefunction doesn’t explicitly account for TB or PB scenarios, which might be relevant if logs grow significantly.
63-87: Add explicit handling for.logextensions if desired.
A.logfile currently defaults toplaintext. If most logs have a.logextension, you may consider mapping them to a specialized syntax.
111-122: Potentially refine for large log file lists.
If the number of log files grows, consider pagination or lazy loading to maintain responsiveness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
web/components/Logs/LogViewer.ce.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
web/components/Logs/LogViewer.ce.vue (4)
14-15: Kudos on properly importinguseTeleport.
This resolves the past reference error concern and ensures the dropdown can be teleported without runtime issues.
90-94: Reconfirm auto-detect behavior vs. user choice.
Auto-detecting the highlight language on file change might override a user’s custom selection. Ensure this is intended.
97-101: Teleport usage looks correct.
This approach prevents rendering issues for the dropdown in certain contexts.
200-204: Global styles appear well-integrated.
Including@unraid/ui/stylesand your base CSS fosters consistency across the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
web/components/Logs/LogViewer.ce.vue (2)
62-87: Consider adding.logextension detection.
The auto-detection logic currently skips.logfiles. If you often handle.logextensions, consider mapping them to'plaintext'or a relevant language.} else if (fileName.endsWith('.log')) { return 'plaintext'; }
110-122: Guard against no matching file in the dropdown.
IfselectedLogFileis no longer present inlogFiles, you might want to resetselectedLogFileor handle it gracefully.web/components/Logs/SingleLogViewer.vue (2)
124-134: Potential large in-memory usage.
Appending new lines tolastChunk.contentcan build a large string if logs are very long. Consider a chunked approach or a maximum buffer size to prevent excessive memory usage.
295-337: Replacealertwith a more consistent UI notification.
Using a plainalertmight disrupt the UX. A toast or an inline error message might be more user-friendly.- alert(`Error downloading log file: ${error instanceof Error ? error.message : String(error)}`); + // Example toast or inline error approach + // showToast(`Error downloading log file: ${error instanceof Error ? error.message : String(error)}`, 'error');web/composables/gql/graphql.ts (1)
610-611: Check if 32-bit integer is sufficient for file size.
Thesizefield is declared as anInt, but some logs could exceed the 2GB boundary. Consider using a 64-bit type (e.g.,Long) for more robust support of large log files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (3)
api/src/graphql/generated/api/operations.tsis excluded by!**/generated/**api/src/graphql/generated/api/types.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
api/package.json(4 hunks)api/src/__test__/store/modules/paths.test.ts(1 hunks)api/src/core/pubsub.ts(1 hunks)api/src/graphql/schema/types/logs/logs.graphql(1 hunks)api/src/store/modules/paths.ts(1 hunks)api/src/unraid-api/auth/user.decorator.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)api/src/unraid-api/main.ts(1 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/LogViewer.page(1 hunks)web/components/Logs/LogViewer.ce.vue(1 hunks)web/components/Logs/SingleLogViewer.vue(1 hunks)web/components/Logs/log.query.ts(1 hunks)web/components/Logs/log.subscription.ts(1 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(7 hunks)web/nuxt.config.ts(1 hunks)web/package.json(1 hunks)web/pages/index.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- api/src/test/store/modules/paths.test.ts
- web/components/Logs/log.subscription.ts
- web/components/Logs/log.query.ts
- api/src/unraid-api/graph/resolvers/logs/logs.module.ts
- api/src/unraid-api/main.ts
- api/src/core/pubsub.ts
- web/pages/index.vue
- api/src/unraid-api/graph/resolvers/resolvers.module.ts
- plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/LogViewer.page
- api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts
- web/nuxt.config.ts
- web/package.json
- api/src/store/modules/paths.ts
- api/src/unraid-api/auth/user.decorator.ts
- api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts
- api/package.json
- api/src/graphql/schema/types/logs/logs.graphql
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (14)
web/components/Logs/LogViewer.ce.vue (2)
14-15: Thanks for importinguseTeleportproperly!
Everything looks correct, and it addresses the issue mentioned in the past review about reference errors.
96-101: Teleport usage is good.
Your logic of callingdetermineTeleportTarget()on open ensures the dropdown sees a valid teleport root.web/components/Logs/SingleLogViewer.vue (1)
90-107: Good approach for auto-scroll via MutationObserver.
This ensures the user stays at the bottom when new lines arrive. No issues found.web/composables/gql/graphql.ts (1)
1800-1820: GraphQL queries and subscriptions look accurate.
These additions align well with the new log file schema objects. Good job!api/src/unraid-api/graph/resolvers/logs/logs.service.ts (7)
45-69: Clean implementation of file listingThe
listLogFilesmethod looks well-implemented with proper error handling and clear object mapping.
77-111: Path validation is correctly implementedGood use of
joinandbasenameto validate the path is within the log directory. This prevents path traversal vulnerabilities.
136-219: Good resource management for file watchersThe implementation properly handles file watchers with subscription counting and cleanup. The chokidar configuration with the
awaitWriteFinishoptions helps prevent partial file reads during active writes.
225-243: Well-designed cleanup mechanismThe reference counting approach for file watchers ensures resources are properly cleaned up when no more clients are subscribed.
277-306: Reading last N lines may be optimizedCurrently, the entire file is read twice: once to count lines and then again to read the last N lines. For large log files, this approach will be inefficient.
Consider implementing a more efficient approach using a reverse reading strategy or a bounded buffer:
private async readLastLines(filePath: string, lineCount: number): Promise<string> { - const totalLines = await this.countFileLines(filePath); - const linesToSkip = Math.max(0, totalLines - lineCount); - - return new Promise((resolve, reject) => { - let currentLine = 0; - let content = ''; - - const stream = createReadStream(filePath); - const rl = createInterface({ - input: stream, - crlfDelay: Infinity, - }); - - rl.on('line', (line) => { - currentLine++; - if (currentLine > linesToSkip) { - content += line + '\n'; - } - }); - - rl.on('close', () => { - resolve(content); - }); - - rl.on('error', (err) => { - reject(err); - }); - }); + return new Promise(async (resolve, reject) => { + try { + // Get file size + const fileStats = await stat(filePath); + if (fileStats.size === 0) { + return resolve(''); + } + + // Use a circular buffer to store the last N lines + const buffer = new Array(lineCount); + let lineCounter = 0; + + const stream = createReadStream(filePath); + const rl = createInterface({ + input: stream, + crlfDelay: Infinity, + }); + + rl.on('line', (line) => { + buffer[lineCounter % lineCount] = line; + lineCounter++; + }); + + rl.on('close', () => { + // Reconstruct content from circular buffer + let content = ''; + const linesRead = Math.min(lineCounter, lineCount); + const startIndex = lineCounter < lineCount ? 0 : lineCounter % lineCount; + + for (let i = 0; i < linesRead; i++) { + const index = (startIndex + i) % lineCount; + content += buffer[index] + '\n'; + } + + resolve(content); + }); + + rl.on('error', (err) => { + reject(err); + }); + } catch (error) { + reject(error); + } + }); }
314-354: Efficient implementation for reading from positionThe
readLinesFromPositionmethod efficiently stops reading once it has read the required number of lines.
38-40:✅ Verification successful
Verify the path configuration exists
Make sure the 'unraid-log-base' path configuration exists in the system. If it doesn't, this service will fail to function properly.
🏁 Script executed:
#!/bin/bash # Check if the unraid-log-base path is defined and accessible rg -A 5 "unraid-log-base"Length of output: 1396
Path configuration verified
The 'unraid-log-base' configuration is confirmed—it is defined inapi/src/store/modules/paths.tsasunraid-log-base: resolvePath('/var/log/' as const). This ensures that the service inlogs.service.tsworks as expected.web/composables/gql/gql.ts (3)
17-19: GraphQL operations for log files properly definedThe GraphQL queries and subscription for log files are correctly defined with all necessary fields.
41-43: Document mapping is correctly implementedThe documents mapping correctly connects the GraphQL query strings to their respective TypeScript types.
79-90: GraphQL function overloads for log operationsThe GraphQL function overloads for log operations are correctly implemented, ensuring proper type safety.
elibosley
force-pushed
the
feat/logs-in-gql
branch
from
f3f84e9 to
b13b209
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
web/components/Logs/LogViewer.ce.vue (3)
52-60: Suggest adding unit tests forformatFileSize.Although the helper function correctly computes file sizes for typical scenarios, consider adding basic unit tests to verify edge cases (e.g. extremely large numbers, small fractional values, etc.).
62-87: Consider expanding auto-detection logic for additional extensions.Right now,
autoDetectLanguagecovers many file types but misses others (e.g..py). If relevant to your application, you could easily extend or parameterize the detection to handle more cases in the future.
104-198: Add test coverage for user interactions and error states.The template properly handles loading, errors, empty logs, and no-file selection. However, it’s best to capture these flows in your end-to-end or component tests to ensure no regressions.
Would you like help drafting a quick test suite for these states?
web/components/Logs/SingleLogViewer.vue (3)
44-49: Ensure prop validations forlogFilePathandlineCount.Currently, the component expects these props but doesn't enforce validations (e.g. non-empty strings for
logFilePath, numeric range forlineCount). Consider using runtime or compile-time prop validations to catch invalid usage.
186-254: Performance caution with large log content and highlighting.While the chunk-based approach mitigates some memory concerns, repeated calls to syntax highlighting for very large logs could be costly. If logs can be huge, you may need additional pagination or streaming solutions to prevent a large memory footprint and slow rendering.
336-337: Replace bare alerts with a custom error notification.Using
alert()is functional but can disrupt the user experience. A dedicated notification component or toast system might better integrate with the rest of your UI and preserve the user’s context.web/composables/gql/graphql.ts (1)
1160-1168: Allow for optional filters or pagination if logs grow in quantity.Currently,
logFilesreturns all files without pagination. If the number of log files is large, adding filtering or pagination arguments may improve performance for queries over time.api/src/unraid-api/graph/resolvers/logs/logs.service.ts (2)
45-69: Ensure large directory handling is efficient.
Listing all files in the directory, followed by individually callingstaton each file, may be expensive if the directory contains many files. For large log directories, consider batching I/O operations or implementing a pagination approach to avoid potential performance bottlenecks.
249-270: Optimize line-counting for large files.
Reading an entire file line-by-line for counting can be time-consuming for large logs. Consider a chunk-based approach or optional OS-level utilities for faster results—especially on frequently accessed logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (3)
api/src/graphql/generated/api/operations.tsis excluded by!**/generated/**api/src/graphql/generated/api/types.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
api/src/__test__/store/modules/paths.test.ts(1 hunks)api/src/core/pubsub.ts(1 hunks)api/src/graphql/schema/types/logs/logs.graphql(1 hunks)api/src/store/modules/paths.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts(1 hunks)api/src/unraid-api/graph/resolvers/logs/logs.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/LogViewer.page(1 hunks)web/components/Logs/LogViewer.ce.vue(1 hunks)web/components/Logs/SingleLogViewer.vue(1 hunks)web/components/Logs/log.query.ts(1 hunks)web/components/Logs/log.subscription.ts(1 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(10 hunks)web/nuxt.config.ts(1 hunks)web/package.json(1 hunks)web/pages/index.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- api/src/test/store/modules/paths.test.ts
- api/src/unraid-api/graph/resolvers/logs/logs.module.ts
- web/package.json
- api/src/unraid-api/graph/resolvers/resolvers.module.ts
- api/src/store/modules/paths.ts
- api/src/core/pubsub.ts
- plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/LogViewer.page
- web/components/Logs/log.query.ts
- api/src/unraid-api/graph/resolvers/logs/logs.resolver.spec.ts
- web/nuxt.config.ts
- api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts
- web/pages/index.vue
- web/components/Logs/log.subscription.ts
- api/src/graphql/schema/types/logs/logs.graphql
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (10)
web/components/Logs/LogViewer.ce.vue (1)
14-15: Good job importing and leveraginguseTeleport.Importing
useTeleportfrom@unraid/uiensures the select dropdown can properly display in teleported contexts. This resolves previous discussion about missing imports and avoids runtime errors.web/components/Logs/SingleLogViewer.vue (1)
99-110: Confirm the observer lifecycle and avoid potential memory leaks.A
MutationObserveris created and observed on mount, but only disconnected on unmount. This design is correct; just confirm all possible mount/unmount cycles are properly handled (e.g. if the component is reused or dynamically mounted multiple times).web/composables/gql/graphql.ts (2)
651-675: Schema additions forLogFileandLogFileContentlook good.Defining fields like
modifiedAtandsizeensures you have all the required file metadata. This clear approach is suitable for log file management.
1872-1891: GraphQL document definitions are coherent and align with the schema.The queries and subscription definitions match the shape of the newly introduced types, ensuring consistency between the client and server.
api/src/unraid-api/graph/resolvers/logs/logs.service.ts (3)
141-219: Consider awaiting the watcher initialization.
this.startWatchingLogFile(normalizedPath);is invoked withoutawait. If subsequent operations rely on the watcher being fully ready, a race condition may occur. Ensure either that you don’t need to rely on immediate readiness or consider usingawait this.startWatchingLogFile(...).
225-243: Confirm proper watcher cleanup.
Stopping the watcher is contingent on subscription counts reaching zero. This is a sound approach, but verify that all flows callinggetLogFileSubscriptionChannelalso callstopWatchingLogFilewhen done, to prevent resource leaks.
277-306: [Duplicate of previous comment] Consider a more performant approach for reading the last N lines.web/composables/gql/gql.ts (3)
19-21: Good addition for listing log files.
The newLogFilesquery helps retrieve log metadata efficiently. No issues found here.
45-47: Maintain consistent naming.
The operation names (LogFiles,LogFileContent,LogFileSubscription) are straightforward. Ensure they remain aligned with the backend resolvers for clarity.
91-102: Overloads align well with existing patterns.
The typed overloads for new queries and subscription fit seamlessly into the existing pattern. This ensures type safety and clarity.
| // Load older log content | ||
| const loadMoreContent = async () => { | ||
| if (state.isLoadingMore || state.isAtTop || !state.canLoadMore) return; | ||
| state.isLoadingMore = true; | ||
| state.canLoadMore = false; | ||
| const firstChunk = state.loadedContentChunks[0]; | ||
| if (firstChunk) { | ||
| const newStartLine = Math.max(1, firstChunk.startLine - DEFAULT_CHUNK_SIZE); | ||
| state.currentStartLine = newStartLine; | ||
| const prevScrollHeight = scrollViewportRef.value?.scrollHeight || 0; | ||
| await refetchLogContent(); | ||
| nextTick(() => { | ||
| if (scrollViewportRef.value) { | ||
| scrollViewportRef.value.scrollTop += scrollViewportRef.value.scrollHeight - prevScrollHeight; | ||
| } | ||
| }); | ||
| if (newStartLine === 1) { | ||
| state.isAtTop = true; | ||
| state.canLoadMore = false; | ||
| } | ||
| } | ||
| }; | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify chunk size with user-defined line counts.
You currently use props.lineCount for the initial fetch but a hardcoded DEFAULT_CHUNK_SIZE of 100 for subsequent loads. For a more consistent user experience, consider making these values sync or configurable.
Here’s a quick snippet illustrating a unified approach:
-const DEFAULT_CHUNK_SIZE = 100;
...
- const newStartLine = Math.max(1, firstChunk.startLine - DEFAULT_CHUNK_SIZE);
+const fetchChunkSize = computed(() => props.lineCount || 100);
...
+ const newStartLine = Math.max(1, firstChunk.startLine - fetchChunkSize.value);📝 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.
| // Load older log content | |
| const loadMoreContent = async () => { | |
| if (state.isLoadingMore || state.isAtTop || !state.canLoadMore) return; | |
| state.isLoadingMore = true; | |
| state.canLoadMore = false; | |
| const firstChunk = state.loadedContentChunks[0]; | |
| if (firstChunk) { | |
| const newStartLine = Math.max(1, firstChunk.startLine - DEFAULT_CHUNK_SIZE); | |
| state.currentStartLine = newStartLine; | |
| const prevScrollHeight = scrollViewportRef.value?.scrollHeight || 0; | |
| await refetchLogContent(); | |
| nextTick(() => { | |
| if (scrollViewportRef.value) { | |
| scrollViewportRef.value.scrollTop += scrollViewportRef.value.scrollHeight - prevScrollHeight; | |
| } | |
| }); | |
| if (newStartLine === 1) { | |
| state.isAtTop = true; | |
| state.canLoadMore = false; | |
| } | |
| } | |
| }; | |
| // Assuming this is at the top of your script or within the component setup: | |
| // Remove or comment out the hardcoded constant if it exists | |
| // const DEFAULT_CHUNK_SIZE = 100; | |
| const fetchChunkSize = computed(() => props.lineCount || 100); | |
| // Load older log content | |
| const loadMoreContent = async () => { | |
| if (state.isLoadingMore || state.isAtTop || !state.canLoadMore) return; | |
| state.isLoadingMore = true; | |
| state.canLoadMore = false; | |
| const firstChunk = state.loadedContentChunks[0]; | |
| if (firstChunk) { | |
| - const newStartLine = Math.max(1, firstChunk.startLine - DEFAULT_CHUNK_SIZE); | |
| + const newStartLine = Math.max(1, firstChunk.startLine - fetchChunkSize.value); | |
| state.currentStartLine = newStartLine; | |
| const prevScrollHeight = scrollViewportRef.value?.scrollHeight || 0; | |
| await refetchLogContent(); | |
| nextTick(() => { | |
| if (scrollViewportRef.value) { | |
| scrollViewportRef.value.scrollTop += scrollViewportRef.value.scrollHeight - prevScrollHeight; | |
| } | |
| }); | |
| if (newStartLine === 1) { | |
| state.isAtTop = true; | |
| state.canLoadMore = false; | |
| } | |
| } | |
| }; |
| async getLogFileContent( | ||
| path: string, | ||
| lines = this.DEFAULT_LINES, | ||
| startLine?: number | ||
| ): Promise<LogFileContent> { | ||
| try { | ||
| // Validate that the path is within the log directory | ||
| const normalizedPath = join(this.logBasePath, basename(path)); | ||
|
|
||
| // Count total lines | ||
| const totalLines = await this.countFileLines(normalizedPath); | ||
|
|
||
| let content: string; | ||
|
|
||
| if (startLine !== undefined) { | ||
| // Read from specific starting line | ||
| content = await this.readLinesFromPosition(normalizedPath, startLine, lines); | ||
| } else { | ||
| // Read the last N lines (default behavior) | ||
| content = await this.readLastLines(normalizedPath, lines); | ||
| } | ||
|
|
||
| return { | ||
| path: normalizedPath, | ||
| content, | ||
| totalLines, | ||
| startLine: startLine !== undefined ? startLine : Math.max(1, totalLines - lines + 1), | ||
| }; | ||
| } catch (error: unknown) { | ||
| this.logger.error(`Error reading log file: ${error}`); | ||
| throw new Error( | ||
| `Failed to read log file: ${error instanceof Error ? error.message : String(error)}` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate path safety and handle large files carefully.
While you defensively normalize the path using basename, validate whether the log directory might contain symbolic links or unexpected file types that could break assumptions. Additionally, for large files, calling countFileLines on each request can be expensive. You might want to consider caching line counts or chunk-based reading if performance becomes an issue.
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Introduced a comprehensive Log Viewer accessible from the web interface and Unraid management, allowing users to easily view, refresh, and download log files. - Enabled real-time log updates with auto-scroll functionality for seamless monitoring. - Enhanced log display with syntax highlighting and detailed file metadata for improved readability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [4.2.0](v4.1.3...v4.2.0) (2025-03-18) ### Features * add resolver for logging ([#1222](#1222)) ([2d90408](2d90408)) * connect settings web component ([#1211](#1211)) ([653de00](653de00)) * improve local dev with install path ([#1221](#1221)) ([32c5b0a](32c5b0a)) * split plugin builds ([4d10966](4d10966)) * swap to absolute paths for css ([#1224](#1224)) ([6f9fa10](6f9fa10)) * update theme application logic and color picker ([#1181](#1181)) ([c352f49](c352f49)) * use patch version if needed on update check ([#1227](#1227)) ([6ed46b3](6ed46b3)) ### Bug Fixes * add INELIGIBLE state to ConfigErrorState enum ([#1220](#1220)) ([1f00212](1f00212)) * **api:** dynamix notifications dir during development ([#1216](#1216)) ([0a382ca](0a382ca)) * **api:** type imports from generated graphql types ([#1215](#1215)) ([fd02297](fd02297)) * **deps:** update dependency @nestjs/schedule to v5 ([#1197](#1197)) ([b1ff6e5](b1ff6e5)) * **deps:** update dependency @vueuse/core to v12 ([#1199](#1199)) ([d8b8339](d8b8339)) * fix changelog thing again ([2426345](2426345)) * fix invalid path to node with sh execution ([#1213](#1213)) ([d12448d](d12448d)) * load tag correctly ([acd692b](acd692b)) * log errors ([629feda](629feda)) * one-command dev & web env files ([#1214](#1214)) ([8218fab](8218fab)) * re-release fixed ([bb526b5](bb526b5)) * recreate watcher on path change ([#1203](#1203)) ([5a9154e](5a9154e)) * update brand loading variants for consistent sizing ([#1223](#1223)) ([d7a4b98](d7a4b98)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
2 participants
Summary by CodeRabbit