Conversation
Summary of ChangesHello @Evrard-Nil, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the application's logging strategy by reclassifying several log messages from Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Code ReviewThis PR appropriately changes logging levels from ✅ Strengths
No Critical Issues FoundAll changes are appropriate:
The conditional logging in Minor Observation (Non-blocking)In ✅ Approved - No critical issues. Ready to merge. |
There was a problem hiding this comment.
Code Review
This pull request correctly changes the log level from error to warn for user-induced errors, which is a great improvement for log clarity and monitoring. The changes are applied consistently across several files. I've added one suggestion to refactor duplicated error-handling logic in crates/api/src/routes/files.rs to enhance maintainability. Overall, this is a solid contribution.
There was a problem hiding this comment.
Pull request overview
This PR improves the logging strategy by changing error-level logs to warning-level logs for user-induced errors (4xx client errors), while keeping error-level logs for server errors (5xx). This follows best practices where error! logs indicate issues requiring immediate attention from developers, while warn! logs indicate expected conditions like invalid user input.
- Changes direct
error!macro calls totracing::warn!for client errors across authentication, authorization, and resource access scenarios - Introduces conditional logging in the files module that dynamically chooses log level based on HTTP status code
- Maintains
error!logs for genuine server-side failures
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/api/src/routes/users.rs | Changes token rotation failure from error to warning |
| crates/api/src/routes/organizations.rs | Changes organization not found errors to warnings |
| crates/api/src/routes/organization_members.rs | Changes invalid member removal params to warning |
| crates/api/src/routes/files.rs | Implements conditional logging pattern based on status code, with client errors as warnings and server errors remaining as errors |
| crates/api/src/routes/auth.rs | Changes OAuth callback and NEAR authentication validation failures to warnings |
| crates/api/src/middleware/body_hash.rs | Changes request body reading failure to warning |
| crates/api/src/middleware/auth.rs | Changes authentication/authorization failures (admin privileges, API key issues, workspace not found) to warnings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| )), | ||
| Err(OrganizationError::InvalidParams(msg)) => { | ||
| error!("Cannot remove member: {}", msg); | ||
| warn!("Cannot remove member: {}", msg); |
There was a problem hiding this comment.
The macro invocation style is inconsistent across the codebase. This file uses warn! (relying on the import), while most other files in this PR use tracing::warn! (fully qualified path). For consistency, either use warn! everywhere (and ensure the import is present) or use tracing::warn! everywhere. The recommended approach is to use the fully qualified tracing::warn! to match the pattern used in the majority of changes in this PR.
| warn!("Cannot remove member: {}", msg); | |
| tracing::warn!("Cannot remove member: {}", msg); |
No description provided.