MPT-20734: add plug router metadata#186
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds declarative UI plug support: a frozen Plug Metadata and Static Assets
Estimated code review effort: Possibly related PRs:
📝 WalkthroughAdds declarative UI plug support: a frozen 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
svazquezco
requested review from
d3rky and
robcsegal
and removed request for
a team
svazquezco
force-pushed
the
codex/MPT-20734-plugrouter
branch
from
0fe7926 to
432a9c6
Compare
![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)
docs/usage.md (2)
143-172: ⚡ Quick winConsider adding router inclusion to the example.
The example shows how to define
plug_routerand register plug providers, but doesn't demonstrate how to include the router in the extension app. Adding this would make the example more complete for first-time users.📝 Suggested addition
After the example code block, consider adding:
# In your app.py ext_app = ExtensionApp(prefix="/api/v2") ext_app.include_router(plug_router)Or incorporate it into the main example to show the full flow.
🤖 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 `@docs/usage.md` around lines 143 - 172, The example defines PlugRouter and register_plugs but omits showing how to attach that router to an extension app; update the docs to include usage of ExtensionApp and include_router so readers see the full flow: after creating plug_router and register_plugs, instantiate ExtensionApp (e.g., ExtensionApp(prefix="...")) and call ext_app.include_router(plug_router) to register the plug providers with the app; reference the PlugRouter, plug_router, register_plugs, ExtensionApp, and include_router symbols in the added text.
170-172: 💤 Low valueConsider clarifying the input format for static paths.
The explanation states that
hrefandiconare "paths relative to the localstatic/folder" and that "The SDK normalizes them under/static/". While technically correct, an explicit example showing the transformation might help users understand the expected input format.📝 Suggested clarification
Consider adding an example transformation:
`href` and `icon` are paths relative to the local `static/` folder. For example, if you have `static/main-menu.js` in your extension package, specify `href="main-menu.js"` in the Plug definition. The SDK normalizes them under `/static/` in generated metadata (e.g., `/static/main-menu.js`), and `mpt-ext meta validate` checks that every referenced file exists locally.🤖 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 `@docs/usage.md` around lines 170 - 172, Add a short explicit example showing how to specify static paths for `href` and `icon` in the docs: explain that these fields should be given as filenames or paths relative to the local static/ folder (e.g., `main-menu.js` or `images/icon.png`), and state that the SDK will normalize them to `/static/<filename>` in generated metadata; reference the `href` and `icon` fields and the `mpt-ext meta validate` command so readers know the validator checks the referenced files exist locally and see the before/after transformation.
🤖 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.
Inline comments:
In `@mpt_extension_sdk/routing/routers/plug.py`:
- Around line 17-23: The code in decorator (function decorator) assumes
plug_provider has __name__, which will fail for callables like
functools.partial; update the name extraction when building PlugRouteDefinition
to guard against missing __name__ by using a safe fallback: try
getattr(plug_provider, "__name__", None), then if None check for a wrapped
function (e.g., getattr(plug_provider, "func", None) and its __name__), and
finally fall back to plug_provider.__class__.__name__ or str(plug_provider) to
produce a stable route name; apply this change where PlugRouteDefinition is
constructed in decorator so name= uses the guarded value.
In `@mpt_extension_sdk/runtime/builders.py`:
- Around line 12-21: The instance field _plug_ids is never cleared, so repeated
calls to PlugMetadataBuilder.build() will treat previously seen IDs as
duplicates; at the start of the build method reset/clear self._plug_ids (e.g.,
self._plug_ids.clear() or reassign a new set) before iterating, then proceed to
call self._iter_plugs(), self._validate_plug(...) and
self._create_meta_plug(...). Apply the same reset to the other build
implementation mentioned (the similar build method around lines 50-54) to ensure
each build run starts with a fresh seen-ID set.
---
Nitpick comments:
In `@docs/usage.md`:
- Around line 143-172: The example defines PlugRouter and register_plugs but
omits showing how to attach that router to an extension app; update the docs to
include usage of ExtensionApp and include_router so readers see the full flow:
after creating plug_router and register_plugs, instantiate ExtensionApp (e.g.,
ExtensionApp(prefix="...")) and call ext_app.include_router(plug_router) to
register the plug providers with the app; reference the PlugRouter, plug_router,
register_plugs, ExtensionApp, and include_router symbols in the added text.
- Around line 170-172: Add a short explicit example showing how to specify
static paths for `href` and `icon` in the docs: explain that these fields should
be given as filenames or paths relative to the local static/ folder (e.g.,
`main-menu.js` or `images/icon.png`), and state that the SDK will normalize them
to `/static/<filename>` in generated metadata; reference the `href` and `icon`
fields and the `mpt-ext meta validate` command so readers know the validator
checks the referenced files exist locally and see the before/after
transformation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6f7338aa-bbe8-4d0a-ae66-e79299231b6b
📒 Files selected for processing (21)
docs/architecture.mddocs/usage.mdmpt_extension_sdk/__init__.pympt_extension_sdk/cli/main.pympt_extension_sdk/extension_app.pympt_extension_sdk/routing/__init__.pympt_extension_sdk/routing/models.pympt_extension_sdk/routing/plugs.pympt_extension_sdk/routing/routers/plug.pympt_extension_sdk/routing/types.pympt_extension_sdk/runtime/app.pympt_extension_sdk/runtime/builders.pympt_extension_sdk/runtime/models.pytests/api/builders/test_api.pytests/api/test_pagination.pytests/api/test_responses.pytests/cli/test_main.pytests/routing/routers/test_plug.pytests/runtime/test_app.pytests/runtime/test_models.pytests/test_extension_app.py
svazquezco
force-pushed
the
codex/MPT-20734-plugrouter
branch
2 times, most recently
from
0bd70ee to
49f537c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mpt_extension_sdk/runtime/app.py (1)
72-76: ⚡ Quick winUse the shared static prefix constant for mount path.
This keeps runtime mount wiring aligned with plug path normalization/validation if the prefix ever changes.
♻️ Proposed refactor
@@ from fastapi import FastAPI, Request, Response from fastapi.staticfiles import StaticFiles @@ from mpt_extension_sdk.observability.bootstrap import ObservabilityBootstrap from mpt_extension_sdk.observability.config import ObservabilityConfig +from mpt_extension_sdk.routing.plugs import STATIC_PATH_PREFIX @@ app.mount( - "/static", + STATIC_PATH_PREFIX.removesuffix("/"), StaticFiles(directory=Path.cwd() / "static", check_dir=False), name="static", )🤖 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 `@mpt_extension_sdk/runtime/app.py` around lines 72 - 76, The mount path is hardcoded as "/static" in the app.mount call; replace that literal with the shared static prefix constant used across the project (e.g., STATIC_PREFIX or the module constant that normalizes plug paths) so runtime routing uses the same canonical prefix. Update the app.mount invocation that references StaticFiles(directory=Path.cwd() / "static", check_dir=False) to use the shared constant instead of the literal string and ensure any import or reference points to the module-exported constant the rest of the codebase uses for static/plug path normalization.
🤖 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.
Inline comments:
In `@mpt_extension_sdk/routing/plugs.py`:
- Around line 33-40: The branch that computes relative_path mishandles the bare
mount path (e.g. "/static" or "/static/") and causes incorrect normalization;
update the logic in the block that uses STATIC_PATH_PREFIX and cleaned_path so
that if cleaned_path exactly equals the mount prefix (with or without a trailing
slash) you treat it as invalid (raise ValueError) or normalize to an empty
relative path consistently, otherwise compute relative_path by removing the
prefix and stripping any leading slash (use
cleaned_path.removeprefix(STATIC_PATH_PREFIX).lstrip("/")). Ensure you still
validate path_parts via PurePosixPath(relative_path).parts and
cls._has_invalid_path_parts.
---
Nitpick comments:
In `@mpt_extension_sdk/runtime/app.py`:
- Around line 72-76: The mount path is hardcoded as "/static" in the app.mount
call; replace that literal with the shared static prefix constant used across
the project (e.g., STATIC_PREFIX or the module constant that normalizes plug
paths) so runtime routing uses the same canonical prefix. Update the app.mount
invocation that references StaticFiles(directory=Path.cwd() / "static",
check_dir=False) to use the shared constant instead of the literal string and
ensure any import or reference points to the module-exported constant the rest
of the codebase uses for static/plug path normalization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 46ef2941-92c0-4393-a6ec-77192cfea782
📒 Files selected for processing (23)
AGENTS.mddocs/architecture.mddocs/usage.mdmpt_extension_sdk/__init__.pympt_extension_sdk/cli/main.pympt_extension_sdk/extension_app.pympt_extension_sdk/routing/__init__.pympt_extension_sdk/routing/models.pympt_extension_sdk/routing/plugs.pympt_extension_sdk/routing/routers/plug.pympt_extension_sdk/routing/types.pympt_extension_sdk/runtime/app.pympt_extension_sdk/runtime/builders.pympt_extension_sdk/runtime/models.pytests/api/builders/test_api.pytests/api/test_pagination.pytests/api/test_responses.pytests/cli/test_main.pytests/routing/routers/test_plug.pytests/runtime/test_app.pytests/runtime/test_builders.pytests/runtime/test_models.pytests/test_extension_app.py
✅ Files skipped from review due to trivial changes (5)
- AGENTS.md
- mpt_extension_sdk/routing/models.py
- tests/api/test_responses.py
- docs/usage.md
- tests/api/test_pagination.py
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/runtime/test_builders.py
- mpt_extension_sdk/routing/types.py
- mpt_extension_sdk/runtime/models.py
- tests/api/builders/test_api.py
- tests/routing/routers/test_plug.py
- mpt_extension_sdk/runtime/builders.py
- tests/test_extension_app.py
- tests/cli/test_main.py
- mpt_extension_sdk/routing/routers/plug.py
- mpt_extension_sdk/extension_app.py
- tests/runtime/test_models.py
- tests/runtime/test_app.py
svazquezco
force-pushed
the
codex/MPT-20734-plugrouter
branch
from
49f537c to
1d19561
Compare
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mpt_extension_sdk/cli/main.py (1)
73-79: 💤 Low valueConsider direct attribute access instead of
getattr.Since
MetaConfig.plugsis a typed attribute (per the builder layer), you could simplify the iteration to use direct access:- for plug in getattr(meta_config, "plugs", None) or []: + for plug in meta_config.plugs or []:This preserves the same None-coalescing behavior while being more explicit about the expected type.
🤖 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 `@mpt_extension_sdk/cli/main.py` around lines 73 - 79, The loop uses getattr(meta_config, "plugs", None) but MetaConfig defines plugs directly; change the iteration in validate_plug_static_assets to use direct attribute access (meta_config.plugs or []) and keep the None-coalescing behavior, calling _validate_static_asset(plug.href, asset_root) and _validate_static_asset(plug.icon, asset_root) as before so the function still validates plug.href and optional plug.icon.
🤖 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.
Nitpick comments:
In `@mpt_extension_sdk/cli/main.py`:
- Around line 73-79: The loop uses getattr(meta_config, "plugs", None) but
MetaConfig defines plugs directly; change the iteration in
validate_plug_static_assets to use direct attribute access (meta_config.plugs or
[]) and keep the None-coalescing behavior, calling
_validate_static_asset(plug.href, asset_root) and
_validate_static_asset(plug.icon, asset_root) as before so the function still
validates plug.href and optional plug.icon.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b6e1b7eb-c40c-4e33-a1cc-df7d71209632
📒 Files selected for processing (23)
AGENTS.mddocs/architecture.mddocs/usage.mdmpt_extension_sdk/__init__.pympt_extension_sdk/cli/main.pympt_extension_sdk/extension_app.pympt_extension_sdk/routing/__init__.pympt_extension_sdk/routing/models.pympt_extension_sdk/routing/plugs.pympt_extension_sdk/routing/routers/plug.pympt_extension_sdk/routing/types.pympt_extension_sdk/runtime/app.pympt_extension_sdk/runtime/builders.pympt_extension_sdk/runtime/models.pytests/api/builders/test_api.pytests/api/test_pagination.pytests/api/test_responses.pytests/cli/test_main.pytests/routing/routers/test_plug.pytests/runtime/test_app.pytests/runtime/test_builders.pytests/runtime/test_models.pytests/test_extension_app.py
✅ Files skipped from review due to trivial changes (2)
- tests/api/test_responses.py
- docs/usage.md
🚧 Files skipped from review as they are similar to previous changes (16)
- AGENTS.md
- tests/runtime/test_builders.py
- tests/runtime/test_models.py
- mpt_extension_sdk/runtime/models.py
- tests/api/builders/test_api.py
- mpt_extension_sdk/routing/models.py
- tests/routing/routers/test_plug.py
- mpt_extension_sdk/extension_app.py
- mpt_extension_sdk/routing/routers/plug.py
- tests/cli/test_main.py
- mpt_extension_sdk/routing/plugs.py
- mpt_extension_sdk/runtime/builders.py
- mpt_extension_sdk/routing/types.py
- tests/api/test_pagination.py
- tests/runtime/test_app.py
- tests/test_extension_app.py
d3rky
approved these changes
May 19, 2026
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.
Closes MPT-20734