codeant-ai-for-open-source[bot] commented on code in PR #37025:
URL: https://github.com/apache/superset/pull/37025#discussion_r2678020861
##########
superset/extensions/local_extensions_watcher.py:
##########
@@ -91,9 +97,8 @@ def setup_local_extensions_watcher(app: Flask) -> None: #
noqa: C901
logger.warning("LOCAL_EXTENSIONS path does not exist: %s",
ext_path)
continue
- dist_path = ext_path / "dist"
- watch_dirs.append(str(dist_path))
- logger.info("Watching LOCAL_EXTENSIONS dist directory: %s", dist_path)
+ watch_dirs.append(str(ext_path))
+ logger.info("Watching LOCAL_EXTENSIONS directory: %s", ext_path)
Review Comment:
**Suggestion:** Bug when appending watch directories: the code appends the
resolved `ext_path` directly even if it is a file path, and can add duplicate
entries; ensure you watch a directory (use `ext_path` if it's a directory else
`ext_path.parent`) and avoid adding the same path more than once. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
watch_dir = ext_path if ext_path.is_dir() else ext_path.parent
watch_dir_str = str(watch_dir)
if watch_dir_str not in watch_dirs:
watch_dirs.append(watch_dir_str)
logger.info("Watching LOCAL_EXTENSIONS directory: %s",
watch_dir_str)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This change is sensible: ext_path may resolve to a file, and scheduling a
watcher with recursive=True on a file can be incorrect or surprising.
Normalizing to a directory (ext_path if it's a dir, else ext_path.parent) and
deduplicating avoids scheduling errors and redundant observers. The improved
snippet addresses a real logic issue.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/extensions/local_extensions_watcher.py
**Line:** 100:101
**Comment:**
*Logic Error: Bug when appending watch directories: the code appends
the resolved `ext_path` directly even if it is a file path, and can add
duplicate entries; ensure you watch a directory (use `ext_path` if it's a
directory else `ext_path.parent`) and avoid adding the same path more than once.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset/extensions/local_extensions_watcher.py:
##########
@@ -46,6 +46,10 @@ def on_any_event(self, event: Any) -> None:
if event.is_directory:
return
+ # Only trigger on changes to files in dist/ directory
+ if "/dist/" not in event.src_path:
Review Comment:
**Suggestion:** Cross-platform path check bug: the new check looks for the
substring "/dist/" which will fail on Windows (backslashes) and can miss or
incorrectly match some paths; use Path.parts (or os.path.sep-aware checks) and
guard for missing/non-string src_path to reliably detect a "dist" component.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
# Only trigger on changes to files in dist/ directory
(cross-platform)
src = getattr(event, "src_path", None)
if not isinstance(src, str):
return
if "dist" not in Path(src).parts:
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current substring check is brittle on Windows (backslashes) and will
also blow up or behave oddly if event.src_path is missing/not a string. The
proposed change to read src safely and check Path(src).parts for a "dist"
component fixes a real cross-platform correctness issue. Path is already
imported in the module, so the suggested approach fits cleanly.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/extensions/local_extensions_watcher.py
**Line:** 49:50
**Comment:**
*Possible Bug: Cross-platform path check bug: the new check looks for
the substring "/dist/" which will fail on Windows (backslashes) and can miss or
incorrectly match some paths; use Path.parts (or os.path.sep-aware checks) and
guard for missing/non-string src_path to reliably detect a "dist" component.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
