feat:skilljson and homescreen #283
Conversation
register utterance examples from skill.json with homescreen register handler with homescreen app drawer companion to OpenVoiceOS/ovos-skill-homescreen#130
WalkthroughThe changes introduce a new decorator function Changes
Possibly related PRs
Poem
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 35 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. 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: 5
🧹 Outside diff range and nitpick comments (1)
ovos_workshop/decorators/__init__.py (1)
162-177: LGTM! The implementation follows established patterns.The decorator implementation is clean and consistent with other decorators in the file.
Consider these improvements:
- Add type hints for better code maintainability:
-def homescreen_app(icon: str): +def homescreen_app(icon: str) -> callable:
- Add validation for the icon path:
def real_decorator(func): + if not isinstance(icon, str): + raise TypeError("Icon path must be a string") + if not icon: + raise ValueError("Icon path cannot be empty") func.homescreen_icon = icon return func
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
ovos_workshop/decorators/__init__.py(1 hunks)ovos_workshop/resource_files.py(4 hunks)ovos_workshop/skills/ovos.py(7 hunks)
🧰 Additional context used
🪛 Ruff
ovos_workshop/skills/ovos.py
861-861: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (5)
ovos_workshop/decorators/__init__.py (1)
166-166: Verify the 'gui' folder convention across the codebase.
The docstring states that icons must be in the 'gui' subfolder. Let's verify this convention.
ovos_workshop/resource_files.py (4)
16-16: LGTM!
The json import is correctly placed with other standard library imports and is necessary for the new JSON file handling functionality.
44-45: LGTM!
The new resource types 'qml' and 'json' are properly added to the SkillResourceTypes namedtuple, maintaining alphabetical ordering and alignment.
659-660: LGTM!
The resource type registration for 'qml' and 'json' is properly implemented:
- Correct file extensions are specified
- Language parameter is appropriately omitted as these are language-agnostic file types
- Maintains consistent formatting with existing entries
668-671: LGTM!
The load_json_file method is well-implemented:
- Follows the established pattern of other resource loading methods
- Uses sensible default for the common case of loading "skill.json"
- Maintains clean and focused functionality
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #283 +/- ##
==========================================
- Coverage 53.41% 51.64% -1.77%
==========================================
Files 37 36 -1
Lines 4362 4312 -50
==========================================
- Hits 2330 2227 -103
- Misses 2032 2085 +53 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
ovos_workshop/resource_files.py (2)
674-677: Add docstring to load_json_file methodThe method lacks documentation explaining its purpose and return type.
Apply this diff to add proper documentation:
def load_json_file(self, name: str = "skill.json") -> Dict[str, str]: + """Load and parse a JSON file from the skill's resources. + + Args: + name: Name of the JSON file (default: skill.json) + Returns: + Dict containing the parsed JSON data, or empty dict if file not found + """ jsonf = JsonFile(self.types.json, name) return jsonf.load()
259-270: Simplify directory structure logicThe nested if-else block for directory paths can be simplified for better maintainability.
Consider this more maintainable approach:
- if resource_subdirectory: - possible_directories = ( - Path(skill_directory, "locale", self.language), - Path(skill_directory, resource_subdirectory, self.language), - Path(skill_directory, resource_subdirectory), - Path(skill_directory, "text", self.language), - ) - else: - possible_directories = ( - Path(skill_directory, "locale", self.language), - Path(skill_directory, "text", self.language), - ) + possible_directories = [ + Path(skill_directory, "locale", self.language), + Path(skill_directory, "text", self.language) + ] + if resource_subdirectory: + possible_directories.extend([ + Path(skill_directory, resource_subdirectory, self.language), + Path(skill_directory, resource_subdirectory) + ])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_workshop/resource_files.py(7 hunks)
🔇 Additional comments (2)
ovos_workshop/resource_files.py (2)
44-45: LGTM: Resource type addition
The addition of the json resource type to SkillResourceTypes namedtuple is appropriate and follows the existing pattern.
665-666: LGTM: Resource type configuration
The addition of the JSON resource type in _define_resource_types is correctly implemented with appropriate parameters.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
ovos_workshop/resource_files.py (1)
674-677: Add method documentation and type hintsThe
load_json_filemethod needs proper documentation and type hints.Apply this diff to improve the documentation:
- def load_json_file(self, name: str = "skill.json") -> Dict[str, str]: + def load_json_file(self, name: str = "skill.json") -> Dict[str, Any]: + """Load and parse a JSON file from the skill's resources. + + Args: + name: Name of the JSON file to load (default: "skill.json") + Returns: + Dictionary containing the parsed JSON data + """ jsonf = JsonFile(self.types.json, name) return jsonf.load()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ovos_workshop/resource_files.py(8 hunks)ovos_workshop/skills/ovos.py(8 hunks)
🧰 Additional context used
🪛 Ruff
ovos_workshop/skills/ovos.py
861-861: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (5)
ovos_workshop/resource_files.py (2)
44-45: LGTM: Resource type additions are well-implemented
The addition of the JSON resource type follows the established pattern and is correctly implemented in both the SkillResourceTypes namedtuple and the resource_types dictionary.
Also applies to: 665-666
368-368: LGTM: Appropriate logging level adjustment
Changing the logging level from error to debug for missing resource files is appropriate, as this is often expected behavior and not necessarily an error condition.
ovos_workshop/skills/ovos.py (3)
817-819: Initialization sequence looks good
The calls to _register_skill_json(), _register_decorated(), _register_app_launcher(), and register_resting_screen() are correctly ordered to ensure proper initialization and registration with the homescreen.
1514-1518: Homescreen loaded event handled appropriately
The handle_homescreen_loaded method effectively re-registers the skill's metadata and app launcher when the homescreen is loaded, ensuring that all interfaces are up-to-date.
837-851:
Ensure skill_meta is not None before accessing
If resources.load_json_file("skill.json") returns None, calling .get() on skill_meta will raise an AttributeError. Consider adding a check to verify that skill_meta is not None before accessing it, preventing potential crashes.
Apply this diff to handle the potential None value:
skill_meta = resources.load_json_file("skill.json")
+ if not skill_meta:
+ self.log.warning(f"skill.json not found or failed to load for {lang}")
+ continue
utts = skill_meta.get("examples", [])Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
ovos_workshop/resource_files.py (2)
409-418: Enhance error handling in JsonFile class.The error handling could be more specific and informative:
- Separate handling for JSON parsing errors vs file I/O errors
- Include the specific error type in the log message
Consider this implementation:
class JsonFile(ResourceFile): def load(self) -> Dict[str, Any]: if self.file_path is not None: try: with open(self.file_path) as f: return json.load(f) - except Exception as e: - LOG.error(f"Failed to load {self.file_path}: {e}") + except json.JSONDecodeError as e: + LOG.error(f"Failed to parse JSON file {self.file_path}: {str(e)}") + except IOError as e: + LOG.error(f"Failed to read JSON file {self.file_path}: {str(e)}") return {}
412-416: Add docstring to JsonFile class.The class lacks documentation explaining its purpose and usage.
Add this docstring:
class JsonFile(ResourceFile): + """Defines a JSON file loader for skill configuration and metadata. + + This class handles loading and parsing of JSON files, which can contain + skill configuration, metadata, or other structured data. + """ def load(self) -> Dict[str, Any]:ovos_workshop/skills/ovos.py (1)
856-864: Optimize attribute access and add error handlingThe code can be improved by:
- Using direct attribute access instead of getattr (as flagged by static analysis)
- Adding error handling for missing attributes
Apply this diff to improve the code:
for attr_name in get_non_properties(self): method = getattr(self, attr_name) - if hasattr(method, 'homescreen_icon'): - event = f"{self.skill_id}.{method.__name__}.homescreen.app" - icon = getattr(method, 'homescreen_icon') + try: + if hasattr(method, 'homescreen_icon'): + event = f"{self.skill_id}.{method.__name__}.homescreen.app" + icon = method.homescreen_icon + self.register_homescreen_app(icon=icon, event=event) + self.add_event(event, method) + except AttributeError as e: + self.log.error(f"Failed to register app launcher: {e}") - self.register_homescreen_app(icon=icon, event=event) - self.add_event(event, method)🧰 Tools
🪛 Ruff
861-861: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ovos_workshop/resource_files.py(9 hunks)ovos_workshop/skills/ovos.py(9 hunks)
🧰 Additional context used
🪛 Ruff
ovos_workshop/skills/ovos.py
861-861: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (9)
ovos_workshop/resource_files.py (5)
45-46: LGTM: Resource type registration is properly implemented.
The json resource type is correctly added to both the SkillResourceTypes namedtuple and resource type initialization.
Also applies to: 652-653
661-664: LGTM: load_json_file method is well-implemented.
The method provides a clean interface for loading JSON files with a sensible default for skill.json.
358-358: LGTM: Appropriate log level change.
Changed from error to debug level for missing resource files, which is more appropriate as this is often an expected condition during resource lookup.
291-291: LGTM: Return type annotation is more accurate.
The Optional[str] return type better represents the actual behavior of the method, as some resource types may not have a subdirectory.
410-410: Verify return type annotation compatibility.
The return type Dict[str, Any] allows for nested JSON structures, but verify if all consumers of this API expect and handle nested structures correctly.
✅ Verification successful
Let me gather more information about how the loaded JSON data is actually used.
Return type annotation Dict[str, Any] is correctly specified and used appropriately
The verification shows that:
- The
load()method correctly returns a dictionary loaded directly from JSON usingjson.load() - The main consumer (
load_json_file) uses it to load "skill.json" which contains nested structures likeexampleslist - The loaded data is accessed safely using
.get()method which handles potential missing keys
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of load_json_file to verify type handling
# Test: Search for method calls to verify type expectations in consuming code
# Search for direct calls to load_json_file
rg "load_json_file\(" -A 3
# Search for JsonFile usage
rg "JsonFile\(" -A 3
Length of output: 1209
Script:
#!/bin/bash
# Find where skill_meta from load_json_file is used
rg "skill_meta" -A 5
# Find all implementations of JsonFile.load() to understand data handling
ast-grep --pattern 'def load(self) -> Dict[str, Any]:
$$$'
# Find where JsonFile is used to understand expected data structures
ast-grep --pattern 'JsonFile($$$)'
Length of output: 1851
ovos_workshop/skills/ovos.py (4)
817-819: LGTM: Startup sequence additions are well-placed
The new initialization methods are correctly placed after data files are loaded and before the skill's initialize method is called.
1514-1518: LGTM: Clean implementation of homescreen reload handler
The method correctly re-registers metadata when the homescreen loads, ensuring synchronization between the skill and homescreen interface.
845-846:
Add error handling for skill.json loading
The load_json_file call could return None if the file is not found or invalid. Add error handling to prevent potential NoneType errors.
Apply this diff to add error handling:
skill_meta = resources.load_json_file("skill.json")
+if not skill_meta:
+ self.log.debug(f'skill.json failed to load for {lang}')
+ continue
utts = skill_meta.get("examples", [])Likely invalid or redundant comment.
918-927:
Add security validation and error handling for icon file operations
The current implementation has several potential issues:
- No validation of icon path (potential path traversal)
- No error handling for file operations
- Missing directory creation
Apply this diff to address these issues:
def register_homescreen_app(self, icon: str, event: str):
"""the icon file MUST be located under 'gui' subfolder"""
+ # Validate icon path
+ if not icon or '..' in icon or '/' in icon or '\\' in icon:
+ self.log.error(f"Invalid icon path: {icon}")
+ return
+
GUI_CACHE_PATH = get_xdg_cache_save_path('ovos_gui')
full_icon_path = f"{self.root_dir}/gui/{icon}"
shared_path = f"{GUI_CACHE_PATH}/{self.skill_id}/{icon}"
- shutil.copy(full_icon_path, shared_path)
+
+ try:
+ # Verify source file exists and is a file
+ if not os.path.isfile(full_icon_path):
+ self.log.error(f"Icon file not found: {full_icon_path}")
+ return
+
+ # Create target directory
+ os.makedirs(os.path.dirname(shared_path), exist_ok=True)
+
+ # Copy file
+ shutil.copy2(full_icon_path, shared_path)
+ except (OSError, IOError) as e:
+ self.log.error(f"Failed to copy icon file: {e}")
+ returnLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
ovos_workshop/skills/ovos.py (1)
852-861: Use direct attribute access instead of getattrThe use of
getattrwith a constant attribute name is unnecessary.Apply this diff to improve code clarity:
def _register_app_launcher(self): # register app launcher if registered via decorator for attr_name in get_non_properties(self): method = getattr(self, attr_name) if hasattr(method, 'homescreen_icon'): - event = f"{self.skill_id}.{method.__name__}.homescreen.app" - icon = getattr(method, 'homescreen_icon') + event = f"{self.skill_id}.{method.__name__}.homescreen.app" + icon = method.homescreen_icon self.register_homescreen_app(icon=icon, event=event) self.add_event(event, method)🧰 Tools
🪛 Ruff
858-858: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_workshop/skills/ovos.py(10 hunks)
🧰 Additional context used
🪛 Ruff
ovos_workshop/skills/ovos.py
858-858: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (4)
ovos_workshop/skills/ovos.py (4)
5-5: LGTM: Required imports added
The new imports support the homescreen integration functionality:
shutilfor file operationsget_xdg_cache_save_pathfor GUI cache path handling
Also applies to: 28-28
1514-1518: Consider enabling app launcher registration
The _register_app_launcher() call is commented out. If this is intentional, consider adding a comment explaining why, or enable it if it should be active.
Run this script to check if any skills are using the app launcher functionality:
#!/bin/bash
# Description: Check for usage of homescreen_app decorator
# Test: Search for homescreen_app decorator usage. Expect: Files using the decorator
ast-grep --pattern 'homescreen_app'817-819: Review startup sequence order
The startup sequence has been modified to include homescreen integration. Consider:
- The order of operations - should
_register_decorated()come before_register_skill_json()? - Why
_register_app_launcher()is commented out
Run this script to check the impact of registration order:
✅ Verification successful
Registration order is correct and app launcher removal is justified
Based on the code analysis:
_register_skill_json()correctly comes first as it loads and registers basic skill metadata from skill.json_register_decorated()follows to handle runtime decorators and doesn't depend on the skill.json registration_register_app_launcher()being commented out is valid since:- Its functionality appears to be already covered by
_register_decorated() - The same comment pattern appears in
handle_homescreen_loaded()method - It only handled decorator-based app launchers which are now part of the broader decorator registration
- Its functionality appears to be already covered by
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any skills depend on a specific registration order
# Test: Search for dependencies between decorated methods and skill.json. Expect: No order dependencies.
# Check for decorated methods that use skill.json
ast-grep --pattern 'homescreen_app'
# Check for skill.json files that reference decorated methods
fd -e json -x grep -l "homescreen" {} \;
Length of output: 259
Script:
#!/bin/bash
# Let's analyze the registration methods and their dependencies
# Check _register_skill_json implementation
ast-grep --pattern '_register_skill_json() {
$$$
}'
# Check _register_decorated implementation
ast-grep --pattern '_register_decorated() {
$$$
}'
# Check _register_app_launcher implementation
ast-grep --pattern '_register_app_launcher() {
$$$
}'
# Look for any imports or dependencies between these methods
rg "_register_(skill_json|decorated|app_launcher)" -A 3 -B 3
Length of output: 6060
915-930:
Add security checks and error handling for icon file operations
The register_homescreen_app method needs:
- Path validation to prevent directory traversal
- Error handling for file operations
Apply this diff to improve security and robustness:
def register_homescreen_app(self, icon: str, event: str):
"""the icon file MUST be located under 'gui' subfolder"""
+ # Validate icon path
+ if os.path.isabs(icon) or '..' in icon or '/' in icon or '\\' in icon:
+ self.log.error(f"Invalid icon path: {icon}")
+ return
# this path is hardcoded in ovos_gui.constants and follows XDG spec
# we use it to ensure resource availability between containers
# it is the only path assured to be accessible both by skills and GUI
GUI_CACHE_PATH = get_xdg_cache_save_path('ovos_gui')
full_icon_path = f"{self.root_dir}/gui/{icon}"
shared_path = f"{GUI_CACHE_PATH}/{self.skill_id}/{icon}"
+ try:
+ if not os.path.isfile(full_icon_path):
+ self.log.error(f"Icon file not found: {full_icon_path}")
+ return
+ os.makedirs(os.path.dirname(shared_path), exist_ok=True)
+ shutil.copy(full_icon_path, shared_path)
+ except Exception as e:
+ self.log.error(f"Failed to copy icon file: {e}")
+ return
self.bus.emit(Message("homescreen.register.app",
{"skill_id": self.skill_id,
"icon": shared_path,
"event": event}))Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
ovos_workshop/resource_files.py (1)
JSON files should be stored in their own subdirectory
Based on the code analysis:
- JSON is a recognized resource type in the codebase (
JsonFileclass andjsonin resource types)- There's a dedicated method
load_json_filefor handling JSON resources- All JSON files found are currently under language-specific text directories
- Other resource types (dialog, vocab, regex, etc.) have dedicated subdirectories
Add "json" to the subdirectories mapping:
subdirectories = dict( dialog="dialog", entity="vocab", intent="vocab", list="dialog", named_value="dialog", regex="regex", template="dialog", vocab="vocab", word="dialog", qml="gui", json="json" # Add this line )🔗 Analysis chain
Line range hint
286-305: Consider adding JSON subdirectory supportThe
_get_resource_subdirectorymethod doesn't include a mapping for the new JSON resource type. Consider whether JSON files should have a dedicated subdirectory like other resource types.Let's check if other JSON files exist in the codebase and their locations:
Let me gather more information about JSON resource handling in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for JSON file locations in the codebase # Expected: Find common patterns in JSON file locations fd -e json --exec dirname {} | sort | uniq -c | sort -nrLength of output: 564
Script:
#!/bin/bash # Check for JSON-related code in resource handling rg -l "json" ovos_workshop/resource_files.py # If found, show the context rg "json|JSON" ovos_workshop/resource_files.py -i -C 5Length of output: 2006
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_workshop/resource_files.py(13 hunks)
🔇 Additional comments (4)
ovos_workshop/resource_files.py (4)
260-266: LGTM: Resource directory handling improvements
The changes improve the resource directory handling by:
- Maintaining a clear hierarchy of resource locations
- Properly handling language-specific resources
646-647: LGTM: Clean JSON resource type integration
The JSON resource type is properly integrated into the SkillResources class with:
- Correct type registration
- Clean implementation of load_json_file method
- Proper return type annotations
Also applies to: 655-658
352-352: LGTM: Appropriate log level adjustment
Changed log level from error to debug for missing resource files, which is more appropriate as this is often an expected condition during resource lookup.
322-322: LGTM: Type annotation improvement
Updated return type annotation to Optional[str] which better reflects that the method can return None.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
ovos_workshop/skills/ovos.py (1)
852-862: Improve attribute access in_register_app_launcherThe method uses
getattrunnecessarily for accessing constant attributes.Apply this diff to improve code clarity:
def _register_app_launcher(self): # register app launcher if registered via decorator for attr_name in get_non_properties(self): method = getattr(self, attr_name) if hasattr(method, 'homescreen_icon'): - icon = getattr(method, 'homescreen_icon') + icon = method.homescreen_icon event = f"{self.skill_id}.{method.__name__}.homescreen.app" LOG.debug(f"homescreen app registered: {event}") self.register_homescreen_app(icon=icon, event=event) self.add_event(event, method)🧰 Tools
🪛 Ruff
858-858: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_workshop/skills/ovos.py(10 hunks)
🧰 Additional context used
🪛 Ruff
ovos_workshop/skills/ovos.py
858-858: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (3)
ovos_workshop/skills/ovos.py (3)
1132-1134: LGTM: Homescreen reload handler
The handle_homescreen_loaded method correctly re-registers metadata when the homescreen is reloaded, ensuring synchronization between the skill and homescreen.
817-819: Verify the commented out registration call
The _register_app_launcher call is commented out, which may indicate incomplete implementation or testing. Please clarify if this is intentional or if it should be uncommented.
Run this script to check for any app launcher registrations in the codebase:
916-931:
Add security checks and error handling for icon file operations
The register_homescreen_app method needs path validation and error handling to prevent security issues and improve reliability.
Apply this diff to add security checks and error handling:
def register_homescreen_app(self, icon: str, event: str):
"""the icon file MUST be located under 'gui' subfolder"""
+ # Validate icon path to prevent directory traversal
+ if os.path.isabs(icon) or '..' in icon or '/' in icon or '\\' in icon:
+ self.log.error("Invalid icon filename provided")
+ return
# this path is hardcoded in ovos_gui.constants and follows XDG spec
# we use it to ensure resource availability between containers
# it is the only path assured to be accessible both by skills and GUI
GUI_CACHE_PATH = get_xdg_cache_save_path('ovos_gui')
full_icon_path = f"{self.root_dir}/gui/{icon}"
shared_path = f"{GUI_CACHE_PATH}/{self.skill_id}/{icon}"
+ try:
+ if not os.path.isfile(full_icon_path):
+ self.log.error(f"Icon file not found: {full_icon_path}")
+ return
+ os.makedirs(os.path.dirname(shared_path), exist_ok=True)
shutil.copy(full_icon_path, shared_path)
+ except Exception as e:
+ self.log.error(f"Failed to copy icon file: {e}")
+ return
self.bus.emit(Message("homescreen.register.app",
{"skill_id": self.skill_id,
"icon": shared_path,
"event": event}))Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ovos_workshop/skills/ovos.py (1)
818-819: Remove or uncomment_register_app_launchercallsThe
_register_app_launchermethod calls are commented out in both the startup sequence and homescreen reload handler. Either remove these lines or uncomment them if the functionality is needed.Also applies to: 1518-1519
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_workshop/skills/ovos.py(12 hunks)
🧰 Additional context used
🪛 Ruff
ovos_workshop/skills/ovos.py
858-858: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (6)
ovos_workshop/skills/ovos.py (6)
5-5: LGTM: Required imports added for new functionality
The new imports support the homescreen integration features:
shutilfor icon file operationsget_xdg_cache_save_pathfor GUI cache path handling
Also applies to: 28-28
837-851: LGTM: Skill metadata registration implementation
The method correctly loads and registers skill.json metadata with the homescreen, enabling example utterances to be displayed.
852-862: LGTM: App launcher registration implementation
The method properly handles registration of decorated methods as homescreen apps.
🧰 Tools
🪛 Ruff
858-858: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
916-931: LGTM: Homescreen app registration implementation
The method handles icon file copying and registration with the homescreen interface.
817-817: LGTM: Skill metadata registration added to startup
The startup sequence now properly includes registering skill metadata with the homescreen.
1132-1134: LGTM: Homescreen reload handler implementation
The handler ensures skill metadata is re-registered when the homescreen loads, maintaining consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
ovos_workshop/skills/ovos.py (1)
858-858: Use direct attribute access instead ofgetattrUsing
getattr(method, 'homescreen_icon')with a constant attribute name is unnecessary and reduces readability.Apply this diff to improve code clarity:
- icon = getattr(method, 'homescreen_icon') + icon = method.homescreen_icon🧰 Tools
🪛 Ruff
858-858: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_workshop/skills/ovos.py(12 hunks)
🧰 Additional context used
🪛 Ruff
ovos_workshop/skills/ovos.py
858-858: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (1)
ovos_workshop/skills/ovos.py (1)
2238-2239: LGTM: Improved error logging
The error message now includes the event name, which will help with debugging issues in event handlers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
ovos_workshop/skills/ovos.py (2)
852-862: Use direct attribute access for homescreen iconUsing
getattrwith a constant attribute name is unnecessary. Replace it with direct attribute access for better readability.Apply this diff to improve code clarity:
def _register_app_launcher(self): # register app launcher if registered via decorator for attr_name in get_non_properties(self): method = getattr(self, attr_name) if hasattr(method, 'homescreen_icon'): event = f"{self.skill_id}.{method.__name__}.homescreen.app" - icon = getattr(method, 'homescreen_icon') + icon = method.homescreen_icon LOG.debug(f"homescreen app registered: {event}") self.register_homescreen_app(icon=icon, event=event) self.add_event(event, method)🧰 Tools
🪛 Ruff
858-858: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
1513-1516: Add error handling for homescreen metadata registrationThe
handle_homescreen_loadedmethod should include error handling to prevent failures from affecting the homescreen.Apply this diff to improve robustness:
def handle_homescreen_loaded(self, message: Message): """homescreen loaded, we should re-register any metadata we want to provide""" - self._register_skill_json() - self._register_app_launcher() + try: + self._register_skill_json() + self._register_app_launcher() + except Exception as e: + self.log.error(f"Failed to register homescreen metadata: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ovos_workshop/res/text/en/skill.error.dialog(1 hunks)ovos_workshop/skills/ovos.py(13 hunks)
✅ Files skipped from review due to trivial changes (1)
- ovos_workshop/res/text/en/skill.error.dialog
🧰 Additional context used
🪛 Ruff
ovos_workshop/skills/ovos.py
858-858: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (1)
ovos_workshop/skills/ovos.py (1)
916-931:
Add security checks and error handling for icon file operations
The register_homescreen_app method needs:
- Input validation to prevent directory traversal
- Error handling for file operations
- Directory creation before copy operation
Apply this diff to improve security and robustness:
def register_homescreen_app(self, icon: str, event: str):
"""the icon file MUST be located under 'gui' subfolder"""
+ # Validate icon path to prevent directory traversal
+ if os.path.isabs(icon) or '..' in icon or '/' in icon or '\\' in icon:
+ self.log.error(f"Invalid icon path: {icon}")
+ return
# this path is hardcoded in ovos_gui.constants and follows XDG spec
# we use it to ensure resource availability between containers
# it is the only path assured to be accessible both by skills and GUI
GUI_CACHE_PATH = get_xdg_cache_save_path('ovos_gui')
full_icon_path = f"{self.root_dir}/gui/{icon}"
shared_path = f"{GUI_CACHE_PATH}/{self.skill_id}/{icon}"
- shutil.copy(full_icon_path, shared_path)
+ try:
+ if not os.path.isfile(full_icon_path):
+ self.log.error(f"Icon file not found: {full_icon_path}")
+ return
+ os.makedirs(os.path.dirname(shared_path), exist_ok=True)
+ shutil.copy(full_icon_path, shared_path)
+ except Exception as e:
+ self.log.error(f"Failed to copy icon file: {e}")
+ return
self.bus.emit(Message("homescreen.register.app",
{"skill_id": self.skill_id,
"icon": shared_path,
"event": event}))Likely invalid or redundant comment.
JarbasAl
force-pushed
the
feat_homescreen
branch
from
bbff2a2 to
51e5457
Compare
JarbasAl
force-pushed
the
feat_homescreen
branch
from
51e5457 to
636dc88
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
ovos_workshop/decorators/__init__.py (2)
163-170: Enhance docstring with usage example and validation note.The docstring should include a usage example and clarify the icon path validation requirements.
Consider updating the docstring:
""" Decorator for adding a method as a homescreen app - the icon file MUST be located under 'gui' subfolder - @param icon: icon file to use in app drawer (relative to "gui" folder) @param name: short name to show under the icon in app drawer + + Args: + icon (str): icon file to use in app drawer (relative to "gui" folder) + name (Optional[str]): short name to show under the icon in app drawer + + Example: + @homescreen_app(icon="images/my_app.png", name="My App") + def my_app_handler(self): + # Handler code + pass + + Note: + The icon file MUST exist in the 'gui' subfolder of your skill. + Supported image formats: png, jpg, svg """
172-177: Consider adding icon path validation.The decorator accepts any string as the icon path without validation. This could lead to runtime errors if the icon file doesn't exist or is in the wrong format.
Consider adding basic validation:
def real_decorator(func): + if not isinstance(icon, str): + raise ValueError("Icon path must be a string") + if not icon.lower().endswith(('.png', '.jpg', '.jpeg', '.svg')): + raise ValueError("Icon must be a PNG, JPG, or SVG file") # Store the icon inside the function # This will be used later to call register_homescreen_app func.homescreen_app_icon = icon func.homescreen_app_name = name return funcNote: Full path validation should be done at runtime when the skill is loaded, as the skill's root path is not available during decoration.
ovos_workshop/settings.py (1)
119-119: Appropriate log level adjustment for pairing status.The change from error to debug level is appropriate here since the
@requires_backenddecorator already handles the pairing requirement. This reduces unnecessary error logs while still maintaining visibility for debugging purposes.Consider documenting the expected log levels in a central logging guidelines document to maintain consistency across the codebase.
ovos_workshop/skills/ovos.py (1)
852-866: Use direct attribute access instead of getattrThe method correctly handles app launcher registration, but uses unnecessary getattr calls.
Apply this diff to improve code clarity:
if hasattr(method, 'homescreen_app_icon'): - name = getattr(method, 'homescreen_app_name') - icon = getattr(method, 'homescreen_app_icon') + name = method.homescreen_app_name + icon = method.homescreen_app_icon event = f"{self.skill_id}.{name or method.__name__}.homescreen.app" name = name or self.__skill_id2name🧰 Tools
🪛 Ruff
857-857: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
859-859: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
ovos_workshop/resource_files.py (1)
Line range hint
285-315: Update method documentation to reflect return type changesThe
_locatemethod inResourceFilenow returnsOptional[str]. Ensure the method's docstring and any inline comments accurately reflect this change for clarity.Apply this diff to update the docstring:
def _locate(self) -> Optional[str]: """Locates a resource file in the skill's locale directory. A skill's locale directory can contain a subdirectory structure defined by the skill author. Walk the directory and any subdirectories to find the resource file. + Returns: + The path to the resource file if found, otherwise `None`. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
ovos_workshop/decorators/__init__.py(2 hunks)ovos_workshop/res/text/en/skill.error.dialog(1 hunks)ovos_workshop/resource_files.py(15 hunks)ovos_workshop/settings.py(3 hunks)ovos_workshop/skills/ovos.py(16 hunks)test/unittests/test_resource_files.py(0 hunks)
💤 Files with no reviewable changes (1)
- test/unittests/test_resource_files.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ovos_workshop/res/text/en/skill.error.dialog
🧰 Additional context used
🪛 Ruff
ovos_workshop/skills/ovos.py
857-857: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
859-859: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🔇 Additional comments (14)
ovos_workshop/decorators/__init__.py (2)
2-2: LGTM!
The import of Optional from typing is appropriate for the new decorator's type hints.
162-179: Verify homescreen integration implementation.
This decorator is part of a larger feature involving skill.json and homescreen integration. Let's verify the implementation details.
✅ Verification successful
Let me gather more information about the implementation details.
Homescreen integration is properly implemented
The verification shows a complete implementation chain:
- The
@homescreen_appdecorator correctly stores icon and name attributes - OVOSSkill class automatically detects decorated methods and registers them via
register_homescreen_app - Registration logic handles icon paths and XDG spec compliance
- Event system integration is in place for app activation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the homescreen app registration implementation
# Check for companion implementation in skill-ovos-homescreen
echo "Checking for homescreen app registration implementation..."
rg -l "homescreen_app_(icon|name)" --type py
# Check for usage examples in the codebase
echo "Checking for usage examples..."
ast-grep --pattern 'homescreen_app($$$)'
Length of output: 353
Script:
#!/bin/bash
# Check the implementation in ovos_workshop/skills/ovos.py
echo "Checking ovos.py implementation..."
rg "homescreen_app" ovos_workshop/skills/ovos.py -B 2 -A 5
# Check if there's any registration logic
echo "Checking for registration logic..."
rg "register.*homescreen" --type py -B 2 -A 5
Length of output: 5244
ovos_workshop/settings.py (3)
129-129: LGTM - Consistent with previous log level adjustment.
This change maintains consistency with the previous log level adjustment in the upload method.
Line range hint 119-139: Verify alignment with PR objectives.
While these logging changes improve the code quality, they seem tangential to the main PR objectives of skill.json and homescreen integration. Please verify if there are additional files that implement the core functionality described in the PR objectives.
#!/bin/bash
# Description: Look for homescreen and skill.json related changes
# Search for homescreen related files
echo "=== Searching for homescreen related files ==="
fd -e py -e json . | rg -i "homescreen"
# Search for skill.json handling
echo "=== Searching for skill.json handling ==="
rg -l "skill.*\.json"139-139: Verify impact on log volume.
While the log level change is consistent with the previous adjustments, since this method is called periodically by a timer (every 60 seconds), we should verify that this won't generate excessive debug logs.
✅ Verification successful
Debug logging in periodic download operation is acceptable
The debug logging is minimal and appropriate for this context:
- Only one debug message is logged during the download operation
- The download operation runs every 60 seconds (1 minute), which is a reasonable interval
- The debug message is only shown when a specific condition is met (device needs pairing)
- There are only 3 total debug log messages in the settings module, all related to device pairing status
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential log volume impact
# Look for other periodic operations that might log at debug level
# Search for Timer usage that might involve logging
rg -B 5 -A 5 "Timer\("
# Search for debug logging in periodic operations
rg -B 5 -A 5 "LOG\.debug.*periodic|interval|timer" --ignore-case
Length of output: 434
Script:
#!/bin/bash
# Let's try a different approach to find timer-related code and logging patterns
# Search for Timer-related imports and usage
rg -B 2 -A 2 "from threading import Timer|Timer\("
# Look for the specific download method and its timer setup
rg -B 10 -A 10 "def download.*settings"
# Search for debug logging patterns in settings.py
rg "LOG\.debug" ovos_workshop/settings.py
# Look for timer interval configuration
rg "interval|INTERVAL|timer_interval" ovos_workshop/settings.py
Length of output: 971
ovos_workshop/skills/ovos.py (4)
817-819: LGTM: Proper initialization sequence for homescreen integration
The registration methods are correctly placed after loading data files, ensuring all required resources are available for homescreen integration.
1505-1509: LGTM: Proper handling of homescreen reload
The method correctly re-registers skill metadata and app launcher when the homescreen is loaded, ensuring proper synchronization.
837-851: 🛠️ Refactor suggestion
Add validation for utterances before registration
While the method correctly handles missing files and registration, it should validate utterances before sending them to the homescreen.
Apply this diff to add validation:
skill_meta = resources.load_json_file("skill.json")
utts = skill_meta.get("examples", [])
+ # Validate utterances are non-empty strings
+ utts = [u for u in utts if isinstance(u, str) and u.strip()]
if utts:
self.log.info(f"Registering example utterances with homescreen for lang: {lang} - {utts}")
self.bus.emit(Message("homescreen.register.examples",
{"skill_id": self.skill_id, "utterances": utts, "lang": lang}))Likely invalid or redundant comment.
926-942:
Add error handling for file operations
The method needs error handling for file operations to prevent crashes and ensure proper icon registration.
Apply this diff to improve robustness:
full_icon_path = f"{self.root_dir}/gui/{icon}"
shared_path = f"{GUI_CACHE_PATH}/{self.skill_id}/{icon}"
- shutil.copy(full_icon_path, shared_path)
+ try:
+ if not os.path.isfile(full_icon_path):
+ self.log.error(f"Icon file not found: {full_icon_path}")
+ return
+ os.makedirs(os.path.dirname(shared_path), exist_ok=True)
+ shutil.copy(full_icon_path, shared_path)
+ except Exception as e:
+ self.log.error(f"Failed to copy icon file: {e}")
+ return
self.bus.emit(Message("homescreen.register.app",
{"skill_id": self.skill_id,
"icon": shared_path,
"name": name,
"event": event}))Likely invalid or redundant comment.
ovos_workshop/resource_files.py (5)
365-366: Add class documentation to JsonFile
Consider adding a docstring to the JsonFile class to provide clarity on its purpose and usage. This enhances maintainability and aids other developers in understanding the class functionality.
371-372: Enhance error handling in JsonFile.load method
As previously noted, improving the error handling by catching specific exceptions like json.JSONDecodeError and IOError will provide more precise debugging information and prevent masking other exceptions.
609-610:
Verify handling of resource types without language
The qml resource type is added without specifying a language (language=None). Ensure that methods relying on the language attribute of ResourceType can handle cases where it is None, particularly in functions like locate_lang_directories and resource loading mechanisms.
Review and test the resource loading functions to confirm they gracefully handle resource types without an associated language.
223-229: Review directory paths for resource localization
In the locate_base_directory method, the possible_directories list is updated to include paths based on the resource_subdirectory and language. Ensure that the constructed paths are accurate and consider edge cases where resource_subdirectory or language might be None.
Run the following script to test directory paths construction:
#!/bin/bash
# Description: Check for cases where resource_subdirectory or language might be None.
# Test cases where resource_subdirectory or language is None
ast-grep --pattern $'def locate_base_directory($_):\n $$$\n resource_subdirectory = $_.get_resource_subdirectory()\n $$$\n possible_directories = [$_(skill_directory, "locale", self.language)]\n $$$' ovos_workshop/resource_files.pyLine range hint 249-268: Ensure consistent handling of resource subdirectories
The _get_resource_subdirectory method now returns Optional[str]. Verify that all usages of this method properly handle the None case to avoid potential TypeError exceptions when None is used where a str is expected.
Run the following script to identify any usages of _get_resource_subdirectory that may not handle None correctly:
✅ Verification successful
None handling is properly implemented
The code correctly handles the Optional[str] return value from _get_resource_subdirectory(). Specifically:
- In
locate_lang_directories, the return value is passed directly to another function, which is safe forNonevalues - In the other usage, there's an explicit check
if resource_subdirectory:before using the value in path construction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of _get_resource_subdirectory and check for None handling.
# Search for method calls to _get_resource_subdirectory
rg -A3 '_get_resource_subdirectory\('
# Check for any concatenations or operations assuming the return value is a string
rg '_get_resource_subdirectory\(\).*?\+' -A1
Length of output: 242
Script:
#!/bin/bash
# Let's try a different approach to find usages of _get_resource_subdirectory
# Search for method calls using ast-grep to find the method definition and its usages
ast-grep --pattern 'def _get_resource_subdirectory($_) {
$$$
}'
# Also search for any direct references to this method
rg "_get_resource_subdirectory" -B2 -A2
# Search for the class that contains this method to understand the usage context
ast-grep --pattern 'class $_ {
$$$
_get_resource_subdirectory($_) {
$$$
}
$$$
}'
Length of output: 1277
Script:
#!/bin/bash
# Let's examine how the return value is used in the calling methods
# Search for the methods that use _get_resource_subdirectory
ast-grep --pattern 'def locate_lang_directories($$$) {
$$$
}'
# Also get the context of where resource_subdirectory is used
rg "resource_subdirectory =" -B5 -A10 ovos_workshop/resource_files.py
Length of output: 1473
* fix:modernize homescreen integration companion to OpenVoiceOS/OVOS-workshop#283 * packaging * fix:add app name * ovos-workshop>=2.4.0,<3.0.0
register utterance examples from skill.json with homescreen
register handler with homescreen app drawer
companion to OpenVoiceOS/ovos-skill-homescreen#130
example decorator usage from local media skill
Summary by CodeRabbit
Release Notes
New Features
homescreen_appdecorator for registering applications with an icon on the homescreen.register_homescreen_appmethod for skills to register application icons with the homescreen.Bug Fixes