[codex] Extract magic literal constants#111
Merged
Merged
Conversation
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Ahmed-Hindy
marked this pull request as ready for review
June 16, 2026 12:55
📝 WalkthroughWalkthroughThe PR replaces inline hardcoded literals with named module-level constants across 12 files, and extracts the contact sheet label sizing logic into a new public helper ChangesMagic Number Extraction and Label Metrics Sharing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/renderkit/processing/video_encoder.py (2)
195-196:⚠️ Potential issue | 🟠 MajorStore
self._process.stdinas a non-optional instance attribute after the None check.Lines 202 and 209 fail type-checking because
self._process.stdinremains typed asOptional[IO]outside__init__. The None guard on line 195 does not narrow the type for later method calls. Assign it toself._stdinimmediately after the guard, then use that attribute inappend_data()andclose().Proposed patch
class _RawFfmpegPipeWriter: @@ if self._process.stdin is None: raise RuntimeError("FFmpeg stdin is not available for writing.") + self._stdin = self._process.stdin def append_data(self, frame: np.ndarray) -> None: @@ - self._process.stdin.write(frame.tobytes()) + self._stdin.write(frame.tobytes()) @@ def close(self) -> None: if self._process.poll() is None: try: - self._process.stdin.close() + self._stdin.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderkit/processing/video_encoder.py` around lines 195 - 196, The type-checker cannot narrow the Optional type of self._process.stdin even after the None guard on line 195, causing type errors in subsequent operations. After the None check guard in the method containing the RuntimeError, assign self._process.stdin to a new instance attribute self._stdin (typed as non-optional IO), then use self._stdin instead of self._process.stdin in both the append_data() method (around line 202) and the close() method (around line 209) to satisfy type-checking requirements.
252-252:⚠️ Potential issue | 🟠 MajorAdd explicit type annotations for
_writerand__exit__parameters.The
_writerattribute is initialized asNone(line 252) but assigned a_RawFfmpegPipeWriterinstance (line 460), causing type checker inference conflicts. The__exit__method parameters lack type annotations. Both need explicit typing.Proposed patch
+from types import TracebackType @@ - self._writer = None + self._writer: Optional[_RawFfmpegPipeWriter] = None @@ - def __exit__(self, exc_type, exc_val, exc_tb) -> None: + def __exit__( + self, + exc_type: Optional[type[BaseException]], + exc_val: Optional[BaseException], + exc_tb: Optional[TracebackType], + ) -> None: """Context manager exit - close video writer.""" self.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderkit/processing/video_encoder.py` at line 252, The _writer attribute is initialized as None but later assigned a _RawFfmpegPipeWriter instance, causing type checker inference conflicts, and the __exit__ method parameters lack type annotations. Add an explicit type annotation to the _writer attribute to indicate it can be either None or a _RawFfmpegPipeWriter instance (using Optional or Union typing), and add proper type annotations to all parameters of the __exit__ method to specify the types for the exception class, value, and traceback parameters.Source: Pipeline failures
src/renderkit/core/ffmpeg_utils.py (1)
122-126:⚠️ Potential issue | 🟠 MajorGuard Windows-only
subprocessattributes to resolve type-check failures.The
subprocess.CREATE_NO_WINDOWandsubprocess.CREATE_NEW_PROCESS_GROUPattributes are Windows-specific and not available in type stubs on other platforms, causing mypy to fail. Add runtime guards usinggetattr()with defaults to ensure type compatibility across environments.The proposed approach using
getattr(subprocess, "CREATE_NO_WINDOW", 0)is valid. However, ensure the patched code follows ruff formatting (line-length: 100, double quotes). Consider whether the codebase prefersgetattr()guards overTYPE_CHECKINGconditionals for similar platform-specific code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderkit/core/ffmpeg_utils.py` around lines 122 - 126, The Windows-specific subprocess constants (CREATE_NO_WINDOW and CREATE_NEW_PROCESS_GROUP) cause type-check failures on non-Windows platforms because they are not available in type stubs. In the code block where os.name equals "nt" and the creationflags variable is assigned, replace the direct attribute access to subprocess.CREATE_NO_WINDOW and subprocess.CREATE_NEW_PROCESS_GROUP with getattr() calls that provide a default value of 0 to ensure type compatibility across all platforms. Ensure the modified code follows ruff formatting standards with line-length of 100 characters and uses double quotes for strings.src/renderkit/logging_utils.py (1)
40-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace dynamic handler attributes with built-in handler names to unblock mypy.
Type-check is failing at Line 113, Line 127, and Line 145 because
renderkit_handleris not a declared attribute on handler types. UseHandler.set_name()/Handler.get_name()as the handler key contract.Proposed fix
def _has_handler(logger: logging.Logger, handler_key: str) -> bool: - return any( - getattr(handler, "renderkit_handler", None) == handler_key for handler in logger.handlers - ) + return any(handler.get_name() == handler_key for handler in logger.handlers) def _get_handler(logger: logging.Logger, handler_key: str) -> logging.Handler | None: for handler in logger.handlers: - if getattr(handler, "renderkit_handler", None) == handler_key: + if handler.get_name() == handler_key: return handler return None @@ if file_handler is None: file_handler = RotatingFileHandler( log_path, maxBytes=_LOG_FILE_MAX_BYTES, backupCount=_LOG_FILE_BACKUP_COUNT, encoding="utf-8", ) - file_handler.renderkit_handler = "file" + file_handler.set_name("file") file_handler.setFormatter(file_formatter) root_logger.addHandler(file_handler) @@ if not _has_handler(root_logger, "console"): console_handler = logging.StreamHandler() - console_handler.renderkit_handler = "console" + console_handler.set_name("console") console_handler.setLevel(log_level) console_handler.setFormatter(console_formatter) root_logger.addHandler(console_handler) @@ ui_handler = CallbackHandler(ui_sink) - ui_handler.renderkit_handler = "ui" + ui_handler.set_name("ui") ui_handler.setLevel(log_level) ui_handler.setFormatter(ui_formatter) root_logger.addHandler(ui_handler)Also applies to: 47-49, 113-113, 127-127, 145-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderkit/logging_utils.py` around lines 40 - 43, Replace the dynamic renderkit_handler attribute with the built-in handler name methods to resolve mypy type checking failures. In the _has_handler function (lines 40-43), replace the getattr call that checks for renderkit_handler with a call to handler.get_name() to retrieve the handler's built-in name. At lines 47-49 where renderkit_handler is being assigned to handlers, replace this with handler.set_name() to set the handler's built-in name instead. This same pattern should be applied consistently at lines 113, 127, and 145 where renderkit_handler is being used, replacing all getattr calls and assignments with the corresponding get_name() and set_name() method calls respectively.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/renderkit/core/ffmpeg_utils.py`:
- Around line 122-126: The Windows-specific subprocess constants
(CREATE_NO_WINDOW and CREATE_NEW_PROCESS_GROUP) cause type-check failures on
non-Windows platforms because they are not available in type stubs. In the code
block where os.name equals "nt" and the creationflags variable is assigned,
replace the direct attribute access to subprocess.CREATE_NO_WINDOW and
subprocess.CREATE_NEW_PROCESS_GROUP with getattr() calls that provide a default
value of 0 to ensure type compatibility across all platforms. Ensure the
modified code follows ruff formatting standards with line-length of 100
characters and uses double quotes for strings.
In `@src/renderkit/logging_utils.py`:
- Around line 40-43: Replace the dynamic renderkit_handler attribute with the
built-in handler name methods to resolve mypy type checking failures. In the
_has_handler function (lines 40-43), replace the getattr call that checks for
renderkit_handler with a call to handler.get_name() to retrieve the handler's
built-in name. At lines 47-49 where renderkit_handler is being assigned to
handlers, replace this with handler.set_name() to set the handler's built-in
name instead. This same pattern should be applied consistently at lines 113,
127, and 145 where renderkit_handler is being used, replacing all getattr calls
and assignments with the corresponding get_name() and set_name() method calls
respectively.
In `@src/renderkit/processing/video_encoder.py`:
- Around line 195-196: The type-checker cannot narrow the Optional type of
self._process.stdin even after the None guard on line 195, causing type errors
in subsequent operations. After the None check guard in the method containing
the RuntimeError, assign self._process.stdin to a new instance attribute
self._stdin (typed as non-optional IO), then use self._stdin instead of
self._process.stdin in both the append_data() method (around line 202) and the
close() method (around line 209) to satisfy type-checking requirements.
- Line 252: The _writer attribute is initialized as None but later assigned a
_RawFfmpegPipeWriter instance, causing type checker inference conflicts, and the
__exit__ method parameters lack type annotations. Add an explicit type
annotation to the _writer attribute to indicate it can be either None or a
_RawFfmpegPipeWriter instance (using Optional or Union typing), and add proper
type annotations to all parameters of the __exit__ method to specify the types
for the exception class, value, and traceback parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 26868aeb-4910-401e-823a-dcf665614cb5
📒 Files selected for processing (13)
src/renderkit/cli/main.pysrc/renderkit/core/converter.pysrc/renderkit/core/ffmpeg_utils.pysrc/renderkit/core/sequence_replacement.pysrc/renderkit/logging_utils.pysrc/renderkit/processing/burnin.pysrc/renderkit/processing/contact_sheet.pysrc/renderkit/processing/video_encoder.pysrc/renderkit/ui/main_window_logic.pysrc/renderkit/ui/main_window_ui.pysrc/renderkit/ui/timeline_controller.pysrc/renderkit/ui/widgets.pytests/test_converter.py
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
2 participants
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
Why
Gemini flagged a set of magic literals. Most were maintainability-only, but the contact-sheet label height estimate had drifted from the renderer formula, which could make converter output sizing inconsistent with actual contact-sheet rendering.
Validation
uv --native-tls run --extra dev ruff check src testsuv --native-tls run --extra dev pytest tests/test_contact_sheet.py tests/test_converter.py tests/test_cli.py tests/test_ui_logic_preview.py tests/test_main_window_settings.py tests/test_logging_utils.py tests/test_sequence_replacement.py tests/test_video_encoder.pyuv --native-tls run --extra dev pytestbefore hook formatting: 176 passed, 9 skippedSummary by CodeRabbit
Release Notes
Refactor
Tests