fix: catch ConfigError in display preview generator#288
Open
ChuckBuilds wants to merge 4 commits intomainfrom
Open
fix: catch ConfigError in display preview generator#288ChuckBuilds wants to merge 4 commits intomainfrom
ChuckBuilds wants to merge 4 commits intomainfrom
Conversation
PR #282 narrowed bare except blocks but missed ConfigError from config_manager.load_config(), which wraps FileNotFoundError, JSONDecodeError, and OSError. Without this, a corrupt or missing config crashes the display preview SSE endpoint instead of falling back to 128x64 defaults. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ConfigError handling to display preview dimension parsing, moves WebInterfaceError to fixed, code-based messages via a new _safe_message lookup, and replaces ad-hoc prints with a module-level logger plus standardized generic error responses in many Flask API endpoints. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
- Remove all traceback.format_exc() from client responses (33 remaining instances) - Sanitize str(e) from client-facing messages, replacing with generic error messages - Replace ~65 bare print() calls with structured logger.exception/error/warning/info/debug - Remove ~35 redundant inline `import traceback` and `import logging` statements - Convert logging.error/warning calls to use module-level named logger - Fix WiFi endpoints that created redundant inline logger instances - Add logger.exception() at all WebInterfaceError.from_exception() call sites - Fix from_exception() in errors.py to use safe messages instead of raw str(exception) - Apply consistent [Tag] prefixes to all logger calls for production triage Only safe, user-input-derived str(e) kept: json.JSONDecodeError handlers (400 responses). Subprocess template print(stdout) calls preserved (not error logging). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web_interface/app.py (1)
488-498: Correct fix for the ConfigError gap.The addition of
ConfigErroris necessary since it inherits fromLEDMatrixError, not fromKeyError,TypeError, orValueError. This properly handles the case whereload_config()raisesConfigErrorfor missing/corrupt config files.Optional: Consider adding a debug log when falling back to defaults to aid remote troubleshooting on Raspberry Pi.
🔧 Suggested logging for debugging
+ import logging + logger = logging.getLogger('web_interface') + # Get display dimensions from config try: main_config = config_manager.load_config() cols = main_config.get('display', {}).get('hardware', {}).get('cols', 64) chain_length = main_config.get('display', {}).get('hardware', {}).get('chain_length', 2) rows = main_config.get('display', {}).get('hardware', {}).get('rows', 32) parallel = main_config.get('display', {}).get('hardware', {}).get('parallel', 1) width = cols * chain_length height = rows * parallel - except (KeyError, TypeError, ValueError, ConfigError): + except (KeyError, TypeError, ValueError, ConfigError) as e: + logger.debug(f"[Display Preview] Using default dimensions (128x64): {e}") width = 128 height = 64As per coding guidelines: "Implement comprehensive logging for remote debugging on Raspberry Pi."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web_interface/app.py` around lines 488 - 498, The except block should catch ConfigError in addition to KeyError/TypeError/ValueError to handle load_config() failures; update the except clause in the try around config_manager.load_config() to include ConfigError and ensure ConfigError is imported from its defining module (where LEDMatrixError/ConfigError are declared). Also when falling back to defaults for width and height (variables width, height), add a debug or warning log via the app's logger describing the fallback and the caught exception to aid remote Raspberry Pi troubleshooting. Ensure references: config_manager.load_config(), main_config, ConfigError, width, height.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web_interface/app.py`:
- Around line 488-498: The except block should catch ConfigError in addition to
KeyError/TypeError/ValueError to handle load_config() failures; update the
except clause in the try around config_manager.load_config() to include
ConfigError and ensure ConfigError is imported from its defining module (where
LEDMatrixError/ConfigError are declared). Also when falling back to defaults for
width and height (variables width, height), add a debug or warning log via the
app's logger describing the fallback and the caught exception to aid remote
Raspberry Pi troubleshooting. Ensure references: config_manager.load_config(),
main_config, ConfigError, width, height.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fed19e22-ef40-483a-84f8-03a143c19044
📒 Files selected for processing (1)
web_interface/app.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web_interface/blueprints/api_v3.py (1)
443-469:⚠️ Potential issue | 🟡 MinorCatch
ConfigErrorhere instead of the wrapped file exceptions.
load_config()now normalizesFileNotFoundError,JSONDecodeError, andOSErrorintoConfigError, so these three handlers never run. Missing/corrupt config will fall through to the generic branch and return"Unexpected error..."with raw exception text instead of the intended safe message.💡 Minimal fix
- except FileNotFoundError as e: - logger.error(f"[DIM SCHEDULE] Config file not found: {e}", exc_info=True) - return error_response( - ErrorCode.CONFIG_LOAD_FAILED, - "Configuration file not found", - status_code=500 - ) - except json.JSONDecodeError as e: - logger.error(f"[DIM SCHEDULE] Invalid JSON in config file: {e}", exc_info=True) - return error_response( - ErrorCode.CONFIG_LOAD_FAILED, - "Configuration file contains invalid JSON", - status_code=500 - ) - except (IOError, OSError) as e: - logger.error(f"[DIM SCHEDULE] Error reading config file: {e}", exc_info=True) - return error_response( - ErrorCode.CONFIG_LOAD_FAILED, - f"Error reading configuration file: {str(e)}", - status_code=500 - ) + except ConfigError as e: + logger.warning("[DIM SCHEDULE] Failed to load config: %s", e, exc_info=True) + return error_response( + ErrorCode.CONFIG_LOAD_FAILED, + "Failed to load dim schedule configuration", + status_code=500 + )Also add
from src.exceptions import ConfigErrorwith the other imports.As per coding guidelines: "Implement graceful degradation to continue operation when non-critical features fail" and "Provide user-friendly error messages that explain what went wrong and potential solutions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web_interface/blueprints/api_v3.py` around lines 443 - 469, Replace the multiple specific exception handlers for FileNotFoundError, json.JSONDecodeError, and (IOError, OSError) with a single except ConfigError block so errors normalized by load_config() are handled with the intended safe message; update the import list to include ConfigError from src.exceptions, and inside the new except ConfigError use logger.error with exc_info=True and return error_response(ErrorCode.CONFIG_LOAD_FAILED, "Configuration file not found or invalid", status_code=500) (keeping the existing generic Exception handler for anything else). Ensure you reference load_config, ConfigError, error_response, and ErrorCode when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/web_interface/errors.py`:
- Around line 217-218: The current code uses cls._safe_message(error_code) when
error_code may be inferred (e.g., CONFIG_SAVE_FAILED) and thus can produce
misleading "Failed to save configuration" text for config-load failures; update
the logic in the error construction so that if error_code was inferred (or does
not match the exception type) you use a generic, action-agnostic message or
derive the message from the exception type instead of always calling
_safe_message(error_code). Locate the code paths around the callsite that build
the response (referencing _safe_message, _get_exception_details, and the
CONFIG_SAVE_FAILED symbol) and change them to prefer exception-derived messages
or a neutral fallback message when inference is involved, ensuring suggestions
match the actual operation (load vs save).
In `@web_interface/blueprints/api_v3.py`:
- Around line 657-659: save_main_config currently logs full request body and
headers (logger.error(f"DEBUG: save_main_config received data: {data}"),
logger.error(f"DEBUG: Content-Type header: {request.content_type}"),
logger.error(f"DEBUG: Headers: {dict(request.headers)}")), which can leak
secrets; change these to emit only a minimal summary (e.g., top-level keys and
length) at debug level and never log full payloads. Before any validation-error
logging in save_main_config (and the similar locations referenced), redact/mask
sensitive fields in the config and headers (Authorization, Cookie, api_key,
password, secret, token, etc.) and log the masked summary instead; remove direct
dict(request.headers) dumps and content bodies from error logs entirely so only
safe, masked summaries are recorded.
---
Outside diff comments:
In `@web_interface/blueprints/api_v3.py`:
- Around line 443-469: Replace the multiple specific exception handlers for
FileNotFoundError, json.JSONDecodeError, and (IOError, OSError) with a single
except ConfigError block so errors normalized by load_config() are handled with
the intended safe message; update the import list to include ConfigError from
src.exceptions, and inside the new except ConfigError use logger.error with
exc_info=True and return error_response(ErrorCode.CONFIG_LOAD_FAILED,
"Configuration file not found or invalid", status_code=500) (keeping the
existing generic Exception handler for anything else). Ensure you reference
load_config, ConfigError, error_response, and ErrorCode when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a95f4022-837f-4133-b3d6-2b71719a3c51
📒 Files selected for processing (2)
src/web_interface/errors.pyweb_interface/blueprints/api_v3.py
Comment on lines
+217
to
218
| message=cls._safe_message(error_code), | ||
| details=cls._get_exception_details(exception), |
There was a problem hiding this comment.
Prevent incorrect config error messaging when error_code is inferred
At Line 217, the message now depends entirely on inferred error_code. Because config exceptions are currently inferred as CONFIG_SAVE_FAILED, config-load failures can return “Failed to save configuration” with mismatched suggestions, which is misleading for users.
Suggested fix
@@
- if not error_code:
- error_code = cls._infer_error_code(exception)
+ if not error_code:
+ error_code = cls._infer_error_code(exception, context)
@@
- def _infer_error_code(cls, exception: Exception) -> ErrorCode:
+ def _infer_error_code(
+ cls,
+ exception: Exception,
+ context: Optional[Dict[str, Any]] = None
+ ) -> ErrorCode:
@@
- if "Config" in exception_name:
- return ErrorCode.CONFIG_SAVE_FAILED
+ if "Config" in exception_name:
+ operation = (context or {}).get("operation")
+ if operation == "save":
+ return ErrorCode.CONFIG_SAVE_FAILED
+ if operation == "load":
+ return ErrorCode.CONFIG_LOAD_FAILED
+ return ErrorCode.CONFIG_LOAD_FAILEDAs per coding guidelines, “Provide user-friendly error messages that explain what went wrong and potential solutions” and “Make intentions clear through naming and structure (Explicit over Implicit principle)”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web_interface/errors.py` around lines 217 - 218, The current code uses
cls._safe_message(error_code) when error_code may be inferred (e.g.,
CONFIG_SAVE_FAILED) and thus can produce misleading "Failed to save
configuration" text for config-load failures; update the logic in the error
construction so that if error_code was inferred (or does not match the exception
type) you use a generic, action-agnostic message or derive the message from the
exception type instead of always calling _safe_message(error_code). Locate the
code paths around the callsite that build the response (referencing
_safe_message, _get_exception_details, and the CONFIG_SAVE_FAILED symbol) and
change them to prefer exception-derived messages or a neutral fallback message
when inference is involved, ensuring suggestions match the actual operation
(load vs save).
…idate config handlers - _infer_error_code: map Config* exceptions to CONFIG_LOAD_FAILED (ConfigError is only raised by load_config(), so CONFIG_SAVE_FAILED produced wrong safe message and wrong suggested_fixes) - Remove leftover DEBUG logs in save_main_config that dumped full request body and all HTTP headers (Authorization, Cookie, etc.) - Replace dead FileNotFoundError/JSONDecodeError/IOError handlers in get_dim_schedule_config with single ConfigError catch (load_config already wraps these into ConfigError) - Remove redundant local `from src.exceptions import ConfigError` imports now covered by top-level import - Strip str(e) from client-facing error messages in dim schedule handler Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web_interface/blueprints/api_v3.py (1)
4576-4577:⚠️ Potential issue | 🟠 MajorStop logging raw/parsed plugin config on validation failures.
These lines can leak secrets/PII into logs (
request.formvalues and full parsed config). Log only field keys and a masked config snapshot.Safer logging pattern
- logger.error("[PluginConfig] Raw form data: %s", json.dumps({k: str(v)[:200] for k, v in form_data.items()}, indent=2)) - logger.error("[PluginConfig] Parsed config: %s", json.dumps(plugin_config, indent=2, default=str)) + logger.error("[PluginConfig] Raw form keys: %s", sorted(form_data.keys())) + masked_config = mask_secret_fields( + plugin_config, + schema.get("properties", {}) if schema else {} + ) + logger.error("[PluginConfig] Parsed config (masked): %s", json.dumps(masked_config, indent=2, default=str))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web_interface/blueprints/api_v3.py` around lines 4576 - 4577, The current error logging in the PluginConfig validation path prints raw form_data and plugin_config (logger.error calls referencing form_data and plugin_config) which can leak secrets; change these logger.error calls to only log the form field keys and a masked/obfuscated snapshot of the config (e.g., replace values with fixed-length stars or "<redacted>" for any non-empty value) and avoid serializing full values—use the same variables (form_data, plugin_config) to derive keys and the masked map before logging so structure is clear but no secrets/PII are emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web_interface/blueprints/api_v3.py`:
- Line 2722: In update_plugin, avoid calling logger.exception outside an active
exception and avoid duplicate logs: inside the except block for plugin updates
(where plugin_id and error_msg are available), remove the standalone
logger.exception("[PluginUpdate] Update failed for %s: %s", plugin_id,
error_msg) and consolidate logging to a single call that includes exception
context—either use logger.exception once inside the except or use
logger.error("[PluginUpdate] Update failed for %s: %s", plugin_id, error_msg,
exc_info=True); also remove the subsequent duplicate error/info calls so the
failure is logged only once with the proper exc_info.
---
Duplicate comments:
In `@web_interface/blueprints/api_v3.py`:
- Around line 4576-4577: The current error logging in the PluginConfig
validation path prints raw form_data and plugin_config (logger.error calls
referencing form_data and plugin_config) which can leak secrets; change these
logger.error calls to only log the form field keys and a masked/obfuscated
snapshot of the config (e.g., replace values with fixed-length stars or
"<redacted>" for any non-empty value) and avoid serializing full values—use the
same variables (form_data, plugin_config) to derive keys and the masked map
before logging so structure is clear but no secrets/PII are emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56c4d8a7-461e-40c2-ad4a-0e6aaee66098
📒 Files selected for processing (2)
src/web_interface/errors.pyweb_interface/blueprints/api_v3.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/web_interface/errors.py
- update_plugin: change logger.exception to logger.error in non-except branch (logger.exception outside an except block logs useless "NoneType: None" traceback) - update_plugin: remove duplicate logger.exception call in except block (was logging the same failure twice) - save_plugin_config validation: stop logging full plugin_config dict (can contain API keys, passwords, tokens) and raw form_data values; log only keys and validation errors instead Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
ConfigErrorto the exception handler indisplay_preview_generator()(follow-up to fix: narrow bare except blocks to specific exception types #282)config_manager.load_config()wrapsFileNotFoundError,JSONDecodeError, andOSErrorasConfigError— without catching it, a corrupt or missing config crashes the display preview SSE endpoint instead of falling back to 128×64 defaultsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit