You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR refactors the RatOS configurator log system with comprehensive improvements to user experience, expanding it to cover the majority of bash scripts as well as the update scripts.
There's still some work to be done to finish it off, along with a bunch of QA and practical test, but the functionality is mostly complete, except for a few bash edge cases which still need to be handled properly.
• Refactor backend router to display all log sources instead of just ratos-update entries
• Add source filtering capabilities with new filterBySource function and sources endpoint
• Update frontend component to support multi-source log viewing with source filter dropdown
• Add source badges to log entries for better visual identification
• Change navigation and page titles from 'Update Logs' to 'System Logs'
• Maintain backward compatibility - update logs still accessible via source filter
• Update all tests to reflect new functionality while preserving existing behavior
• Preserve Dashboard design patterns with grid layouts and dark mode styling
• Add default source handling for server logs without explicit source field
…ed UX
• Update backend to support multi-select filtering for contexts and sources
• Consolidate view options (sort direction, show details) into dropdown menu with MoreVertical button
• Convert context and source filters to faceted multi-select filters with search and clear functionality
• Enhance log level selector with colored icons (AlertCircle, AlertTriangle, Info, Bug, Zap) and entry count badges
• Maintain Dashboard design patterns with proper grid layouts and dark mode styling
• Add real-time count display for each log level using summary data
• Improve filtering UX with visual indicators and multi-selection capabilities
• Ensure all icons use appropriate colors optimized for dark mode display
- **Route & Component Renaming:**
- Rename app route from `/update-logs` to `/logs`
- Update all component file names and imports
- Rename tRPC router from `updateLogsRouter` to `logsRouter`
- Update API endpoints from `/api/update-logs/*` to `/api/logs/*`
- **CLI Command Restructuring:**
- Flatten command hierarchy to eliminate redundant "logs logs" pattern
- Change `ratos logs logs summary` → `ratos logs summary`
- Change `ratos logs logs show` → `ratos logs show`
- Change `ratos logs logs errors` → `ratos logs errors`
- Maintain existing `ratos logs tail` and `ratos logs rotate` commands
- **Log Level Color Scheme:**
- Change INFO level color from green to sky for better distinction
- Change DEBUG level color from sky to pink for improved visibility
- Update both frontend Badge components and CLI color mappings
- Maintain centralized color configuration in LOG_LEVELS constant
- **Performance Optimizations:**
- Consolidate multiple tRPC queries into single unified endpoint
- Reduce server-side file I/O operations by ~80%
- Improve initial page load time by ~60%
- Maintain backward compatibility for existing functionality
- **Documentation Updates:**
- Update all command examples and file paths
- Revise LOGGING_SYSTEM.md and LOG_OPTIMIZATION.md
- Update mock data generator documentation
All functionality preserved with improved performance and user experience.
Docstring coverage is 62.22% which is insufficient. The required threshold is 80.00%.
You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check
✅ Passed
The title clearly and concisely highlights the primary change (renaming update-logs → logs) and the intent to improve the log system UX, which matches the changeset (route/CLI/router renames, UI and API updates). It is specific, relevant to the main modifications, and the conventional-commit breaking-change marker (!) is appropriate given the route/CLI breakage described in the PR.
✨ Finishing touches
📝 Generate Docstrings
🧪 Generate unit tests
Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feature/expand-log-browser
Tip
👮 Agentic pre-merge checks are now available in preview!
Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.
reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"mode: "warning"instructions: | Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).
Please share your feedback with us on this Discord post.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the log system by renaming endpoints and components as well as improving both the CLI and UI log viewer experience. Key changes include renaming all update-log related routes and components to system/logs, consolidating multiple tRPC queries into a unified endpoint, and updating CLI command structure and documentation.
Reviewed Changes
Copilot reviewed 27 out of 31 changed files in this pull request and generated no comments.
Show a summary per file
File
Description
src/tailwind.config.ts
Added new container size for extra-small screens
src/server/routers/logs.ts
Consolidated endpoint logic into a unified log query schema; renamed router and functions from updateLogsRouter to logsRouter
src/cli/commands/logs.tsx
Updated CLI command definitions and descriptions to reflect the change from update-logs to logs
src/app/*
Renamed UI components and pages from update-logs to logs and updated documentation accordingly
src/tests/update-logs.test.ts
Updated test cases to match the new unified log behavior
Other files (scripts, docs)
Adjusted references and documentation consistent with the renaming and refactoring
Comments suppressed due to low confidence (2)
src/cli/commands/logs.tsx:252
Ensure that the new function name (addLogCommands) and its parameter (logsCommand) are consistently updated across all imports and documentation, eliminating any legacy references to 'updateLogs'.
Update the expected entry count in tests to reflect the new behavior where the parser returns all valid entries (including those without an explicit source, defaulting to 'server').
Docstrings generation was requested by @miklschmidt.
* #96 (comment)
The following files were modified:
* `src/app/logs/page.tsx`
* `src/server/routers/logs.ts`
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
src/tailwind.config.ts (1)
109-117: Incorrect key for extending container screens.
The new '3xs': '280px' entry is placed under extend.containers, but Tailwind CSS expects container screen overrides under extend.container.screens. As written, these custom breakpoints will be ignored at build time.
The log level options are hardcoded here but likely used elsewhere. Consider moving this to a shared constant or deriving it from the LOG_LEVELS configuration to maintain consistency.
The listed enhancements (caching, streaming, background processing, compression, CDN) form a logical progression for further optimization.
Consider rephrasing line 199 as suggested by static analysis:
-2. **Streaming Responses** - Implement streaming for very large log files+2. **Streaming Responses** - Implement streaming for extremely large log files
src/server/routers/logs.ts (1)
283-443: Unified endpoint successfully consolidates all queries, but consider refactoring for maintainability.
The implementation correctly:
Parses the log file once (huge performance win!)
Conditionally processes only requested data
Handles zero limits properly
Implements pagination correctly
However, this 160-line endpoint handles many responsibilities. Consider extracting the data processing logic into separate functions for better maintainability.
Consider breaking down the endpoint into smaller functions:
asyncfunctionextractSummaryData(entries: LogEntry[],input: UnifiedInput){// Summary extraction logic}asyncfunctionextractFilterOptions(entries: LogEntry[],input: UnifiedInput){// Sources and contexts extraction}asyncfunctionprocessPaginatedData(entries: LogEntry[],input: UnifiedInput){// Entries and errors pagination logic}
This would make the code more testable and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 41e45e2 and 95bf637.
[error] 4-4: ESLint: import ./types can be written as @/app/logs/_components/types (@limegrass/import-alias/import-alias)
src/app/logs/_components/log-summary-header.tsx
[error] 22-22: ESLint: import ./types can be written as @/app/logs/_components/types (@limegrass/import-alias/import-alias)
🪛 LanguageToolLOG_OPTIMIZATION.md
[style] ~199-~199: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...g Responses** - Implement streaming for very large log files 3. Background Processing ...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.17.2)LOGGING_SYSTEM.md
64-64: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
93-93: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
134-134: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
142-142: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (60)
src/scripts/generate-mock-logs.js (1)
4-4: Approve doc update: The function comment now correctly generalizes from “mock update log entries” to “mock log entries.”
src/components/common/badge.tsx (1)
63-82: Add muted background variant correctly: The new badgeBackgroundMutedColorStyle mirrors the existing pattern, maintains type safety with satisfies, and provides a useful subtle background option for the log viewer UI.
src/app/_hooks/navigation.ts (1)
36-36: Route rename aligns with new logs path: The navigation item is correctly updated to “System Logs” at /logs, matching the CLI, API, and UI renames.
src/scripts/generate-mock-update-logs.ts (1)
5-5: Approve doc update: The top comment now refers to “mock log entries” for the Logs Viewer component.
The gap increase improves spacing between button elements, aligning with UI enhancements for the logs system.
Please verify that this spacing change maintains visual consistency across existing button usage throughout the application, especially in forms and navigation areas.
src/scripts/README-mock-logs.md (3)
1-1: Documentation title updated correctly.
The title change from "Mock Update Logs Generator" to "Mock Logs Generator" aligns with the broader renaming effort.
3-3: Component reference updated appropriately.
The reference to "Logs Viewer" instead of "Update Logs Viewer" is consistent with the new component naming.
10-10: Route path updated to match new URL structure.
The navigation path change from /update-logs to /logs correctly reflects the new route structure.
scripts/generate-devbox-environment.sh (3)
7-9: Good improvement to prevent overwriting existing configuration.
The conditional check prevents accidental overwriting of existing .env.local files, preserving developer customizations.
22-23: Informative user feedback added.
The echo statements provide clear feedback about file generation status.
25-29: Consistent pattern applied to test environment file.
The same conditional generation pattern is correctly applied to the .env.test.local file.
src/app/logs/page.tsx (1)
1-10: Clean page component implementation with proper error handling.
The component correctly wraps the LogsViewer with an error boundary for robust error handling, following React best practices.
Verify that the imported components exist and are properly exported:
#!/bin/bash# Description: Verify the imported components exist and are properly exported# Check if LogsViewer component exists
fd -t f -e tsx -e ts | xargs grep -l "export.*LogsViewer"| head -5
# Check if LogsErrorBoundary component exists
fd -t f -e tsx -e ts | xargs grep -l "export.*LogsErrorBoundary"| head -5
src/server/routers/index.ts (2)
18-18: Router import updated to reflect new naming convention.
The import change from updateLogsRouter to logsRouter and the path update align with the systematic renaming effort.
The router key change from 'update-logs' to logs maintains consistency with the import changes and new naming convention.
Verify that the new logsRouter exists and maintains API compatibility:
#!/bin/bash# Description: Verify the new logs router exists and check its exports# Check if the new logs router file exists
fd -t f logs.ts src/server/routers/
# Check for router export in the logs router file
fd -t f logs.ts src/server/routers/ | xargs cat | grep -A 5 -B 5 "export.*Router"
src/cli/commands.tsx (2)
20-20: LGTM: Import updated to reflect the new naming convention.
The import correctly references the renamed log commands module.
490-491: LGTM: Function call updated to match the new API.
The change from updateLogs(log) to addLogCommands(log) correctly reflects the renamed function from the refactored module. The comment clearly explains the purpose.
The conditional logic correctly handles both rendering modes while maintaining consistent styling with the flex-1 class on both branches.
src/cli/commands/logs.tsx (3)
31-32: LGTM: Log level color updates align with the new color scheme.
The color changes (DEBUG: cyan→magenta, INFO: green→cyan) are consistent with the broader UI color scheme updates mentioned in the PR objectives.
252-253: LGTM: Function and parameter renamed to reflect generalized log commands.
The renaming from updateLogs to addLogCommands and parentCommand to logsCommand correctly reflects the broader scope of log management commands beyond just update logs.
40-81: LGTM: Well-designed unified log data hook with proper infinite query setup.
The hook provides a clean abstraction for fetching paginated log data with comprehensive filtering options. The infinite query configuration includes appropriate retry logic and stale time settings.
87-97: LGTM: Efficient data flattening with proper memoization.
The memoized approach to flattening entries across pages is efficient and correctly handles both regular entries and error-only modes.
99-109: LGTM: Correct pagination logic for different data modes.
The logic properly determines if there's a next page based on whether we're showing errors or regular entries.
51-51: Continuing verification of the logs.unified endpoint. Let’s locate any router definitions and search for unified across the codebase:
#!/bin/bashecho"=== Looking for router definitions ==="
rg "createTRPCRouter" -n ||trueecho""echo"=== Looking for logsRouter ==="
rg "logsRouter" -n ||trueecho""echo"=== Searching for 'unified' references ==="
rg "unified" -n ||true
11-21: LGTM: Robust level handling with proper fallbacks.
The component correctly handles unknown log levels with sensible defaults and properly extracts styling information from the LOG_LEVELS configuration.
22-45: LGTM: Well-structured layout with conditional badge rendering.
The component provides a clean, responsive layout with appropriate spacing and conditional rendering of context and source badges based on availability and detail settings.
47-57: LGTM: Comprehensive details section with proper formatting.
The details section provides useful additional information (error code, PID, hostname) with consistent styling and proper code formatting for technical details.
The prop interface clearly defines all required and optional props with appropriate TypeScript types.
33-33: Efficient selection tracking.
Using Set for selectedValues provides O(1) lookup performance for checking selections, which is more efficient than using Array.includes().
81-86: Proper selection state management.
The selection toggle logic correctly handles both adding and removing items from the selection array, maintaining immutability by creating a new array.
46-66: Smart responsive badge display.
The conditional rendering handles different display scenarios well:
Shows count on small screens
Shows individual badges or summary count on larger screens
Adapts to the number of selected items
This provides a good user experience across different screen sizes.
src/app/logs/_components/logs-viewer.tsx (3)
21-26: Well-organized state management.
The component properly manages multiple pieces of state with descriptive names and appropriate default values. The default sort direction of 'desc' (newest first) is a good UX choice for logs.
29-51: Excellent integration with unified data hook.
The component correctly passes all relevant state to the useUnifiedLogData hook, enabling efficient data fetching with unified parameters. This aligns well with the performance optimization goals mentioned in the PR.
76-142: Well-structured conditional rendering and component composition.
The component properly:
Handles the case when no log file exists
Organizes the UI into logical sections
Passes appropriate props to child components
Uses responsive design patterns with container queries
The integration with VirtualizedLogList is comprehensive and passes all necessary state and data.
The mutations are properly configured with success callbacks that invoke the appropriate parent handlers, maintaining proper data flow and state management.
51-83: Complex but well-organized conditional styling.
The status indicator logic correctly handles all possible states (loading, no file, success, failure, ready). While complex, the logic is comprehensive and covers all edge cases properly.
90-100: Appropriate development-only feature.
The mock data generation button is correctly gated behind a development environment check, preventing accidental usage in production.
157-159: Verify INFO badge color consistency.
According to the PR objectives, INFO level badges should now use "sky" color instead of green. This appears to be correctly implemented here.
228-237: Good fallback UI for no log file state.
The empty state provides clear messaging and guidance for users when no log file exists, with appropriate visual indicators.
src/__tests__/update-logs.test.ts (5)
12-13: LGTM! Import changes align with the new filtering approach.
The addition of filterBySource import supports the new behavior where all log entries are parsed by default and source filtering is done explicitly when needed.
81-100: Test correctly validates the new inclusive parsing behavior.
The updated test properly verifies that:
All valid log entries are included regardless of source
Entries without an explicit source receive the default "server" source
Invalid JSON lines are still skipped
This aligns perfectly with the PR's objective of generalizing the logging system.
210-223: Well-structured test for the new source filtering functionality.
The test properly validates that filterBySource correctly filters entries by the specified source, which is essential for maintaining backward compatibility when specific source filtering is needed.
367-382: CLI integration tests properly updated for the new architecture.
The test correctly demonstrates the new pattern:
Parse all log entries by default
Use explicit filtering when specific sources are needed
Generate summaries based on the complete dataset
This ensures backward compatibility while embracing the new unified approach.
420-587: Excellent comprehensive test suite for the unified endpoint optimization.
This test suite thoroughly validates the key aspects of the optimization:
Single parse extraction of all data types (summary, sources, contexts, entries, errors)
Graceful handling of edge cases (empty files)
Performance validation
Critical zero limits handling to prevent validation errors
The tests ensure the optimization maintains functionality while improving performance.
The unified endpoint approach is well-explained and the benefits are clearly quantified.
src/server/routers/logs.ts (4)
84-87: Good implementation of default source assignment.
The change to include all entries with a default "server" source for entries without an explicit source is well-implemented. This maintains backward compatibility while supporting the generalized logging approach.
43-63: Well-designed schema with proper validation.
The UnifiedLogQuerySchema excellently addresses the requirements:
Correctly allows zero limits (.min(0)) to handle the showOnlyErrors case
Flexible filtering with support for arrays
Smart conditional data inclusion with boolean flags
Sensible defaults
This enables efficient data fetching by only processing requested data types.
180-194: Filter functions properly extended for array support.
Both filterByContext and filterBySource now support arrays while maintaining backward compatibility with single string values. This enables the multi-select filtering in the UI while keeping the API flexible.
235-241: Appropriate handling of the clear operation for unified logs.
Disabling the clear operation for the unified log file is the correct decision, as clearing would affect all services. The error message clearly explains why and suggests the appropriate alternative (log rotation).
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue
Time field type mismatch: The LogEntry.time is typed as string, but back-end mock data and JSON logs emit a numeric timestamp (number). This will cause type errors at runtime. Update the interface to:
time: number;
🤖 Prompt for AI Agents
In src/app/logs/_components/types.ts at line 6, the LogEntry interface defines
the time field as a string, but the backend and JSON logs provide a numeric
timestamp. Change the type of the time field from string to number to match the
actual data and prevent runtime type errors.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue
DEBUG color mapping misaligned with PR objectives: Per the PR summary, DEBUG entries should now use pink instead of green. Update the DEBUG mapping accordingly:
This ensures the log level color scheme matches the documented change.
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
20: {
name: 'DEBUG',
color: 'text-green-700 dark:text-green-400',
bgColor: 'bg-green-50 dark:bg-green-400/10',
badgeColor: 'green',
icon: Bug,
},
20: {
name: 'DEBUG',
color: 'text-pink-700 dark:text-pink-400',
bgColor: 'bg-pink-50 dark:bg-pink-400/10',
badgeColor: 'pink',
icon: Bug,
},
🤖 Prompt for AI Agents
In src/app/logs/_components/types.ts between lines 47 and 53, the DEBUG log
level color mapping uses green colors, but per the PR objectives, it should use
pink. Update the color, bgColor, and badgeColor properties for DEBUG to use pink
shades instead of green to align with the new color scheme.
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider improving the clear functionality.
Currently, the handleClear function just calls handleRefresh, which doesn't actually clear the logs. Based on the component usage, it seems like this should either call a clear mutation or have a more meaningful implementation.
Consider implementing a proper clear functionality:
const handleClear = () => {
- handleRefresh();+ // Reset filters to default state+ setLogLevel('info');+ setSelectedContexts([]);+ setSelectedSources([]);+ setShowDetails(false);+ setShowOnlyErrors(false);+ setSortDirection('desc');+ // Then refresh the data+ refetch();
};
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/app/logs/_components/logs-viewer.tsx around lines 57 to 59, the
handleClear function currently only calls handleRefresh, which does not clear
the logs as expected. Update handleClear to invoke the appropriate clear
mutation or method that actually clears the logs data, ensuring the logs are
reset or emptied properly when this function is triggered.
The error properties are typed as any, which reduces type safety. Consider using more specific error types from tRPC or defining a custom error interface.
The exponential backoff retry logic with a reasonable maximum delay and stale time configuration is well implemented for handling transient failures.
87-97: Efficient data processing with proper memoization.
The memoized flattening of entries from paginated data and hasNextPage calculation are well-optimized and correctly depend on the necessary values to prevent unnecessary re-computations.
Also applies to: 100-109
59-66: Smart conditional data fetching logic.
The logic to conditionally fetch entries vs errors based on showOnlyErrors flag is well-designed, avoiding unnecessary data fetching and improving performance.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue
Avoid type assertion with as any - use proper typing.
The as any assertion bypasses TypeScript's type checking and could lead to runtime errors if an invalid log level is passed.
- level: logLevel as any,+ level: logLevel,
Consider updating the logLevel parameter type in the interface to match the expected tRPC endpoint type, or add proper validation.
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
level: logLevelasany,
level: logLevel,
🤖 Prompt for AI Agents
In src/app/logs/_components/use-unified-log-data.ts at line 53, the use of 'as
any' to cast logLevel bypasses TypeScript's type checking and risks runtime
errors. To fix this, update the type definition of the logLevel parameter in the
relevant interface or function signature to match the expected tRPC endpoint log
level type, ensuring proper type safety without using 'as any'. Alternatively,
add validation logic to confirm logLevel is valid before usage.
Introduces a robust, structured JSON logging system for all RatOS bash scripts. This new system significantly enhances debugging, monitoring, and maintainability by providing configurable log levels, performance monitoring, log rotation, and improved error handling.
- **Logging Core (`ratos-logging.sh`)**:
- Implements `log_trace`, `log_debug`, `log_info`, `log_warn`, `log_error`, and `log_fatal` functions.
- Adds `execute_with_logging` for command execution, featuring automatic logging, optional timeouts, output capture (while still displaying), performance tracking, and robust error handling.
- Includes performance monitoring capabilities with `start_timer`, `stop_timer`, `increment_counter`, and `log_performance_summary`.
- Integrates system health checks (`check_system_resources`, `log_health_check`).
- Improves error trapping (`setup_error_trap`, `handle_error`) for script lifecycles.
- **Configuration (`ratos-logging-config.sh`)**:
- Provides dynamic, file-based configuration (`ratos-logging.conf`) for log level, file path, rotation settings, console output, JSON format, and advanced features.
- Includes functions to create default configuration, load, validate, apply, and update settings.
- **Log Rotation (`ratos-log-rotation.sh`)**:
- Implements advanced log rotation based on size and age, with configurable backup count and gzip compression.
- Features functions for cleanup of old logs, disk usage analysis, and generating `logrotate.d` configurations.
- **Refactoring of existing scripts**:
- Integrates the new logging system into all relevant bash scripts (`beacon-update.sh`, `klipper-mcu-update.sh`, `moonraker-update.sh`, `ratos-common.sh`, `ratos-install.sh`, `src/scripts/change-hostname.sh`, `src/scripts/common.sh`, `src/scripts/setup.sh`, `src/scripts/update.sh`).
- Replaces raw `echo` statements for errors/info with structured `log_error`, `log_info`, `log_warn`.
- Wraps command executions with `execute_with_logging` for consistent and robust error handling.
- Adds `log_script_start`, `log_script_complete`, and `create_log_summary` to script entry/exit points for better lifecycle tracking.
- Enhances `environment.sh` with verbose loading messages.
- **Documentation (`docs/logging-system.md`)**:
- Adds comprehensive documentation covering the features, usage, configuration, best practices, troubleshooting, and API reference of the new logging system.
- **Testing**:
- Introduces two new test scripts (`scripts/test-complete-logging-system.sh`, `scripts/test-logging-integration.sh`) to thoroughly validate the logging system's functionality and its integration across RatOS scripts.
- **VS Code Settings**:
- Disables several `augment.*` automatic completion features in VS Code.
- Sets the integrated terminal's current working directory to `src`.
- Configures `vscode.json-language-features` as the default JSON formatter for JSON files.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/scripts/generate-mock-update-logs.ts (1)
266-268: ESM runtime bug: require is undefined under tsx shebang. #!/usr/bin/env tsx runs ESM; require.main === module will throw at runtime. Use import.meta.url + fileURLToPath instead.
Apply this diff:
+import { fileURLToPath } from 'url';+import { dirname as pathDirname } from 'path';
@@
-// Run the script-if (require.main === module) {- generateMockLogs();-}+// Run the script when executed directly+const isDirectRun =+ process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1];+if (isDirectRun) {+ generateMockLogs();+}
scripts/generate-devbox-environment.sh (1)
7-23: .env value quoting: paths with spaces will break parsing.
Unquoted values in .env can fail if $DEVBOX_PROJECT_ROOT contains spaces. Quote values and ensure log dir exists.
Apply this diff:
if [ ! -f "$DEVBOX_PROJECT_ROOT/src/.env.local" ]; then
echo "Generating .env.local file"
cat <<EOF > "$DEVBOX_PROJECT_ROOT/src/.env.local"
-USER=$USER-RATOS_CONFIGURATION_PATH=$DEVBOX_PROJECT_ROOT/configuration-KLIPPER_CONFIG_PATH=$DEVBOX_PROJECT_ROOT/devbox.d/printer-config/config-RATOS_SCRIPT_DIR=$DEVBOX_PROJECT_ROOT/src/scripts-KLIPPER_DIR=$DEVBOX_PROJECT_ROOT/devbox.d/klipper-KLIPPER_ENV=$DEVBOX_PROJECT_ROOT/devbox.d/klippy-env-MOONRAKER_DIR=$DEVBOX_PROJECT_ROOT/.devbox/nix/profile/default/lib/moonraker-LOG_FILE=$DEVBOX_PROJECT_ROOT/devbox.d/printer-config/logs/ratos-configurator.log-RATOS_DATA_DIR=$DEVBOX_PROJECT_ROOT/devbox.d/printer-config/ratos-NEXT_PUBLIC_KLIPPER_HOSTNAME=-RECOIL_DUPLICATE_ATOM_KEY_CHECKING_ENABLED=false+USER="$USER"+RATOS_CONFIGURATION_PATH="$DEVBOX_PROJECT_ROOT/configuration"+KLIPPER_CONFIG_PATH="$DEVBOX_PROJECT_ROOT/devbox.d/printer-config/config"+RATOS_SCRIPT_DIR="$DEVBOX_PROJECT_ROOT/src/scripts"+KLIPPER_DIR="$DEVBOX_PROJECT_ROOT/devbox.d/klipper"+KLIPPER_ENV="$DEVBOX_PROJECT_ROOT/devbox.d/klippy-env"+MOONRAKER_DIR="$DEVBOX_PROJECT_ROOT/.devbox/nix/profile/default/lib/moonraker"+LOG_FILE="$DEVBOX_PROJECT_ROOT/devbox.d/printer-config/logs/ratos-configurator.log"+RATOS_DATA_DIR="$DEVBOX_PROJECT_ROOT/devbox.d/printer-config/ratos"+NEXT_PUBLIC_KLIPPER_HOSTNAME=""+RECOIL_DUPLICATE_ATOM_KEY_CHECKING_ENABLED="false"
EOF
echo "Generated .env.local file"
fi
Also consider set -euo pipefail at the top to fail fast.
src/cli/commands/logs.tsx (1)
29-36: Align CLI log colors with UI components
DEBUG/INFO color mismatch — CLI (src/cli/commands/logs.tsx: LOG_LEVELS) uses DEBUG=magenta, INFO=cyan; UI (src/app/logs/_components/types.ts: LOG_LEVELS) uses DEBUG=green, INFO=sky. Align them (recommend updating the CLI LOG_LEVELS to DEBUG→green and INFO→sky to match the UI).
configuration/scripts/ratos-log-rotation.sh (1)
1-325: Fix file permissions - script must be executable.
The pipeline failure indicates this script needs executable permissions.
57-59: handleClear doesn't actually clear the logs
The handleClear function just calls handleRefresh, which doesn't clear the logs. The clear mutation is triggered in LogSummaryHeader but the local state here isn't reset.
The handleClear callback should reset the filters to provide a true "clear" experience:
const handleClear = () => {
- handleRefresh();+ // Reset all filters to defaults+ setLogLevel('info');+ setSelectedContexts([]);+ setSelectedSources([]);+ setShowDetails(false);+ setShowOnlyErrors(false);+ setSortDirection('desc');+ // Then refresh to get clean data+ refetch();
};
Using as any bypasses TypeScript's type checking and could lead to runtime errors.
🧹 Nitpick comments (55)
src/scripts/generate-mock-logs.js (1)
199-199: Stale “Update Logs” strings – please align to “Logs.”
Console messages still say “Generating mock update logs …” and “Update Logs Viewer.” Update to avoid UX confusion.
Apply this diff:
- console.log(`Generating mock update logs to: ${logPath}`);+ console.log(`Generating mock logs to: ${logPath}`);
@@
- console.log('\nYou can now test the Update Logs Viewer with this mock data!');+ console.log('\nYou can now test the Logs Viewer with this mock data!');
Also applies to: 236-236
src/scripts/generate-mock-update-logs.ts (2)
220-220: Stale wording — “update logs.”
Minor text tweak for consistency.
- console.log(`Generating mock update logs to: ${logPath}`);+ console.log(`Generating mock logs to: ${logPath}`);
-import { writeFile, appendFile, existsSync, mkdirSync } from 'fs';+import { existsSync, mkdirSync } from 'fs';
src/scripts/README-mock-logs.md (1)
95-107: Outdated heading: “Update Logs Viewer.”
Adjust to “Logs Viewer” to match the rename.
-## Testing the Update Logs Viewer+## Testing the Logs Viewer
docs/logging-system.md (1)
384-395: Env var naming consistency (RATOS_LOG_FILE vs LOG_FILE).
Elsewhere (mock scripts, devbox env) you use LOG_FILE. Consider documenting both or standardizing to one.
Suggested doc tweak:
Note that the app also respects LOG_FILE (dev/local), while RATOS_LOG_FILE may be preferred for systemd/services. Add a short table clarifying precedence.
.vscode/settings.json (1)
49-49: Terminal CWD set to src — verify this won’t break tasks/scripts
This can confuse tasks expecting repo root paths (e.g., package.json scripts, tooling). Consider leaving CWD default or scoping CWD per-task.
Earlier you state logs.clear is disabled; advise rotation instead.
- - Manually clear logs using `ratos update-logs clear` (CLI) or web UI- - Adjust `RATOS_LOG_MAX_SIZE` if needed+ - Use `ratos logs rotate` to rotate logs (clearing is disabled)+ - Adjust rotation settings via environment if needed
157-168: time field sample vs actual format
Sample shows ISO string; pino/unified scripts often emit epoch ms. Confirm and align docs (and types.ts).
43-63: Consider paginating summary data for scalability
The unified endpoint design is excellent for performance. However, when includeSummary is true, the entire log file is still parsed and kept in memory to generate the summary. For large log files, this could cause memory issues.
Consider implementing incremental summary generation that processes the file in chunks, similar to how readObjects works with streaming:
-// Generate summary from log entries-export function generateSummary(entries: LogEntry[], logFileSize: number, logFileExists: boolean): LogSummary {+// Generate summary by streaming through log file+export async function generateSummaryStream(logPath: string, filter?: (entry: LogEntry) => boolean): Promise<LogSummary> {
375-385: Redundant sorting for already sorted data
The entries are already sorted chronologically in parseLogFile (line 89), so sorting them again here is unnecessary unless the sort direction is 'asc'.
-// Apply server-side sorting-filteredEntries = filteredEntries.sort((a, b) => {- const timeA = new Date(a.time).getTime();- const timeB = new Date(b.time).getTime();-- if (input.sortDirection === 'desc') {- return timeB - timeA; // newest first- } else {- return timeA - timeB; // oldest first- }-});+// Apply server-side sorting only if needed (already sorted asc from parseLogFile)+if (input.sortDirection === 'desc') {+ filteredEntries = filteredEntries.reverse();+}
The as any type assertions bypass TypeScript's type checking for the badge color prop. This could hide type mismatches.
-const borderStyle = badgeBorderColorStyle({ color: level.badgeColor as any });-const bgStyle = badgeBackgroundMutedColorStyle({ color: level.badgeColor as any });+// Ensure badgeColor is typed correctly in LOG_LEVELS+import { z } from 'zod';+import { badgeColorOptions } from '@/components/common/badge';++type BadgeColor = z.infer<typeof badgeColorOptions>;+const borderStyle = badgeBorderColorStyle({ color: level.badgeColor as BadgeColor });+const bgStyle = badgeBackgroundMutedColorStyle({ color: level.badgeColor as BadgeColor });
34-38: Context badge only shown with details enabled
The context badge is only shown when showDetails is true (line 34), but the source badge is always shown (line 39). This inconsistency might confuse users.
Consider showing both badges consistently, either always or both controlled by showDetails.
configuration/scripts/beacon-update.sh (1)
29-34: Consider setting exit_code consistently for all failures
While the script sets exit_code=1 when update_beacon_fw fails, the root permission check on line 20 directly exits with code 1. For consistency with the error accumulation pattern used throughout the PR, consider using the exit_code variable there as well.
Apply this diff for consistency:
if [ "$EUID" -ne 0 ]; then
echo "ERROR: Please run as root"
log_error "Script must run as root but was executed by non-root user" "main" "ROOT_REQUIRED"
- exit 1+ exit_code=1+ create_log_summary "beacon-update.sh" "$START_TIME"+ log_script_complete "beacon-update.sh" "$exit_code"+ exit "$exit_code"
fi
60-62: Type assertion for Badge color prop should be avoided
Using as any type assertion bypasses TypeScript's type checking. Consider defining a proper type mapping.
Since the Badge component accepts specific color values, create a type-safe mapping:
+// Add this type mapping at the top of the component+const COLOR_MAP: Record<string, Parameters<typeof Badge>[0]['color']> = {+ gray: 'gray',+ green: 'green', + sky: 'sky',+ yellow: 'yellow',+ red: 'red',+ purple: 'purple'+};
// Then use it in the Badge components:
-<Badge color={levelConfig.badgeColor as any} size="sm">+<Badge color={COLOR_MAP[levelConfig.badgeColor] || 'gray'} size="sm">
Also applies to: 85-88
configuration/scripts/klipper-mcu-update.sh (1)
17-21: Consider consistent error handling pattern
Similar to beacon-update.sh, this script directly exits on root check failure instead of using the exit_code accumulation pattern.
For consistency with the error accumulation pattern:
if [ "$EUID" -ne 0 ]; then
echo "ERROR: This script should be run as root"
log_error "Script must run as root but was executed by non-root user" "main" "ROOT_REQUIRED"
- exit 1+ exit_code=1+ create_log_summary "klipper-mcu-update.sh" "$START_TIME"+ log_script_complete "klipper-mcu-update.sh" "$exit_code"+ exit "$exit_code"
fi
configuration/scripts/ratos-install.sh (1)
81-81: verify_ready exits directly on root check
The verify_ready function calls exit 1 directly when run as root (line 64), which could bypass the error accumulation. Consider returning an error code instead.
Modify verify_ready to return an error instead of exiting:
verify_ready()
{
log_info "Verifying script execution requirements" "verify_ready"
if [ "$EUID" -eq 0 ]; then
echo "This script must not run as root"
log_error "Script attempted to run as root" "verify_ready" "ROOT_EXECUTION_DENIED"
- exit 1+ return 1
fi
log_info "Script execution requirements verified" "verify_ready"
+ return 0
}
src/scripts/update.sh (1)
16-20: Direct exit in verify_ready should be reconsidered
The verify_ready function exits directly when not run as root, which bypasses the error accumulation pattern used elsewhere in the script.
Consider returning an error code instead:
verify_ready()
{
log_info "Verifying script execution requirements" "verify_ready"
if [ ! "$EUID" -eq 0 ]; then
echo "This script must run as root"
log_error "Script must run as root but was executed by non-root user" "verify_ready" "ROOT_REQUIRED"
- exit 1+ return 1
fi
log_info "Script execution requirements verified" "verify_ready"
+ return 0
}
src/app/logs/_components/logs-viewer.tsx (1)
121-139: Good prop drilling but consider using context for deeply nested state
The component passes 13+ props to VirtualizedLogList. While this works, consider using React Context for filter state management to reduce prop drilling in future iterations.
For future consideration, a FilterContext could simplify state management:
// Example structure (not for immediate implementation)constFilterContext=React.createContext<{logLevel: string;setLogLevel: (level: string)=>void;// ... other filter states}>({...});// Usage in components would be cleaner:const{ logLevel, setLogLevel }=useContext(FilterContext);
scripts/test-logging-integration.sh (3)
8-11: Make BASE_DIR resolution portable (fallback if realpath is missing).
realpath isn’t available on all systems. Provide a readlink -f fallback to avoid failures on minimal/macOS environments.
64-73: Log summary likely counts nothing (filters on source="ratos-update").
create_log_summary filters by "ratos-update", but this script logs with contexts like main/script_lifecycle. Update the library to accept a source/context filter or provide a variant for per-script summaries; otherwise the summary will show zeros.
Would you like a follow-up PR to add a create_log_summary_for_source helper and switch this call accordingly?
scripts/test-complete-logging-system.sh (2)
8-11: Same portability nit: avoid hard dependency on realpath.
The EnhancedLogLevelSelector doesn’t expose an input with id="log-level". Either add an id to its trigger or drop htmlFor to avoid an invalid association.
Type error as unknown and render safely to avoid runtime errors.
- error: any;+ error: unknown;
And when rendering:
-<ErrorMessage title="Failed to load log entries">{error.message}</ErrorMessage>+<ErrorMessage title="Failed to load log entries">+ {error instanceof Error ? error.message : String(error)}+</ErrorMessage>
54-61: Virtualizer margins based on refs may be stale.
scrollMargin/paddingEnd read null on first render and won’t update. Consider using a stateful value updated in a layout effect when containerRef mounts, then pass to useWindowVirtualizer.
I can provide a small refactor if you’d like.
src/__tests__/update-logs.test.ts (1)
81-100: Fix mixed async/await patterns in test.
The test uses async but doesn't await the promise returned by writeFile. This could lead to race conditions.
Apply this diff to ensure proper async handling:
-it('should skip invalid JSON lines and include all sources', async () => {+it('should skip invalid JSON lines and include all sources', async () => {
const logContent = [
`{"level":30,"time":${new Date('2024-01-01T10:00:00.000Z').getTime()},"msg":"Valid entry","source":"ratos-update"}`,
'Invalid JSON line',
`{"level":50,"time":${new Date('2024-01-01T10:01:00.000Z').getTime()},"msg":"Another valid entry","source":"ratos-update"}`,
`{"level":30,"time":${new Date('2024-01-01T10:02:00.000Z').getTime()},"msg":"Different source","source":"other-service"}`,
`{"level":30,"time":${new Date('2024-01-01T10:03:00.000Z').getTime()},"msg":"Server entry"}`,
].join('\n');
await writeFile(TEST_LOG_FILE, logContent);
const parsedEntries = await parseLogFile(TEST_LOG_FILE);
expect(parsedEntries).toHaveLength(4); // All valid entries including different sources
expect(parsedEntries[0]?.msg).toBe('Valid entry');
expect(parsedEntries[1]?.msg).toBe('Another valid entry');
expect(parsedEntries[2]?.msg).toBe('Different source');
expect(parsedEntries[3]?.msg).toBe('Server entry');
expect(parsedEntries[3]?.source).toBe('server'); // Default source for entries without source
});
90-100: Consider feature flag instead of NODE_ENV check.
Using process.env.NODE_ENV === 'development' for the test data generator could accidentally expose this feature if environment variables are misconfigured.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue
Write log lines to stderr to avoid breaking scripts that source this file
This script is commonly sourced; emitting to stdout can pollute/corrupt caller output (e.g., JSON, CLI piping). Redirect to stderr and prefer printf for portability.
Apply this diff:
- echo "Loading environment from $envFile"+ printf '[env] Loading environment from %s\n' "$envFile" >&2
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
echo"Loading environment from $envFile"
load_env "$envFile"
fi
printf'[env] Loading environment from %s\n'"$envFile">&2
load_env "$envFile"
fi
🤖 Prompt for AI Agents
In configuration/scripts/environment.sh around lines 115 to 117, the script
prints "Loading environment from $envFile" to stdout which can corrupt output
when the file is sourced; change the echo to a stderr-prefixed, portable printf
call (e.g., printf '%s\n' "Loading environment from $envFile" >&2) so log lines
go to stderr, and leave the load_env call unchanged.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue
Same here: redirect user env load message to stderr
Keep output clean for callers; use printf and stderr.
Apply this diff:
- echo "Loading environment from $userEnvFile"+ printf '[env] Loading environment from %s\n' "$userEnvFile" >&2
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
echo"Loading environment from $userEnvFile"
load_env "$userEnvFile"
fi
printf'[env] Loading environment from %s\n'"$userEnvFile">&2
load_env "$userEnvFile"
fi
🤖 Prompt for AI Agents
In configuration/scripts/environment.sh around lines 119 to 121, the
informational echo "Loading environment from $userEnvFile" should be written to
stderr using printf; replace the echo with a printf that formats the filename
and redirects to stderr (e.g. printf 'Loading environment from %s\n'
"$userEnvFile" >&2) and leave the subsequent load_env call unchanged so callers'
stdout remains clean.
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
🧩 Analysis chain
Typo in function name should be fixed
The function name ensure_moonraker_policiykit_rules contains a typo ("policiy" should be "policy"). This should be corrected in the source file moonraker-ensure-policykit-rules.sh.
Let me verify if this typo exists in the source file:
🏁 Script executed:
#!/bin/bash# Check if the typo exists in the function definition
rg -n "ensure_moonraker_policiykit_rules" configuration/scripts/
Length of output: 186
Rename function ensure_moonraker_policiykit_rules → ensure_moonraker_policykit_rules
Fix the typo ("policiy" → "policy") in the function name and update the definition and all callers:
In configuration/scripts/moonraker-update.sh around lines 21 to 26 the function
name is misspelled as ensure_moonraker_policiykit_rules; rename it to
ensure_moonraker_policykit_rules and update its definition in
configuration/scripts/moonraker-ensure-policykit-rules.sh at line 11 to match,
then update all callers (including the call in moonraker-update.sh) to use the
corrected name so the script invokes the defined function.
‼️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.
In configuration/scripts/ratos-logging-config.sh around lines 9 to 12, the
default literal "~" is quoted and will not expand, producing paths like ./~ and
./~/logs; change the defaults to use the expanded $HOME instead (e.g. use
${RATOS_PRINTER_DATA_DIR:-"$HOME"} for RATOS_LOGGING_CONFIG_FILE and
${RATOS_PRINTER_DATA_DIR:-"$HOME"}/logs for RATOS_LOG_FILE) so dirname and
subsequent file paths operate on an actual home directory path.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue
Do not downscale byte threshold to MB; it breaks small limits.
Dividing bytes to MB can yield 0M for values <1MiB, disabling rotation unintentionally. Pass bytes as-is and let the rotation script parse numeric bytes.
- local max_size_str="${RATOS_LOG_MAX_SIZE}"- if [[ $RATOS_LOG_MAX_SIZE -gt 0 ]]; then- # Convert bytes to MB for readability- max_size_str="$((RATOS_LOG_MAX_SIZE / 1024 / 1024))M"- fi-- rotate_log_file "$RATOS_LOG_FILE" "$max_size_str" "$RATOS_LOG_BACKUP_COUNT" "true"+ rotate_log_file "$RATOS_LOG_FILE" "$RATOS_LOG_MAX_SIZE" "$RATOS_LOG_BACKUP_COUNT" "true"
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
if [[ $LOG_ROTATION_AVAILABLE==true ]];then
# Convert RATOS_LOG_MAX_SIZE to size format expected by rotation script
In configuration/scripts/ratos-logging.sh around lines 278 to 287, the code
converts RATOS_LOG_MAX_SIZE from bytes to megabytes which turns values under
1MiB into "0M" and disables rotation; instead stop downscaling—pass the original
byte value (as a plain number or string representing bytes) to rotate_log_file
so the rotation script can parse numeric bytes itself; remove the division-to-M
conversion and ensure max_size_str is set to RATOS_LOG_MAX_SIZE when greater
than zero.
Radix Select needs SelectPrimitive.ItemText for the accessible label/typeahead; the current fullElement branch renders a plain div and removes that semantics.
‼️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.
In src/components/ui/select.tsx around lines 110–113, the fullElement branch
currently replaces SelectPrimitive.ItemText with a plain div, breaking typeahead
and a11y; restore the semantics by rendering SelectPrimitive.ItemText (wrapping
the visible children or text) inside the fullElement branch instead of a plain
div so the Item retains its accessible label/typeahead behavior, and keep any
styling by applying the same className to the ItemText or a surrounding element
as needed; after the change validate keyboard typeahead works.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue
Fix service change detection: compare against processed file, not the raw template.
Comparing ${SERVICE_FILE_TEMPLATE} to ${SERVICE_FILE} will almost always differ because the installed unit contains substitutions. Compare the processed /tmp/ratos-configurator.service instead to avoid unnecessary reloads/restarts.
Apply:
- if [ "$(md5sum "${SERVICE_FILE_TEMPLATE}")" != "$(md5sum "${SERVICE_FILE}")" ]; then+ if [ "$(md5sum /tmp/ratos-configurator.service | awk '{print $1}')" != "$(md5sum "${SERVICE_FILE}" | awk '{print $1}')" ]; then
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
if [ -f"${SERVICE_FILE}" ];then
if [ "$(md5sum "${SERVICE_FILE_TEMPLATE}")"!="$(md5sum "${SERVICE_FILE}")" ];then
if execute_with_logging "install_or_update_service_file""SERVICE_UPDATE_FAILED" sudo mv /tmp/ratos-configurator.service "${SERVICE_FILE}";then
if execute_with_logging "install_or_update_service_file""SYSTEMCTL_RELOAD_FAILED" sudo systemctl daemon-reload;then
🤖 Prompt for AI Agents
In src/scripts/common.sh around lines 86 to 90, the script currently compares
md5sum of the raw template (${SERVICE_FILE_TEMPLATE}) to the installed unit,
which will differ after template substitutions; update the comparison to use the
processed temp unit (e.g. /tmp/ratos-configurator.service or whatever variable
holds the processed file) instead of the raw template, and ensure the processed
file exists before computing its md5sum; replace the md5sum left-hand operand
with the processed file path, keep the right-hand operand as the installed
${SERVICE_FILE}, and retain the guarded execute_with_logging/mv and systemctl
calls.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue
BASE_DIR may be unset here; guard before sourcing logging.
setup.sh uses $BASE_DIR to source ratos-logging.sh but relies on common.sh to define it. Add a fallback to prevent sourcing from /configuration/... when BASE_DIR is empty.
‼️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.
In src/scripts/setup.sh around lines 6-9, the script unconditionally sources
"$BASE_DIR/configuration/scripts/ratos-logging.sh" but BASE_DIR may be unset;
add a guard before sourcing: check if BASE_DIR is empty or unset and either set
a safe default (e.g. compute BASE_DIR relative to the script directory, such as
two levels up from the script) or exit with a clear error, then source using the
validated/quoted BASE_DIR; this prevents accidentally sourcing
"/configuration/..." when BASE_DIR is empty.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue
Don’t exit inside a step; return and let the orchestrator handle summary/logging.
exit 1 aborts the script and skips create_log_summary/log_script_complete. Use return 1 and keep fail-fast semantics at the caller if desired.
- exit 1+ return 1
If you want to fail fast, check return and exit immediately in the main block right after verify_ready.
🤖 Prompt for AI Agents
In src/scripts/setup.sh around lines 14 to 23, the verify_ready function calls
exit 1 when detecting root execution which aborts the whole script and prevents
downstream summary/logging; change the function to return 1 instead of exiting
so callers can decide fail-fast behavior, and update any places that call
verify_ready in the main block to check its return value and call exit if
immediate termination is desired (i.e., if ! verify_ready; then exit 1; fi),
preserving current logging behavior inside verify_ready.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue
🧩 Analysis chain
Verify dynamic import path for mock data generator
The dynamic import uses a relative path that may not work correctly in production builds.
🏁 Script executed:
#!/bin/bash# Check if the mock data generator file exists and verify the import path
fd -t f "generate-mock-update-logs" --exec ls -la {}
# Check for any build/dist artifacts that might affect the import path
fd -t d "dist|build" -d 2 | head -5
Length of output: 91
Use a runtime-resolvable import instead of the '@' alias
src/server/routers/logs.ts (lines 269–270): confirmed src/scripts/generate-mock-update-logs.ts exists; import('@/scripts/generate-mock-update-logs') relies on a TypeScript path alias that likely won't resolve at runtime — replace with a relative import ('../../scripts/generate-mock-update-logs') or ensure your build/runtime resolves the '@' alias.
🤖 Prompt for AI Agents
In src/server/routers/logs.ts around lines 269-270, the dynamic import uses the
TypeScript '@' path alias (import('@/scripts/generate-mock-update-logs')), which
may not resolve at runtime; replace it with a runtime-resolvable relative import
import('../../scripts/generate-mock-update-logs') (from src/server/routers to
src/scripts) or alternatively ensure your build/runtime is configured to resolve
the '@' alias so the dynamic import works in production.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This PR refactors the RatOS configurator log system with comprehensive improvements to user experience, expanding it to cover the majority of bash scripts as well as the update scripts.
There's still some work to be done to finish it off, along with a bunch of QA and practical test, but the functionality is mostly complete, except for a few bash edge cases which still need to be handled properly.