feat: allow goodmaps plugins#379
Open
raven-wing wants to merge 9 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds capability-based plugin bases, updates plugin discovery and manifests, routes frontend rendering through type-aware field and overlay components, and refreshes plugin docs and tests. ChangesOverlay and field plugin support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
goodmap/goodmap.py (1)
70-84: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing
capabilityon a plugin surfaces as a misleading warning.
plugin_class.capability(Line 79) has no default inGoodmapPluginBase; if a third-party plugin author forgets to set it, this raisesAttributeError, caught by the blindexcept Exceptionat Lines 82-84 and reported as"Failed to serve static files for plugin '%s'"— misattributing a plugin-contract violation to a static-file problem.🐞 Suggested fix to disambiguate the failure mode
try: plugin_class = ep.load() + if not hasattr(plugin_class, "capability"): + logger.warning( + "Plugin '%s' does not declare a 'capability'; skipping", ep.name + ) + return None, None mod_path = os.path.dirname(os.path.realpath(inspect.getfile(plugin_class)))🤖 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 `@goodmap/goodmap.py` around lines 70 - 84, The plugin manifest builder in goodmap.py is swallowing a missing plugin capability as a static-file failure, which makes the warning misleading. Update the logic around the manifest_entry construction in the function that returns bp and manifest_entry to handle plugin_class.capability explicitly before the broad except, and emit a warning that names the plugin contract issue when the capability is absent. Keep the static-file error path separate so the existing logger.warning in the exception handler is only used for actual serving failures, not for missing capability on GoodmapPluginBase implementations.
🧹 Nitpick comments (1)
frontend/tests/plugins/MapOverlays.test.jsx (1)
30-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a render-based assertion for missing overlay config, not just
getPluginConfig.This test only asserts
getPluginConfigdefaulting; it doesn't verify thatMapOverlaysactually passes a safe ({}) config to the mounted component when none was registered. Once theregisterPlugin/getOverlayPluginsdefaulting fix (seepluginRegistry.js) lands, a render assertion here would guard against regression.✅ Suggested additional coverage
it('exposes the registered config via getPluginConfig and defaults to {}', () => { const Noop = () => null; act(() => registerPlugin('with-config', Noop, { a: 1 }, 'overlay')); act(() => registerPlugin('without-config', Noop, undefined, 'overlay')); expect(getPluginConfig('with-config')).toEqual({ a: 1 }); expect(getPluginConfig('without-config')).toEqual({}); }); + + it('renders overlay plugin with a safe default config when none is registered', () => { + const Receiver = ({ config }) => <span>{JSON.stringify(config)}</span>; + act(() => registerPlugin('no-config-overlay', Receiver, undefined, 'overlay')); + + render(<MapOverlays isMapLoading={false} />); + + expect(screen.getByText('{}')).toBeInTheDocument(); + });🤖 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 `@frontend/tests/plugins/MapOverlays.test.jsx` around lines 30 - 37, The current MapOverlays test only checks getPluginConfig defaulting and does not verify the mounted overlay receives a safe config object. Update the MapOverlays.test.jsx case around registerPlugin/getPluginConfig to also render MapOverlays with the registered overlay plugin and assert the Noop overlay component is invoked with an empty {} config when none was registered. Use the existing getOverlayPlugins/MapOverlays behavior and the Noop component in this test to confirm the render path matches the defaulting fix.
🤖 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 `@docs/plugins.rst`:
- Around line 68-72: The manifest example in the plugin docs is using the wrong
field name for routing. Update the example and surrounding text in
docs/plugins.rst to document capability instead of kind, matching the actual
manifest contract used by Goodmap. Refer to the manifest entry shape described
near the Goodmap bundle path and ensure the routing explanation reflects
capability: "overlay" for MapOverlayPluginBase plugins and the non-overlay case
for PluginSlot.
In `@frontend/src/plugins/pluginRegistry.js`:
- Around line 4-7: `registerPlugin` is storing `config` as-is, which leaves
overlay plugins with `undefined` config while `getPluginConfig` applies a `{}`
fallback; normalize the default in `registerPlugin` so every registry entry
always has a config object. Update the registry entry creation used by
`getPluginConfig` and `getOverlayPlugins` so both paths stay consistent and
`MapOverlays`/`MapComponentInner` never pass `undefined` to overlay components.
In `@goodmap/goodmap.py`:
- Around line 179-183: The plugins config read path in goodmap.py is using a
blind except around app.db.get_plugins_data(), which hides the underlying DB
failure and triggers Ruff BLE001. Update the try/except to catch the specific
exception type(s) that get_plugins_data() can raise, and keep the warning
context while preserving the actual error details in the log. Use the
get_plugins_data() call site and the surrounding frontend plugins fallback logic
to locate the block.
---
Outside diff comments:
In `@goodmap/goodmap.py`:
- Around line 70-84: The plugin manifest builder in goodmap.py is swallowing a
missing plugin capability as a static-file failure, which makes the warning
misleading. Update the logic around the manifest_entry construction in the
function that returns bp and manifest_entry to handle plugin_class.capability
explicitly before the broad except, and emit a warning that names the plugin
contract issue when the capability is absent. Keep the static-file error path
separate so the existing logger.warning in the exception handler is only used
for actual serving failures, not for missing capability on GoodmapPluginBase
implementations.
---
Nitpick comments:
In `@frontend/tests/plugins/MapOverlays.test.jsx`:
- Around line 30-37: The current MapOverlays test only checks getPluginConfig
defaulting and does not verify the mounted overlay receives a safe config
object. Update the MapOverlays.test.jsx case around
registerPlugin/getPluginConfig to also render MapOverlays with the registered
overlay plugin and assert the Noop overlay component is invoked with an empty {}
config when none was registered. Use the existing getOverlayPlugins/MapOverlays
behavior and the Noop component in this test to confirm the render path matches
the defaulting fix.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4975b1f8-f1b9-42a6-bc59-b02495bef143
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
docs/plugins.rstfrontend/src/components/Map/MapComponent.jsxfrontend/src/plugins/GlobalPlugins.jsxfrontend/src/plugins/MapOverlays.jsxfrontend/src/plugins/pluginLoader.jsfrontend/src/plugins/pluginRegistry.jsfrontend/tests/plugins/MapOverlays.test.jsxfrontend/tests/plugins/PluginSlot.test.jsxgoodmap/__init__.pygoodmap/goodmap.pygoodmap/plugin.pypyproject.tomltests/unit_tests/test_goodmap.py
💤 Files with no reviewable changes (1)
- frontend/src/plugins/GlobalPlugins.jsx
Comment on lines
+179
to
+183
| try: | ||
| plugins_data = app.db.get_plugins_data() | ||
| except Exception: | ||
| logger.warning("Could not read plugin config data; frontend plugins get empty config") | ||
| plugins_data = {} |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Blind except Exception masks the real DB failure.
Catching all exceptions when reading get_plugins_data() and silently falling back to {} hides the actual error type, making production diagnostics harder.
🔧 Suggested improvement to preserve diagnostic detail
try:
plugins_data = app.db.get_plugins_data()
- except Exception:
- logger.warning("Could not read plugin config data; frontend plugins get empty config")
+ except Exception:
+ logger.warning(
+ "Could not read plugin config data; frontend plugins get empty config",
+ exc_info=True,
+ )
plugins_data = {}As per static analysis hints, Ruff flags this as BLE001 ("Do not catch blind exception: Exception").
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| plugins_data = app.db.get_plugins_data() | |
| except Exception: | |
| logger.warning("Could not read plugin config data; frontend plugins get empty config") | |
| plugins_data = {} | |
| try: | |
| plugins_data = app.db.get_plugins_data() | |
| except Exception: | |
| logger.warning( | |
| "Could not read plugin config data; frontend plugins get empty config", | |
| exc_info=True, | |
| ) | |
| plugins_data = {} |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 181-181: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@goodmap/goodmap.py` around lines 179 - 183, The plugins config read path in
goodmap.py is using a blind except around app.db.get_plugins_data(), which hides
the underlying DB failure and triggers Ruff BLE001. Update the try/except to
catch the specific exception type(s) that get_plugins_data() can raise, and keep
the warning context while preserving the actual error details in the log. Use
the get_plugins_data() call site and the surrounding frontend plugins fallback
logic to locate the block.
Source: Linters/SAST tools
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@frontend/src/plugins/PluginSlot.jsx`:
- Around line 5-15: The PluginSlot component keeps rendering a stale plugin when
pluginName changes because useEffect only resubscribes and never refreshes the
Plugin state immediately. Update PluginSlot so the effect in PluginSlot resolves
getPlugin(pluginName) right away before calling subscribe, ensuring setPlugin is
triggered for the current pluginName and the old plugin is not left mounted
until a later registerPlugin event.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17473573-47ef-40b4-8eb0-3db3ec7cd14e
📒 Files selected for processing (11)
frontend/src/components/MarkerPopup/mapCustomTypeToReactComponent.jsxfrontend/src/plugins/MapOverlays.jsxfrontend/src/plugins/PluginSlot.jsxfrontend/src/plugins/pluginLoader.jsfrontend/src/plugins/pluginRegistry.jsfrontend/tests/MarkerPopup/LocationDetailsBox.test.jsxfrontend/tests/plugins/MapOverlays.test.jsxfrontend/tests/plugins/PluginSlot.test.jsxgoodmap/goodmap.pytests/unit_tests/test_formatter.pytests/unit_tests/test_goodmap.py
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/src/plugins/MapOverlays.jsx
- frontend/src/plugins/pluginLoader.js
- frontend/tests/plugins/MapOverlays.test.jsx
- tests/unit_tests/test_goodmap.py
- frontend/src/plugins/pluginRegistry.js
- goodmap/goodmap.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
goodmap/plugin.py (2)
26-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
__init__overrides.
MapOverlayPluginBase.__init__andMarkerFieldPluginBase.__init__both simply callsuper().__init__(config)— identical to the inheritedGoodmapPluginBase.__init__. They can be dropped; the subclasses would inherit the same constructor behavior automatically.♻️ Remove redundant constructors
class MapOverlayPluginBase(GoodmapPluginBase): ... capability: ClassVar[str] = "overlay" - - def __init__(self, config: dict[str, Any]) -> None: - super().__init__(config)class MarkerFieldPluginBase(GoodmapPluginBase): ... capability: ClassVar[str] = "field" - - def __init__(self, config: dict[str, Any]) -> None: - super().__init__(config)Also applies to: 44-59
🤖 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 `@goodmap/plugin.py` around lines 26 - 42, The subclasses are overriding constructors without adding any behavior, so remove the redundant __init__ methods from MapOverlayPluginBase and MarkerFieldPluginBase and let them inherit GoodmapPluginBase.__init__ directly. Keep the capability class variables and other class definitions intact, and verify no subclass-specific initialization is lost by checking the GoodmapPluginBase constructor is still the only config initializer used.
20-23: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
capabilitycontract isn't enforced at runtime.
capability: ClassVar[str]is a bare annotation with no default; per typing semantics this is purely a hint for type checkers and has no runtime effect. A third-party plugin subclassingGoodmapPluginBasedirectly (rather than viaMapOverlayPluginBase/MarkerFieldPluginBase) and forgetting to setcapabilitywill only fail with anAttributeErrorwhen the manifest-building code ingoodmap.pylater readsplugin.capability— far from the plugin author's own code, with a confusing error.Consider using
abc.ABC+@property@abstractmethod`` (or a__init_subclass__check) so the missing contract is caught at class-definition/instantiation time.♻️ Example using __init_subclass__ for early validation
class GoodmapPluginBase(PluginBase): - capability: ClassVar[str] + capability: ClassVar[str] + + def __init_subclass__(cls, **kwargs): + super().__init_subclass__(**kwargs) + if "capability" not in cls.__dict__ and not hasattr(cls, "capability"): + raise TypeError(f"{cls.__name__} must define a 'capability' class attribute") def __init__(self, config: dict[str, Any]) -> None: super().__init__(config)🤖 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 `@goodmap/plugin.py` around lines 20 - 23, The `capability` requirement on `GoodmapPluginBase` is only a type annotation, so missing values are not caught until `goodmap.py` later accesses `plugin.capability`. Update `GoodmapPluginBase` to enforce the contract at class-definition or instantiation time, using either `abc.ABC` with an abstract `capability` property or a `__init_subclass__` validation, so direct subclasses cannot forget to define `capability` without an immediate, clearer failure.frontend/tests/MarkerPopup/FieldRenderer.test.jsx (1)
8-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for renderer refresh on
typechange within the same mounted instance.The suite covers initial resolution (builtin, plugin, fallback, primitive, collision) but not re-rendering the same
FieldRendererinstance with a differentvalue.type. Given the stale-state concern raised inFieldRenderer.jsx(Lines 44-58), a regression test usingrerenderwith a changedtypewould catch this class of bug going forward.🤖 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 `@frontend/tests/MarkerPopup/FieldRenderer.test.jsx` around lines 8 - 51, Add a regression test in FieldRenderer.test.jsx for updating the same mounted FieldRenderer instance with a different value.type using rerender; start with one type, then rerender with another and assert the displayed renderer changes accordingly. Use the existing FieldRenderer, render, and rerender patterns so the test catches stale state in the component logic that resolves renderers by type.
🤖 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 `@frontend/src/components/MarkerPopup/builtinFieldRenderers.jsx`:
- Around line 48-64: The CTAButtonField component currently always renders a
clickable button even when sanitizeUrl rejects the value, unlike HyperlinkField
which falls back to plain text. Update CTAButtonField in
builtinFieldRenderers.jsx to validate the URL before rendering and, when unsafe,
render non-interactive text instead of the styled button; keep the existing
button/open-in-new-tab behavior only for sanitized URLs.
In `@frontend/src/components/MarkerPopup/FieldRenderer.jsx`:
- Around line 44-58: The FieldRenderer state can become stale when value.type
changes on an already mounted instance because the useState initializer only
runs once and the current useEffect subscription in FieldRenderer only reacts to
registry events. Update FieldRenderer so it resolves the renderer synchronously
whenever type changes, not just inside the subscribe callback, and keep the
subscription only for future register/unregister updates. Use the existing type,
resolveFieldRenderer, subscribe, and setRenderer logic to ensure the renderer
always matches the latest value.type.
---
Nitpick comments:
In `@frontend/tests/MarkerPopup/FieldRenderer.test.jsx`:
- Around line 8-51: Add a regression test in FieldRenderer.test.jsx for updating
the same mounted FieldRenderer instance with a different value.type using
rerender; start with one type, then rerender with another and assert the
displayed renderer changes accordingly. Use the existing FieldRenderer, render,
and rerender patterns so the test catches stale state in the component logic
that resolves renderers by type.
In `@goodmap/plugin.py`:
- Around line 26-42: The subclasses are overriding constructors without adding
any behavior, so remove the redundant __init__ methods from MapOverlayPluginBase
and MarkerFieldPluginBase and let them inherit GoodmapPluginBase.__init__
directly. Keep the capability class variables and other class definitions
intact, and verify no subclass-specific initialization is lost by checking the
GoodmapPluginBase constructor is still the only config initializer used.
- Around line 20-23: The `capability` requirement on `GoodmapPluginBase` is only
a type annotation, so missing values are not caught until `goodmap.py` later
accesses `plugin.capability`. Update `GoodmapPluginBase` to enforce the contract
at class-definition or instantiation time, using either `abc.ABC` with an
abstract `capability` property or a `__init_subclass__` validation, so direct
subclasses cannot forget to define `capability` without an immediate, clearer
failure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c577cd1c-9433-490e-b306-856246016eba
📒 Files selected for processing (19)
docs/api.rstdocs/plugins.rstfrontend/src/components/Map/components/AccessibilityTable.jsxfrontend/src/components/MarkerPopup/FieldRenderer.jsxfrontend/src/components/MarkerPopup/LocationDetails.jsxfrontend/src/components/MarkerPopup/builtinFieldRenderers.jsxfrontend/src/components/MarkerPopup/fieldContent.jsfrontend/src/components/MarkerPopup/mapCustomTypeToReactComponent.jsxfrontend/src/plugins/MapOverlays.jsxfrontend/src/plugins/PluginSlot.jsxfrontend/src/plugins/pluginRegistry.jsfrontend/tests/MarkerPopup/FieldRenderer.test.jsxfrontend/tests/MarkerPopup/LocationDetailsBox.test.jsxfrontend/tests/plugins/PluginSlot.test.jsxgoodmap/__init__.pygoodmap/goodmap.pygoodmap/plugin.pytests/unit_tests/test_formatter.pytests/unit_tests/test_goodmap.py
💤 Files with no reviewable changes (3)
- frontend/src/components/MarkerPopup/mapCustomTypeToReactComponent.jsx
- frontend/src/plugins/PluginSlot.jsx
- frontend/tests/plugins/PluginSlot.test.jsx
✅ Files skipped from review due to trivial changes (1)
- docs/api.rst
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/plugins/MapOverlays.jsx
- goodmap/goodmap.py
- frontend/src/plugins/pluginRegistry.js
- tests/unit_tests/test_goodmap.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
1 participant
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 by CodeRabbit
pluginNameand fieldtype; inactive/misconfigured plugins are not served.platzkydependency version.