Add a node events page #173
Conversation
Signed-off-by: Dave Thaler <[email protected]>
|
Warning Rate limit exceeded@dthaler has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request introduce new methods for managing and displaying Orcanode events and uptime metrics. A method Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 10
🧹 Outside diff range and nitpick comments (7)
OrcanodeMonitor/Pages/DataplicityNode.cshtml.cs (1)
Input validation is needed for the serial parameter
The codebase shows that while
IsNullOrEmptychecks are used extensively, there's no validation of the serial parameter before it's used in the API call. TheGetDataplicityDataAsyncmethod only checks if the serial is empty but doesn't validate its format or content. This could lead to potential security issues when making external API calls.
OrcanodeMonitor/Pages/DataplicityNode.cshtml.cs: Add input validation before using the serial parameterOrcanodeMonitor/Core/Fetcher.cs: Consider enhancing theGetDataplicityDataAsyncmethod to include proper validationExample validation needed:
- Check for null or whitespace
- Validate length constraints
- Verify the serial follows expected format (alphanumeric)
- Sanitize input to prevent injection attacks
🔗 Analysis chain
Line range hint
44-51: Consider adding input validation for the serial parameter.The
serialparameter is directly used in an external API call without validation. Consider adding input validation to prevent potential security issues.Let's check how serial is validated in other parts of the codebase:
Consider adding validation before using the serial parameter:
public async Task<IActionResult> OnGetAsync(string serial) { + if (string.IsNullOrWhiteSpace(serial) || !IsValidSerial(serial)) + { + return BadRequest("Invalid serial number"); + } _serial = serial; string rawJson = await Fetcher.GetDataplicityDataAsync(serial); var formatter = new JsonFormatter(); _jsonData = formatter.FormatJson(rawJson); return Page(); } +private bool IsValidSerial(string serial) +{ + // Add appropriate validation logic based on your requirements + // For example: check length, allowed characters, format, etc. + return !string.IsNullOrWhiteSpace(serial) && serial.Length <= 50 && + System.Text.RegularExpressions.Regex.IsMatch(serial, "^[a-zA-Z0-9-]+$"); +}
Let me check the implementation of
GetDataplicityDataAsyncto understand if there's any validation happening there.
Let me check if there are any other security measures or input validation patterns used in the codebase that we should follow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for patterns of serial validation rg -A 3 "GetDataplicityDataAsync|serial.*validation"Length of output: 1008
Script:
#!/bin/bash # Get the implementation of GetDataplicityDataAsync rg -A 15 "public async static Task<string> GetDataplicityDataAsync" OrcanodeMonitor/Core/Fetcher.csLength of output: 653
Script:
#!/bin/bash # Look for input validation patterns and security measures rg -A 5 "IsNullOrEmpty|BadRequest|ModelState\.IsValid|[Vv]alidat" --type csLength of output: 13421
OrcanodeMonitor/Pages/NodeEvents.cshtml.cs (1)
8-8: Remove unused importSystem.Xml.LinqThis import is not used in the code and should be removed to keep the imports clean and relevant.
-using System.Xml.Linq;OrcanodeMonitor/Models/OrcanodeEvent.cs (1)
Line range hint
1-153: Consider separating concerns and making severity rules configurable.The
OrcanodeEventclass currently handles multiple responsibilities:
- Database persistence (EF Core entity)
- Business logic (severity calculation)
- DTO conversion
- String representation
Consider:
- Moving the severity calculation to a separate service class
- Making the severity rules configurable through settings
- Using separate DTOs for different use cases (persistence vs API)
This would improve maintainability and flexibility.
OrcanodeMonitor/Pages/Index.cshtml.cs (1)
Line range hint
104-115: Optimize the uptime percentage calculation and improve maintainability.Two suggestions:
- Reuse the GetUptimePercentage method to avoid duplicate calculations
- Consider making the color thresholds configurable
public string NodeUptimePercentageBackgroundColor(Orcanode node) { - int value = Orcanode.GetUptimePercentage(node.ID, _events, SinceTime); + int value = GetUptimePercentage(node); if (value < 1) { return ColorTranslator.ToHtml(Color.Red); } else if (value > 99) { return ColorTranslator.ToHtml(Color.LightGreen); } return ColorTranslator.ToHtml(Color.Yellow); }OrcanodeMonitor/Pages/Index.cshtml (1)
74-77: LGTM! Consider adding ARIA attributes for accessibility.The implementation of the clickable uptime percentage is clean and consistent with the page's existing patterns. The link appropriately opens in a new tab to preserve the dashboard state.
Consider adding ARIA attributes to improve accessibility:
-<a asp-page="/NodeEvents" asp-route-id="@item.ID" style="color: @Model.NodeUptimePercentageTextColor(item)" target="_blank"> +<a asp-page="/NodeEvents" asp-route-id="@item.ID" style="color: @Model.NodeUptimePercentageTextColor(item)" target="_blank" + aria-label="View events for @item.DisplayName (opens in new tab)">OrcanodeMonitor/Models/Orcanode.cs (2)
395-401: Add input validation and XML documentation.The method lacks XML documentation and validation for the
orcanodeIdandsinceparameters.Add parameter validation and documentation:
+ /// <summary> + /// Calculates the uptime percentage for a node based on its events since a specified date. + /// </summary> + /// <param name="orcanodeId">The ID of the node to calculate uptime for</param> + /// <param name="events">List of node events</param> + /// <param name="since">The start date for uptime calculation</param> + /// <returns>Uptime percentage as an integer between 0 and 100</returns> + /// <exception cref="ArgumentException">Thrown when orcanodeId is null or empty</exception> public static int GetUptimePercentage(string orcanodeId, List<OrcanodeEvent> events, DateTime since) { + if (string.IsNullOrEmpty(orcanodeId)) + { + throw new ArgumentException("Node ID cannot be null or empty", nameof(orcanodeId)); + } + + if (since > DateTime.UtcNow) + { + throw new ArgumentException("Start date cannot be in the future", nameof(since)); + } + if (events == null) { return 0; }
434-441: Consider capturing DateTime.UtcNow once to prevent race conditions.The current implementation uses
DateTime.UtcNowdirectly in calculations, which could lead to slight inconsistencies if the execution time varies between the two branches.Capture the current time once at the start of the method:
+ DateTime now = DateTime.UtcNow; if (lastValue == Orcanode.OnlineString) { - up += DateTime.UtcNow - start; + up += now - start; } else { - down += DateTime.UtcNow - start; + down += now - start; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- OrcanodeMonitor/Core/Fetcher.cs (1 hunks)
- OrcanodeMonitor/Models/Orcanode.cs (1 hunks)
- OrcanodeMonitor/Models/OrcanodeEvent.cs (3 hunks)
- OrcanodeMonitor/Pages/DataplicityNode.cshtml.cs (1 hunks)
- OrcanodeMonitor/Pages/Index.cshtml (1 hunks)
- OrcanodeMonitor/Pages/Index.cshtml.cs (2 hunks)
- OrcanodeMonitor/Pages/NodeEvents.cshtml (1 hunks)
- OrcanodeMonitor/Pages/NodeEvents.cshtml.cs (1 hunks)
- OrcanodeMonitor/Pages/NodeEvents.cshtml.css (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- OrcanodeMonitor/Pages/NodeEvents.cshtml.css
🔇 Additional comments (7)
OrcanodeMonitor/Pages/DataplicityNode.cshtml.cs (1)
25-26: LGTM! Good practice initializing fields.The initialization of private fields with empty strings prevents potential null reference issues.
OrcanodeMonitor/Pages/NodeEvents.cshtml.cs (1)
33-37: Verify node existence before processingThe code should verify that the provided node ID exists in the database before attempting to fetch events.
Consider adding a validation method to verify node existence:
private bool ValidateNode(string nodeId) { return _databaseContext.Orcanodes.Any(n => n.Id == nodeId); }OrcanodeMonitor/Models/OrcanodeEvent.cs (2)
149-149: LGTM! The ToString implementation is more user-friendly.The change to use
NodeNameinstead ofSlugimproves readability while maintaining consistent time formatting.
105-105: Verify the impact of nullable Orcanode navigation property.The change to make
Orcanodenullable is logically correct, but we should verify that all queries and code paths handle the null case appropriately.Let's check for direct Orcanode property access that might not handle null:
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to find any usage of Orcanode, including property access, LINQ queries, and null checks to better understand the impact of making it nullable.
Based on the search results, I can now generate the final response:
Making
Orcanodenullable is safe and correctly handledThe codebase already handles null
Orcanodeappropriately:
- The
NodeNameproperty uses null-coalescing operator:Orcanode?.DisplayName ?? "<Unknown>"- The foreign key
OrcanodeIdremains non-nullable in the database schema (as seen in migrations)- No direct unsafe access to
Orcanodeproperty was found in the codebase- The navigation property is only used in a safe manner through optional chaining
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct Orcanode property access that might need null checking rg "\.Orcanode\." --type csLength of output: 29
Script:
#!/bin/bash # Let's try a more comprehensive search for Orcanode usage patterns rg "Orcanode[^.]" --type cs -A 2 # Also search for potential LINQ queries involving Orcanode rg "from.*Orcanode|join.*Orcanode|select.*Orcanode" --type cs # Check if there are any null checks on Orcanode rg "Orcanode\s*[!=]=\s*null" --type csLength of output: 88443
OrcanodeMonitor/Pages/Index.cshtml.cs (2)
27-28: LGTM: Good defensive programming practice.Initializing collections in the constructor prevents potential null reference exceptions.
100-100: LGTM: Good separation of concerns.Moving the uptime calculation logic to the Orcanode class improves code organization. Let's verify all callers are updated.
✅ Verification successful
All callers correctly use the static Orcanode.GetUptimePercentage method
The search results confirm that all usages of GetUptimePercentage have been properly updated to use the static method from the Orcanode class:
- The static implementation exists in
OrcanodeMonitor/Models/Orcanode.cs- All callers in
Index.cshtml.csandNodeEvents.cshtml.csare using the correctOrcanode.GetUptimePercentagestatic method with the expected parameters🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct calls to the old GetUptimePercentage implementation rg -l "GetUptimePercentage.*\{" --type csLength of output: 43
Script:
#!/bin/bash # Search for any usage of GetUptimePercentage method rg "GetUptimePercentage" --type cs -A 2 # Also search for the method definition in Orcanode class ast-grep --pattern 'class Orcanode { $$$ GetUptimePercentage($$$) { $$$ } $$$ }'Length of output: 2234
OrcanodeMonitor/Models/Orcanode.cs (1)
402-433: LGTM! The uptime calculation logic is correct and handles edge cases properly.The implementation correctly:
- Processes events chronologically
- Accumulates up/down time based on event values
- Handles division by zero
- Implements proper rounding
Also applies to: 442-450
Signed-off-by: Dave Thaler <[email protected]>
dthaler
force-pushed
the
nodeeventspage
branch
from
af4d504
to
467e3ce
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🛑 Comments failed to post (3)
OrcanodeMonitor/Pages/NodeEvents.cshtml.css (3)
10-12:
⚠️ Potential issueConsider using relative units for better accessibility.
Using fixed pixel values for font sizes can cause accessibility issues. Consider using relative units like
remto respect user browser settings.h1 { - font-size: 32px; + font-size: 2rem; }📝 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.h1 { font-size: 2rem; }
1-4: 🛠️ Refactor suggestion
Consider using CSS variables for better maintainability.
While the current table styling works, using CSS variables would make it easier to maintain consistent styles across the application.
+:root { + --border-width: 1px; + --border-color: black; +} + table, th, td { - border: 1px solid black; + border: var(--border-width) solid var(--border-color); border-collapse: collapse; }📝 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.:root { --border-width: 1px; --border-color: black; } table, th, td { border: var(--border-width) solid var(--border-color); border-collapse: collapse; }
14-20:
⚠️ Potential issueEnhance button states for better accessibility and interactivity.
The current styling has several areas for improvement:
- Color contrast ratio should be verified for accessibility
- Missing hover and focus states for better user interaction
- Colors should use CSS variables for consistency
+:root { + --primary-color: #0066cc; + --primary-text: #ffffff; +} + .selected { - background-color: blue; - color: white; + background-color: var(--primary-color); + color: var(--primary-text); } + .unselected { - border-color: blue; + border-color: var(--primary-color); +} + +.selected:hover, .unselected:hover { + opacity: 0.9; +} + +.selected:focus, .unselected:focus { + outline: 2px solid var(--primary-color); + outline-offset: 2px; }📝 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.:root { --primary-color: #0066cc; --primary-text: #ffffff; } .selected { background-color: var(--primary-color); color: var(--primary-text); } .unselected { border-color: var(--primary-color); } .selected:hover, .unselected:hover { opacity: 0.9; } .selected:focus, .unselected:focus { outline: 2px solid var(--primary-color); outline-offset: 2px; }
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
OrcanodeMonitor/Pages/NodeEvents.cshtml.cs (2)
8-8: Remove unused importThe
System.Xml.Linqnamespace is imported but not used in this file.-using System.Xml.Linq;
14-14: Make database context readonlyThe database context field should be marked as readonly since it's only initialized in the constructor.
-private OrcanodeMonitorContext _databaseContext; +private readonly OrcanodeMonitorContext _databaseContext;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- OrcanodeMonitor/Models/OrcanodeEvent.cs (3 hunks)
- OrcanodeMonitor/Pages/Index.cshtml (2 hunks)
- OrcanodeMonitor/Pages/NodeEvents.cshtml (1 hunks)
- OrcanodeMonitor/Pages/NodeEvents.cshtml.cs (1 hunks)
- OrcanodeMonitor/Pages/NodeEvents.cshtml.css (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- OrcanodeMonitor/Models/OrcanodeEvent.cs
- OrcanodeMonitor/Pages/Index.cshtml
- OrcanodeMonitor/Pages/NodeEvents.cshtml
- OrcanodeMonitor/Pages/NodeEvents.cshtml.css
🧰 Additional context used
📓 Learnings (1)
OrcanodeMonitor/Pages/NodeEvents.cshtml.cs (1)
Learnt from: dthaler PR: orcasound/orcanode-monitor#173 File: OrcanodeMonitor/Pages/NodeEvents.cshtml.cs:0-0 Timestamp: 2024-10-25T18:29:41.800Z Learning: In the `NodeEventsModel` class, the ID is part of the route, so the functions `OnGet` and `OnPost` cannot be called with an empty ID.
🔇 Additional comments (3)
OrcanodeMonitor/Pages/NodeEvents.cshtml.cs (3)
19-20: Improve time range handlingThe current string-based time range selection could be improved using an enum for type safety and maintainability. This suggestion from the previous review is still valid.
25-31: LGTM!The constructor properly initializes all fields and follows dependency injection best practices.
33-44: LGTM!The error handling implementation is robust with proper logging and fallback behavior.
Signed-off-by: Dave Thaler <[email protected]>
Fixes #172
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor