Conversation
This comment has been minimized.
This comment has been minimized.
|
oh, no wonder you opened #230 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/rovr/config/schema.json (1)
107-111: Add a minimum bound forfps_decrease_factor.Without a lower bound, invalid values (e.g.,
0or negatives) can pass schema validation.🛠️ Suggested schema guard
"fps_decrease_factor": { "type": "integer", + "minimum": 1, "default": 1, "description": "FPS decrease factor" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rovr/config/schema.json` around lines 107 - 111, The fps_decrease_factor schema lacks a lower bound allowing 0 or negative values; update the fps_decrease_factor property in schema.json (the JSON object named "fps_decrease_factor") to include a "minimum": 1 (keeping "type": "integer" and "default": 1) so validation rejects zero and negative values.src/rovr/functions/path.py (1)
634-635: PreferPreviewTypes | Noneover a duplicatedLiteralunion.This keeps the return type synchronized with
PreviewTypesand avoids future drift.♻️ Suggested refactor
def match_mime_to_preview_type( widget: DOMNode, mime_type: str, -) -> ( - Literal["text", "image", "pdf", "archive", "folder", "remime", "resvg", "font", "video"] - | None -): +) -> PreviewTypes | None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rovr/functions/path.py` around lines 634 - 635, Replace the duplicated Literal union type with the shared alias: change the annotation that currently reads Literal["text","image","pdf","archive","folder","remime","resvg","font","video"] | None to use PreviewTypes | None so the signature (return or variable type) stays synchronized with the PreviewTypes definition; update any import or reference to PreviewTypes in the same module if needed and ensure type-checking passes for functions/methods using this annotation (e.g., the function or variable surrounding this annotation).
🤖 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/rovr/classes/config.py`:
- Around line 834-835: The config now allows the "video" type but the generated
MIME defaults and docs were not updated: update the
_ROVR_CONFIG_INTERFACE_MIME_RULES_DEFAULT mapping to include the new entry
"video/mp4": "video" (and any equivalent video MIME(s) needed) and update the
mime_rules documentation string (the variable/docblock used to describe valid
mime types) to list "video" among the valid types so generated artifacts remain
consistent with the enum change.
In `@src/rovr/config/schema.json`:
- Around line 123-126: The "show_controls" JSON Schema property has a type
mismatch: its "default" is the string "true" but the schema "type" is boolean;
update the "show_controls" property's "default" to the boolean true (not a
quoted string) so the default value matches the declared type and avoids leaking
incorrectly typed defaults.
---
Nitpick comments:
In `@src/rovr/config/schema.json`:
- Around line 107-111: The fps_decrease_factor schema lacks a lower bound
allowing 0 or negative values; update the fps_decrease_factor property in
schema.json (the JSON object named "fps_decrease_factor") to include a
"minimum": 1 (keeping "type": "integer" and "default": 1) so validation rejects
zero and negative values.
In `@src/rovr/functions/path.py`:
- Around line 634-635: Replace the duplicated Literal union type with the shared
alias: change the annotation that currently reads
Literal["text","image","pdf","archive","folder","remime","resvg","font","video"]
| None to use PreviewTypes | None so the signature (return or variable type)
stays synchronized with the PreviewTypes definition; update any import or
reference to PreviewTypes in the same module if needed and ensure type-checking
passes for functions/methods using this annotation (e.g., the function or
variable surrounding this annotation).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.tomlsrc/rovr/classes/config.pysrc/rovr/classes/type_aliases.pysrc/rovr/config/config.tomlsrc/rovr/config/schema.jsonsrc/rovr/core/preview_container.pysrc/rovr/functions/path.pysrc/rovr/variables/constants.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/rovr/classes/config.py (1)
834-835:⚠️ Potential issue | 🟠 MajorSync MIME defaults/docs with the new
videopreview type.You added
Literal["video"], but_ROVR_CONFIG_INTERFACE_MIME_RULES_DEFAULT(Line 75-95) still lacks"video/mp4": "video", and themime_rulesdocs at Line 620 still omitvideo.🛠️ Proposed fix
_ROVR_CONFIG_INTERFACE_MIME_RULES_DEFAULT = { @@ "application/x-font-.*": "font", + "video/mp4": "video", } @@ - Map MIME type patterns to preview types. Uses regex patterns. Valid preview types: text, image, pdf, archive, folder, resvg, font, remime. + Map MIME type patterns to preview types. Uses regex patterns. Valid preview types: text, image, pdf, archive, folder, resvg, font, remime, video. @@ image/svg\+xml: resvg inode/directory: folder + video/mp4: video text/.*: text """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rovr/classes/config.py` around lines 834 - 835, The config added Literal["video"] but the defaults/docs weren't updated: add the video MIME mapping to _ROVR_CONFIG_INTERFACE_MIME_RULES_DEFAULT (add "video/mp4": "video") and update the mime_rules documentation entry that lists mappings to include the video type so docs and defaults match the new preview type; reference the symbols _ROVR_CONFIG_INTERFACE_MIME_RULES_DEFAULT and the mime_rules doc block to make the edits.src/rovr/config/schema.json (1)
123-126:⚠️ Potential issue | 🟠 MajorFix
show_controlsdefault type mismatch.
show_controlsis declared as boolean, but Line 125 sets"default": "true"as a string. This can propagate wrong-typed config values.🛠️ Proposed fix
"show_controls": { "type": "boolean", - "default": "true", + "default": true, "description": "Whether to show player controls (pause button and time)" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rovr/config/schema.json` around lines 123 - 126, The schema entry for show_controls has a type mismatch: it's declared as boolean but the default is the string "true"; update the default to a boolean value (true) in the show_controls schema entry so the "default" matches the declared "type" (reference symbol: show_controls in schema.json).
🤖 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/rovr/config/schema.json`:
- Around line 107-110: The fps_decrease_factor schema currently allows zero and
negative values; add a lower bound to reject non-positive integers by adding a
"minimum": 1 constraint (or "exclusiveMinimum": 0 if you prefer) to the
fps_decrease_factor property in src/rovr/config/schema.json so validation
requires values >= 1; keep the existing "type": "integer" and "default": 3 and
update the description if needed to reflect the new constraint.
---
Duplicate comments:
In `@src/rovr/classes/config.py`:
- Around line 834-835: The config added Literal["video"] but the defaults/docs
weren't updated: add the video MIME mapping to
_ROVR_CONFIG_INTERFACE_MIME_RULES_DEFAULT (add "video/mp4": "video") and update
the mime_rules documentation entry that lists mappings to include the video type
so docs and defaults match the new preview type; reference the symbols
_ROVR_CONFIG_INTERFACE_MIME_RULES_DEFAULT and the mime_rules doc block to make
the edits.
In `@src/rovr/config/schema.json`:
- Around line 123-126: The schema entry for show_controls has a type mismatch:
it's declared as boolean but the default is the string "true"; update the
default to a boolean value (true) in the show_controls schema entry so the
"default" matches the declared "type" (reference symbol: show_controls in
schema.json).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/rovr/classes/config.pysrc/rovr/config/config.tomlsrc/rovr/config/schema.jsonsrc/rovr/core/preview_container.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/rovr/core/preview_container.py
|
This feature is capable of (and can) absolutely obliterate your memory. My memory usage spiked to 96% (it never hits 96%; memreduct keeps it below 90%). This is how badly this runs. Sixel Renderer is also pretty bad; it flickers quite a lot. Also, an extra few dependencies that I'm trying to avoid, like numpy and av, and maybe use the system's ffmpeg executable |
In future I want to add lazy loading
agree, refactor is planned |
Not just lazy loading, it also needs to destroy the objects. The thread also doesn't get cancelled when you exit (please use a
fair enough, this is python, what did I expect, but i was hoping it wouldn't be that often...
yes it also managed to use 100% of my memory just now... |
2 participants
Added video preview support
Added new config variables:
resolves #210
by submitting this pull request, i agree that
poe checkto check for any style issuesSummary by CodeRabbit
New Features
Chores