Repository navigation
feat(models): add dashboard models and enums (OMN-1284) - #363
Conversation
Add dashboard-related models and enums to omnibase_core for UI presentation. Enums: - EnumWidgetType: CHART, TABLE, METRIC_CARD, STATUS_GRID, EVENT_FEED - EnumDashboardStatus: INITIALIZING, CONNECTED, DISCONNECTED, ERROR Models: - ModelDashboardConfig: Dashboard layout, refresh rates, widget configs - ModelWidgetDefinition: Widget wrapper with discriminated union config - ModelWidgetConfig*: Type-specific configs (chart, table, metric_card, status_grid, event_feed) - ModelCapabilityView: UI projection of capability data - ModelNodeView: UI projection of node data Design decisions: - config_kind Literal discriminator (not duplicating enum values) - EnumWidgetType as canonical vocabulary - Simple refresh_interval_seconds (no runtime timestamps in config) - Frozen ConfigDict on all definition models - UUID for dashboard_id and widget_id (per ONEX conventions) - Single class per file (helper models in separate files)
📝 WalkthroughWalkthroughAdds a dashboard domain: new enums, ~17 immutable Pydantic dashboard models (configs, widget defs, view models, filters), a hex-color validator with re-exports, many unit tests, and a minor all export form change. No runtime control-flow changes. Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @tests/unit/enums/test_enum_widget_type.py:
- Around line 101-106: The test_enum_membership currently checks string
membership directly against EnumWidgetType which is invalid because
EnumMeta.__contains__ expects enum members; update the assertions in
test_enum_membership to check the enum .value instead (e.g., use any(m.value ==
"chart" for m in EnumWidgetType) or build a list of values via [m.value for m in
EnumWidgetType]) and similarly verify that "invalid_widget" is not present by
asserting not any(...); keep the existing EnumWidgetType.CHART assertion as-is
to test member membership.
In @tests/unit/models/dashboard/test_model_capability_view.py:
- Around line 14-16: Add the pytest unit marker to this test module by applying
a module-level pytestmark or decorating the TestModelCapabilityView class: add
either a top-level assignment pytestmark = pytest.mark.unit or annotate the
TestModelCapabilityView class with @pytest.mark.unit so the tests under
TestModelCapabilityView are marked as unit tests.
In @tests/unit/models/dashboard/test_model_widget_definition.py:
- Around line 22-25: Add the pytest unit marker to this test module or class so
tests are categorizable; either add a module-level statement `pytestmark =
pytest.mark.unit` at top of the file or decorate the TestModelWidgetDefinition
class with `@pytest.mark.unit` (affecting the class containing
test_create_with_chart_config) to follow the tests/**/*.py guidelines.
🧹 Nitpick comments (14)
src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
20-20: Consider adding hex color validation.The description mentions "hex" format, but there's no validation enforcing this. If strict validation is desired, consider adding a
field_validatorto ensure proper hex format (e.g.,#RRGGBB).Example validation using Pydantic field_validator
from pydantic import field_validator import re @field_validator("color") @classmethod def validate_hex_color(cls, v: str | None) -> str | None: if v is not None and not re.match(r"^#[0-9A-Fa-f]{6}$", v): raise ValueError("Color must be in hex format (#RRGGBB)") return vsrc/omnibase_core/models/dashboard/model_metric_threshold.py (1)
17-17: Consider adding hex color validation.The
colorfield is documented as accepting hex colors but lacks format validation. Invalid color strings could be accepted, potentially causing rendering issues in the UI.♻️ Proposed validation using Pydantic's field_validator
from pydantic import BaseModel, ConfigDict, Field +from pydantic import field_validator +import re __all__ = ["ModelMetricThreshold"] class ModelMetricThreshold(BaseModel): """Threshold configuration for metric display.""" model_config = ConfigDict(frozen=True, extra="forbid", from_attributes=True) value: float = Field(..., description="Threshold value") color: str = Field(..., description="Color when threshold is reached (hex)") label: str | None = Field(default=None, description="Threshold label") + + @field_validator("color") + @classmethod + def validate_hex_color(cls, v: str) -> str: + """Validate hex color format.""" + if not re.match(r"^#[0-9A-Fa-f]{6}$", v): + raise ValueError("color must be a valid hex color (e.g., #FF5733)") + return vsrc/omnibase_core/models/dashboard/model_chart_axis_config.py (1)
17-18: Consider validating min_value < max_value relationship.When both
min_valueandmax_valueare provided, there's no validation ensuring thatmin_valueis less thanmax_value. This could lead to invalid axis configurations.♻️ Proposed validation using Pydantic's model_validator
-from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, model_validator __all__ = ["ModelChartAxisConfig"] class ModelChartAxisConfig(BaseModel): """Configuration for a chart axis.""" model_config = ConfigDict(frozen=True, extra="forbid", from_attributes=True) label: str | None = Field(default=None, description="Axis label") min_value: float | None = Field(default=None, description="Minimum axis value") max_value: float | None = Field(default=None, description="Maximum axis value") show_grid: bool = Field(default=True, description="Show grid lines") + + @model_validator(mode="after") + def validate_axis_range(self) -> "ModelChartAxisConfig": + """Validate that min_value is less than max_value when both are provided.""" + if self.min_value is not None and self.max_value is not None: + if self.min_value >= self.max_value: + raise ValueError("min_value must be less than max_value") + return selfsrc/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
36-38: Consider validating non-empty series for data-driven chart types.While an empty series tuple is valid for initial configuration, certain chart types (line, bar, area, scatter) typically require at least one series to render meaningfully. Consider adding a model validator to ensure at least one series is present when
chart_typeis not "pie".📊 Optional validator for series requirement
from pydantic import model_validator class ModelWidgetConfigChart(BaseModel): # ... existing fields ... @model_validator(mode="after") def validate_series_for_chart_type(self) -> "ModelWidgetConfigChart": """Ensure data-driven chart types have at least one series.""" if self.chart_type in {"line", "bar", "area", "scatter"} and not self.series: raise ValueError(f"Chart type '{self.chart_type}' requires at least one series") return selfsrc/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
37-40: Consider validating trend_key when show_trend is enabled.When
show_trend=Truebuttrend_key=None, the widget cannot display a trend indicator. Consider adding a model validator to ensuretrend_keyis provided when trend display is enabled.✅ Validator for trend configuration consistency
from pydantic import model_validator class ModelWidgetConfigMetricCard(BaseModel): # ... existing fields ... @model_validator(mode="after") def validate_trend_configuration(self) -> "ModelWidgetConfigMetricCard": """Ensure trend_key is provided when show_trend is enabled.""" if self.show_trend and self.trend_key is None: raise ValueError("trend_key must be provided when show_trend is True") return selfsrc/omnibase_core/models/dashboard/model_widget_config_status_grid.py (1)
38-46: Note: Mutable dict in frozen model.While the model itself is frozen, the
status_colorsdict field is mutable after instantiation. This is acceptable if runtime modification of colors is intended, but if true immutability is required, consider usingfrozendictorMappingProxyType.Since the PR objectives mention "frozen ConfigDict on definition models" and tests validate immutability, this design appears intentional. However, if complete immutability is desired:
🔒 Optional: Use immutable mapping
from types import MappingProxyType class ModelWidgetConfigStatusGrid(BaseModel): # ... other fields ... status_colors: MappingProxyType[str, str] = Field( default_factory=lambda: MappingProxyType({ "healthy": "#22c55e", "warning": "#eab308", "error": "#ef4444", "unknown": "#6b7280", }), description="Status value to color mapping", )Note: This requires a custom Pydantic validator to handle MappingProxyType serialization.
src/omnibase_core/models/dashboard/model_dashboard_config.py (1)
53-55: Consider using an enum for theme instead of Literal.While
Literal["light", "dark", "system"]works, defining anEnumThemewould provide better type safety, IDE support, and reusability. This aligns with the PR's pattern of using enums likeEnumDashboardStatusandEnumWidgetType.🎨 Optional: Define EnumTheme
Create
src/omnibase_core/enums/enum_theme.py:from enum import Enum __all__ = ["EnumTheme"] class EnumTheme(str, Enum): """Dashboard theme values.""" LIGHT = "light" DARK = "dark" SYSTEM = "system"Then update this model:
from omnibase_core.enums import EnumTheme class ModelDashboardConfig(BaseModel): # ... theme: EnumTheme = Field( default=EnumTheme.SYSTEM, description="Dashboard theme preference" )tests/unit/models/dashboard/test_model_widget_definition.py (2)
98-168: Prefer not to rely onisinstance(...)for union discrimination assertions.Given the “no
isinstancechecks” guideline, consider asserting onconfig_kindplus one type-specific field, or usingtype(widget.config) is ModelWidgetConfigChart(etc.) for strictness. As per coding guidelines.
169-178: Make the frozen-model mutation assertion resilient to Pydantic’s raised exception type.Depending on Pydantic version/config, assignment to a frozen model can raise
ValidationErrororTypeError. Consider accepting both to avoid version-coupled flakes.Proposed change
- with pytest.raises(ValidationError): + with pytest.raises((TypeError, ValidationError)): widget.title = "New Title" # type: ignore[misc]src/omnibase_core/models/dashboard/model_widget_config_event_feed.py (1)
16-40: Avoid naming the fieldfilter(built-in shadowing); considerevent_filterwith alias.
filtershadows the Python built-in, which tends to create incidental confusion. If the JSON contract needs to remain"filter", you can rename the attribute (e.g.,event_filter) and set an alias for wire format.Proposed change
class ModelWidgetConfigEventFeed(BaseModel): @@ - filter: ModelEventFilter | None = Field( - default=None, description="Event filtering configuration" - ) + event_filter: ModelEventFilter | None = Field( + default=None, + alias="filter", + description="Event filtering configuration", + )tests/unit/models/dashboard/test_model_capability_view.py (1)
84-92: Make the frozen-model mutation assertion resilient to Pydantic’s raised exception type.Depending on Pydantic version/config, assigning to a frozen model can raise
ValidationErrororTypeError.Proposed change
- with pytest.raises(ValidationError): + with pytest.raises((TypeError, ValidationError)): view.name = "new-name" # type: ignore[misc]src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
35-42: Consider makingdefault_sort_directionconditional/optional whendefault_sort_keyis unset.Right now direction is always present even when
default_sort_keyisNone. Optional direction (or a short doc note) can reduce ambiguity for consumers.tests/unit/enums/test_enum_widget_type.py (1)
10-10: Prefer testing the public import surface + reduce repetition with parametrization.
Given the PR summary saysEnumWidgetTypeis exported viaomnibase_core.enums.__init__, importing fromomnibase_core.enumsbetter validates the intended public API. Also, several tests repeat the same mapping checks and could be collapsed via@pytest.mark.parametrizefor maintainability.Also applies to: 17-23, 67-73, 75-82
src/omnibase_core/models/dashboard/__init__.py (1)
6-43: LGTM: clean re-export surface; consider making__all__immutable.
This is a straightforward “barrel” module and the exports look consistent with the imports. Minor: consider using a tuple for__all__to avoid accidental runtime mutation.Optional tweak
-__all__ = [ +__all__ = ( # Dashboard Configuration "ModelDashboardConfig", "ModelDashboardLayoutConfig", # Widget Definition "ModelWidgetConfig", "ModelWidgetDefinition", # View Models "ModelCapabilityView", "ModelNodeView", # Chart Widget "ModelChartAxisConfig", "ModelChartSeriesConfig", "ModelWidgetConfigChart", # Table Widget "ModelTableColumnConfig", "ModelWidgetConfigTable", # Metric Card Widget "ModelMetricThreshold", "ModelWidgetConfigMetricCard", # Status Grid Widget "ModelStatusItemConfig", "ModelWidgetConfigStatusGrid", # Event Feed Widget "ModelEventFilter", "ModelWidgetConfigEventFeed", -] +)Also applies to: 44-70
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/omnibase_core/enums/__init__.pysrc/omnibase_core/enums/enum_dashboard_status.pysrc/omnibase_core/enums/enum_widget_type.pysrc/omnibase_core/models/dashboard/__init__.pysrc/omnibase_core/models/dashboard/model_capability_view.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_dashboard_layout_config.pysrc/omnibase_core/models/dashboard/model_event_filter.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_node_view.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/models/dashboard/model_widget_config_event_feed.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/models/dashboard/model_widget_definition.pytests/unit/enums/test_enum_dashboard_status.pytests/unit/enums/test_enum_widget_type.pytests/unit/models/dashboard/__init__.pytests/unit/models/dashboard/test_model_capability_view.pytests/unit/models/dashboard/test_model_dashboard_config.pytests/unit/models/dashboard/test_model_node_view.pytests/unit/models/dashboard/test_model_widget_definition.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Use 'poetry run' for all Python commands (pytest, mypy, python -m, etc.). Never use pip or direct python execution.
Always use protocol names for service resolution (e.g., 'ProtocolEventBus', 'ProtocolLogger'), never concrete class names. Use duck typing without isinstance checks.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception. Include error context in the context parameter.
Use the @standard_error_handling decorator on async functions to eliminate 6+ lines of try/catch boilerplate in error handling.
Use PEP 604 union syntax (e.g., 'str | None') instead of legacy Optional/Union. Enforce via ruff rule UP007.
Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers.
Order exception tuples alphabetically by exception class name (e.g., except (AttributeError, TypeError, ValidationError) not except (ValueError, TypeError, AttributeError)).
Use centralized exception constants from omnibase_core.errors.exception_groups (PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS, FILE_IO_ERRORS, etc.) instead of manually listing exceptions.
Maintain 100% mypy strict mode compliance (0 errors) across all source files using type annotations. Both mypy and pyright must pass in CI.
Format Python code with Black: poetry run black src/ tests/. Sort imports with isort: poetry run isort src/ tests/. Lint with ruff: poetry run ruff check src/ tests/
Do NOT use 'from future import annotations' when runtime type introspection is needed (Pydantic models, FastAPI endpoints). Use it when forward references are needed or to prevent circular imports.
Use EnumNodeKind for architectural role (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) for pipeline routing. Use EnumNodeType for implementation type (COMPUTE_GENERIC, TRA...
Files:
src/omnibase_core/models/dashboard/model_widget_config_chart.pytests/unit/models/dashboard/__init__.pysrc/omnibase_core/enums/enum_widget_type.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_capability_view.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_widget_config_event_feed.pysrc/omnibase_core/models/dashboard/model_event_filter.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pytests/unit/enums/test_enum_dashboard_status.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pytests/unit/models/dashboard/test_model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/enums/__init__.pytests/unit/models/dashboard/test_model_widget_definition.pysrc/omnibase_core/models/dashboard/__init__.pytests/unit/enums/test_enum_widget_type.pysrc/omnibase_core/enums/enum_dashboard_status.pytests/unit/models/dashboard/test_model_capability_view.pytests/unit/models/dashboard/test_model_node_view.pysrc/omnibase_core/models/dashboard/model_dashboard_layout_config.pysrc/omnibase_core/models/dashboard/model_node_view.pysrc/omnibase_core/models/dashboard/model_widget_definition.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Follow directory-specific naming prefixes for Python files in src/omnibase_core/. Examples: cli_, constants_, container_, decorator_, enum_, error_, factory_, mixin_, model_, node_, etc.
Use FileRegistry for loading YAML contracts: registry = FileRegistry(); contract = registry.load(Path('config/runtime_host.yaml'))
Use ModelEventEnvelope for inter-service event communication. Structure events with proper envelope wrapping for consistency.
Files:
src/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/enums/enum_widget_type.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_capability_view.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_widget_config_event_feed.pysrc/omnibase_core/models/dashboard/model_event_filter.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/enums/__init__.pysrc/omnibase_core/models/dashboard/__init__.pysrc/omnibase_core/enums/enum_dashboard_status.pysrc/omnibase_core/models/dashboard/model_dashboard_layout_config.pysrc/omnibase_core/models/dashboard/model_node_view.pysrc/omnibase_core/models/dashboard/model_widget_definition.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Add 'from_attributes=True' to ConfigDict for immutable value objects nested in other Pydantic models used with pytest-xdist parallel execution.
Use Pydantic 2.11+ for all data models. Use Pydantic v2 syntax, ConfigDict, and model_validate methods.
Files:
src/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_capability_view.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_widget_config_event_feed.pysrc/omnibase_core/models/dashboard/model_event_filter.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/models/dashboard/__init__.pysrc/omnibase_core/models/dashboard/model_dashboard_layout_config.pysrc/omnibase_core/models/dashboard/model_node_view.pysrc/omnibase_core/models/dashboard/model_widget_definition.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% code coverage across test suite. Run tests with: poetry run pytest tests/ --cov=src/omnibase_core/
Run all tests with parallel workers: poetry run pytest tests/ uses 4 workers locally with 60s timeout and loadscope distribution. CI uses 20 parallel splits on GitHub Actions.
Use pytest markers for test categorization: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
Use pytest 8.4+ for testing. Use pytest fixtures, markers, and parallel execution with pytest-xdist.
Files:
tests/unit/models/dashboard/__init__.pytests/unit/enums/test_enum_dashboard_status.pytests/unit/models/dashboard/test_model_dashboard_config.pytests/unit/models/dashboard/test_model_widget_definition.pytests/unit/enums/test_enum_widget_type.pytests/unit/models/dashboard/test_model_capability_view.pytests/unit/models/dashboard/test_model_node_view.py
🧠 Learnings (4)
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ for all data models. Use Pydantic v2 syntax, ConfigDict, and model_validate methods.
Applied to files:
src/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/models/dashboard/model_dashboard_layout_config.py
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add 'from_attributes=True' to ConfigDict for immutable value objects nested in other Pydantic models used with pytest-xdist parallel execution.
Applied to files:
src/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pytests/unit/models/dashboard/test_model_dashboard_config.py
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelEventEnvelope for inter-service event communication. Structure events with proper envelope wrapping for consistency.
Applied to files:
src/omnibase_core/models/dashboard/model_widget_config_event_feed.pysrc/omnibase_core/models/dashboard/model_event_filter.py
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer for dependency injection in node constructors, not ModelContainer. ModelONEXContainer provides service resolution via get_service(), while ModelContainer is only a value wrapper.
Applied to files:
src/omnibase_core/models/dashboard/model_node_view.py
🧬 Code graph analysis (15)
src/omnibase_core/models/dashboard/model_widget_config_chart.py (3)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)src/omnibase_core/models/dashboard/model_chart_axis_config.py (1)
ModelChartAxisConfig(11-19)src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
ModelChartSeriesConfig(13-23)
src/omnibase_core/models/dashboard/model_capability_view.py (1)
src/omnibase_core/models/metadata/model_metadata_field_info.py (2)
uuid(158-177)namespace(272-287)
src/omnibase_core/models/dashboard/model_widget_config_event_feed.py (2)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)src/omnibase_core/models/dashboard/model_event_filter.py (1)
ModelEventFilter(11-24)
tests/unit/enums/test_enum_dashboard_status.py (1)
src/omnibase_core/enums/enum_dashboard_status.py (4)
EnumDashboardStatus(11-38)is_operational(23-25)is_terminal(28-30)requires_reconnection(33-38)
src/omnibase_core/models/dashboard/model_dashboard_config.py (4)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(158-177)src/omnibase_core/enums/enum_dashboard_status.py (1)
EnumDashboardStatus(11-38)src/omnibase_core/models/dashboard/model_dashboard_layout_config.py (1)
ModelDashboardLayoutConfig(11-19)src/omnibase_core/models/dashboard/model_widget_definition.py (1)
ModelWidgetDefinition(41-65)
src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (2)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)src/omnibase_core/models/dashboard/model_status_item_config.py (1)
ModelStatusItemConfig(11-18)
src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (2)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)src/omnibase_core/models/dashboard/model_metric_threshold.py (1)
ModelMetricThreshold(11-18)
tests/unit/models/dashboard/test_model_dashboard_config.py (5)
src/omnibase_core/enums/enum_dashboard_status.py (1)
EnumDashboardStatus(11-38)src/omnibase_core/models/dashboard/model_dashboard_config.py (1)
ModelDashboardConfig(22-61)src/omnibase_core/models/dashboard/model_dashboard_layout_config.py (1)
ModelDashboardLayoutConfig(11-19)src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
ModelWidgetConfigChart(19-46)src/omnibase_core/models/dashboard/model_widget_definition.py (1)
ModelWidgetDefinition(41-65)
src/omnibase_core/models/dashboard/model_widget_config_table.py (2)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)src/omnibase_core/models/dashboard/model_table_column_config.py (1)
ModelTableColumnConfig(13-27)
src/omnibase_core/enums/__init__.py (2)
src/omnibase_core/enums/enum_dashboard_status.py (1)
EnumDashboardStatus(11-38)src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)
tests/unit/models/dashboard/test_model_widget_definition.py (6)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
ModelWidgetConfigChart(19-46)src/omnibase_core/models/dashboard/model_widget_config_event_feed.py (1)
ModelWidgetConfigEventFeed(16-40)src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
ModelWidgetConfigMetricCard(16-44)src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
ModelWidgetConfigTable(18-44)src/omnibase_core/models/dashboard/model_widget_definition.py (1)
ModelWidgetDefinition(41-65)
src/omnibase_core/models/dashboard/__init__.py (15)
src/omnibase_core/models/dashboard/model_capability_view.py (1)
ModelCapabilityView(13-30)src/omnibase_core/models/dashboard/model_chart_axis_config.py (1)
ModelChartAxisConfig(11-19)src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
ModelChartSeriesConfig(13-23)src/omnibase_core/models/dashboard/model_dashboard_config.py (1)
ModelDashboardConfig(22-61)src/omnibase_core/models/dashboard/model_dashboard_layout_config.py (1)
ModelDashboardLayoutConfig(11-19)src/omnibase_core/models/dashboard/model_event_filter.py (1)
ModelEventFilter(11-24)src/omnibase_core/models/dashboard/model_metric_threshold.py (1)
ModelMetricThreshold(11-18)src/omnibase_core/models/dashboard/model_status_item_config.py (1)
ModelStatusItemConfig(11-18)src/omnibase_core/models/dashboard/model_table_column_config.py (1)
ModelTableColumnConfig(13-27)src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
ModelWidgetConfigChart(19-46)src/omnibase_core/models/dashboard/model_widget_config_event_feed.py (1)
ModelWidgetConfigEventFeed(16-40)src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
ModelWidgetConfigMetricCard(16-44)src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (1)
ModelWidgetConfigStatusGrid(18-46)src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
ModelWidgetConfigTable(18-44)src/omnibase_core/models/dashboard/model_widget_definition.py (1)
ModelWidgetDefinition(41-65)
tests/unit/enums/test_enum_widget_type.py (1)
src/omnibase_core/enums/enum_widget_type.py (3)
EnumWidgetType(11-39)is_data_bound(25-31)is_aggregation(34-39)
tests/unit/models/dashboard/test_model_capability_view.py (1)
src/omnibase_core/models/dashboard/model_capability_view.py (1)
ModelCapabilityView(13-30)
src/omnibase_core/models/dashboard/model_widget_definition.py (6)
src/omnibase_core/models/metadata/model_metadata_field_info.py (1)
uuid(158-177)src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
ModelWidgetConfigChart(19-46)src/omnibase_core/models/dashboard/model_widget_config_event_feed.py (1)
ModelWidgetConfigEventFeed(16-40)src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
ModelWidgetConfigMetricCard(16-44)src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (1)
ModelWidgetConfigStatusGrid(18-46)src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
ModelWidgetConfigTable(18-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (24)
tests/unit/models/dashboard/__init__.py (1)
1-3: LGTM!Standard license headers for the test package initializer.
src/omnibase_core/models/dashboard/model_event_filter.py (1)
1-24: LGTM!The model follows all coding guidelines:
- Pydantic v2 syntax with
ConfigDict- Immutable with
frozen=Trueandfrom_attributes=Trueper guidelines- Uses tuples for immutability
- Clear field descriptions with sensible defaults (empty = all)
src/omnibase_core/models/dashboard/model_table_column_config.py (1)
1-27: LGTM!The model adheres to all coding guidelines:
- Pydantic v2 syntax with
ConfigDict- PEP 604 union syntax (
int | None,str | None)- Immutable with
frozen=Trueandfrom_attributes=True- Proper use of
Literalfor constrained values- Clear field descriptions with appropriate defaults
src/omnibase_core/enums/enum_widget_type.py (2)
24-31: LGTM! The property logic is well-designed.The
is_data_boundproperty correctly identifies widgets that require external data sources (CHART, TABLE, EVENT_FEED).
33-39: LGTM! The property logic is correct.The
is_aggregationproperty correctly identifies widgets that display aggregated metrics.src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
1-23: LGTM! The model follows all guidelines.The implementation adheres to coding standards:
- Pydantic v2 syntax with
ConfigDict- PEP 604 union syntax (
str | None)- Immutable with
frozen=Trueandfrom_attributes=True- Proper use of
Literalforseries_typesrc/omnibase_core/models/dashboard/model_capability_view.py (1)
13-30: LGTM! Well-structured view model.The model correctly follows all Pydantic v2 conventions with frozen configuration, strict extra field handling, and attribute-based initialization. Field definitions are comprehensive and appropriately typed for UI projection use cases.
src/omnibase_core/models/dashboard/model_status_item_config.py (1)
11-18: LGTM! Clean configuration model.The model is well-structured with appropriate field types and follows all Pydantic v2 conventions. The frozen, strict configuration is suitable for this immutable value object.
src/omnibase_core/enums/__init__.py (1)
66-67: LGTM! New enum imports properly integrated.Both
EnumDashboardStatusandEnumWidgetTypeare correctly imported with appropriate tracking comments (OMN-1284) and exported in__all__. The widget type domain is properly categorized.Also applies to: 301-302, 518-519
src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
19-46: LGTM! Well-structured immutable widget config.The model follows Pydantic v2 best practices with frozen configuration, discriminated union support via
config_kind, and comprehensive field descriptions. The use of immutable tuples forseriesaligns with the frozen model constraint.src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
16-44: LGTM! Comprehensive metric card configuration.The model is well-structured with appropriate field constraints (precision 0-10, format literals) and follows Pydantic v2 best practices with frozen, immutable configuration.
src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (1)
18-46: LGTM! Well-designed status grid configuration.The model follows Pydantic v2 best practices with appropriate field constraints (columns 1-12) and immutable tuple for items. The use of
default_factoryfor the status colors mapping is correct.src/omnibase_core/models/dashboard/model_dashboard_config.py (3)
22-61: LGTM! Comprehensive dashboard configuration model.The model follows Pydantic v2 best practices with frozen configuration, appropriate use of
default_factoryfor nested models, and comprehensive field validations. The design aligns well with the PR objectives.
31-31: Field naming is correct and intentional.The field is properly named
nameinModelDashboardConfig. Tests consistently use this field name, and the description "Dashboard display name" clarifies its purpose. Related models likeModelNodeViewandModelCapabilityViewuse a different pattern with separatedisplay_namefields, butModelDashboardConfigcorrectly usesnameas the single display field. No changes are needed.
46-50: Field name is consistent across code and tests.The field name
refresh_interval_secondsis used consistently in both the model definition and test files. No usage of the abbreviated form "refresh_interval_sec" exists in the codebase. The variance mentioned is only in the PR summary description, not in the actual implementation or test coverage.src/omnibase_core/enums/enum_dashboard_status.py (1)
11-38: LGTM! Well-designed status enum with semantic properties.The enum provides a clear state machine for dashboard lifecycle management. The four states (INITIALIZING, CONNECTED, DISCONNECTED, ERROR) cover the expected operational scenarios, and the helper properties (
is_operational,is_terminal,requires_reconnection) enable clean conditional logic without magic string comparisons.The design correctly distinguishes between:
- Terminal errors (ERROR only) that require intervention
- Recoverable states (DISCONNECTED) that need reconnection but aren't terminal
- Operational state (CONNECTED) for normal operation
src/omnibase_core/models/dashboard/model_dashboard_layout_config.py (1)
11-19: No action needed—grid constraint design is intentional.
ModelDashboardLayoutConfig.columns(max 24) andModelWidgetDefinition.width(max 12) serve different purposes by design. Widget width specifies relative span (1–12 units) independent of layout size, allowing flexible UI rendering across variable grid configurations (12-column, 24-column, etc.). This decoupling is validated and tested as-is.src/omnibase_core/models/dashboard/model_node_view.py (1)
11-114: LGTM! Well-designed UI projection model.The implementation demonstrates several best practices:
- Proper use of
ConfigDict(frozen=True, extra="forbid", from_attributes=True)aligned with coding guidelines for Pydantic models used with pytest-xdist- String-based
node_idwith clear justification comment for UI flexibility- Comprehensive field validation with
min_length=1for required fields- Clean utility methods (
get_display_name,get_qualified_id) with fallback logic- Excellent documentation with cross-references to canonical models
The model appropriately separates UI concerns from domain models while maintaining consistency with
ModelNodeIdentity.tests/unit/enums/test_enum_dashboard_status.py (1)
13-141: LGTM! Comprehensive enum test coverage.The test suite demonstrates excellent coverage:
- All enum members and values validated
- Property methods (
is_operational,is_terminal,requires_reconnection) thoroughly tested- Cross-validation of property relationships (lines 54-64)
- String conversion and construction from strings
- JSON serialization roundtrip
- Edge cases (invalid values, empty strings)
The tests provide strong confidence in the enum's behavior and integration.
tests/unit/models/dashboard/test_model_node_view.py (1)
12-172: LGTM! Thorough model test coverage.The test suite provides comprehensive validation:
- Creation scenarios (minimal, full, with various optional fields)
- Default value verification
- Immutability enforcement via frozen model (lines 84-91)
- Field validation (empty strings, required fields, extra fields forbidden)
- Utility method behavior with edge cases (lines 109-141)
- Serialization roundtrips (dict and JSON)
The tests ensure the model behaves correctly across all usage scenarios.
src/omnibase_core/models/dashboard/model_widget_definition.py (2)
30-38: LGTM! Properly implemented discriminated union.The discriminated union pattern is correctly implemented using Pydantic v2's
Field(discriminator="config_kind")syntax. This allows type-safe deserialization and proper mypy support for the union of widget config types.The use of
config_kindas the discriminator (rather than duplicating enum values) aligns with the design decisions mentioned in the PR objectives.
41-65: LGTM! Well-structured widget definition model.The implementation demonstrates good practices:
- UUID for
widget_idper ONEX conventions- Proper grid layout validation (width 1-12, height ≥1, row/col ≥0)
- Frozen model with
from_attributes=Truefor pytest-xdist compatibility- Clear separation between layout positioning and widget-specific config
extra_configappropriately restricted to string values only (documented in field description)tests/unit/models/dashboard/test_model_dashboard_config.py (2)
20-46: LGTM! Complete layout configuration test coverage.The tests validate all aspects of
ModelDashboardLayoutConfig:
- Default values align with grid system conventions (12 columns, 100px rows, 16px gap)
- Custom value assignment
- Serialization roundtrip
49-192: LGTM! Comprehensive dashboard configuration test coverage.The test suite demonstrates excellent coverage:
- Minimal and full creation scenarios
- Default value verification including nested
layoutobject- Widget handling with discriminated union config (lines 75-90, 127)
- Refresh interval semantics including
Noneas disabled (lines 100-108)- Immutability via frozen model
- Field validation (invalid UUID, invalid refresh interval constraints)
- Theme literals and enum status values across all members
- UUID parsing from strings for both dashboard and widget IDs
The tests provide strong confidence in the model's behavior and integration with the discriminated union pattern.
Critical: - Fix enum membership test to check .value membership correctly Major: - Add @pytest.mark.unit markers to dashboard test classes Minor: - Recategorize EnumDashboardStatus to Dashboard domain - Add comprehensive docstrings to dashboard config models Nitpicks: - Add hex color validation to ModelMetricThreshold - Add validation: data charts require series, trend_key required when show_trend - Change extra_config from dict to Mapping for immutability - Rename filter field to event_filter with backwards-compatible alias - Make default_sort_direction conditional on default_sort_key - Create EnumDashboardTheme enum replacing Literal type - Replace isinstance() checks with config_kind discriminator in tests - Make frozen model mutation tests resilient to exception types - Add parametrized import surface tests - Make __all__ immutable (tuple)
Pull Request Review - PR #363: Dashboard Models and EnumsOverall Assessment✅ Excellent work! This PR demonstrates high-quality implementation aligned with ONEX standards. The code is well-structured, thoroughly tested, and follows established conventions. Strengths1. Architecture & Design 🏗️
2. ONEX Compliance ✅
3. Validation & Error Handling 🛡️
4. Test Coverage 🧪
5. Documentation 📚
Minor Observations & Suggestions1. Enum Helper Properties ConsistencyThe enum helper properties (e.g., @property
def is_operational(self) -> bool:
"""Check if dashboard is in an operational state.
Returns:
True if dashboard is connected and ready for use.
Example:
>>> status = EnumDashboardStatus.CONNECTED
>>> if status.is_operational:
... # Process dashboard data
"""
return self == EnumDashboardStatus.CONNECTED2. ModelNodeView String ID JustificationGood use of 3. Event Feed Filter Alias PatternIn event_filter: ModelEventFilter | None = Field(
default=None,
alias="filter",
serialization_alias="filter",
description="Event filtering configuration",
)This works well for JSON API compatibility while keeping Python names explicit. Consider adding a comment explaining this choice. 4. Layout Grid ConstraintsThe widget width constraint (model_widget_definition.py:58): width: int = Field(default=1, ge=1, le=12, description="Grid column span")This hard-codes 12-column max, but
5. Threshold Ordering
thresholds: tuple[ModelMetricThreshold, ...] = Field(
default=(), description="Color thresholds for the metric"
)Consider whether thresholds should be sorted by value for correct threshold evaluation. You might add a validator to ensure sorted order or document the expected ordering. 6. Chart Series Data Key ValidationThe Performance & Security✅ No Issues Found
Test Coverage AnalysisExcellent Coverage:
Potential Additions (optional):
Alignment with CLAUDE.md
Recommendations🟢 Approved for Merge (with optional enhancements)The code is production-ready. The suggestions above are optional improvements that could be addressed in follow-up PRs if needed:
SummaryThis PR demonstrates excellent engineering practices:
Great work, @jonahgabriel! 🎉 Reviewed by: Claude Sonnet 4.5 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/omnibase_core/enums/enum_dashboard_theme.py:
- Line 8: Replace the mutable list assigned to __all__ with an immutable tuple
to follow project standard; locate the __all__ variable near EnumDashboardTheme
in enum_dashboard_theme.py and change __all__ = ["EnumDashboardTheme"] to use
parentheses so it becomes a tuple containing "EnumDashboardTheme".
In @src/omnibase_core/models/dashboard/model_metric_threshold.py:
- Line 10: The module currently exposes __all__ as a mutable list containing
"ModelMetricThreshold"; change __all__ to be an immutable tuple (e.g.,
("ModelMetricThreshold",)) so the exported names are immutable — update the
__all__ definition that references ModelMetricThreshold accordingly.
In @src/omnibase_core/models/dashboard/model_widget_definition.py:
- Line 28: The __all__ variable currently defined as a list should be changed to
an immutable tuple; update the module-level __all__ that lists
"ModelWidgetDefinition" and "ModelWidgetConfig" to use a tuple instead of a list
(replace the list literal with a tuple literal) so __all__ becomes immutable and
consistent with other modules.
🧹 Nitpick comments (8)
src/omnibase_core/enums/__init__.py (1)
518-521: LGTM! Dashboard domain exports correctly added to__all__.The three new dashboard enums are properly exported with appropriate domain grouping and PR reference.
Note: The PR objectives mention making
__all__immutable (tuple), but since this file has a pre-existing pattern of using a list for__all__, converting the entire list might be outside the scope of this PR. Consider addressing this in a follow-up refactoring.src/omnibase_core/models/dashboard/model_chart_axis_config.py (2)
8-8: Consider using tuple for__all__to enforce immutability.The PR objectives mention making
__all__immutable (tuple). While a list works, a tuple aligns better with the immutability theme of frozen models.Suggested change
-__all__ = ["ModelChartAxisConfig"] +__all__ = ("ModelChartAxisConfig",)
22-24: Consider validating min/max value relationship.When both
min_valueandmax_valueare provided, there's no validation ensuringmin_value < max_value. This could lead to invalid axis configurations.Optional validation
+from pydantic import BaseModel, ConfigDict, Field, model_validator + class ModelChartAxisConfig(BaseModel): # ... existing fields ... + + @model_validator(mode="after") + def validate_min_max(self) -> "ModelChartAxisConfig": + """Validate min_value is less than max_value when both are set.""" + if self.min_value is not None and self.max_value is not None: + if self.min_value >= self.max_value: + raise ValueError("min_value must be less than max_value") + return selfsrc/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
46-51: Consider usingSelffor return type annotation.For consistency with
ModelWidgetConfigTable(which usesSelf), and to follow modern Python typing practices, consider usingSelfinstead of the string annotation.Suggested change
-from typing import Literal +from typing import Literal, Self # ... in the validator ... @model_validator(mode="after") - def validate_trend_key_when_show_trend(self) -> "ModelWidgetConfigMetricCard": + def validate_trend_key_when_show_trend(self) -> Self: """Validate that trend_key is set when show_trend is enabled."""src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
48-56: Consider extractingdata_chart_typesas a class constant and usingSelf.The inline set works but extracting it as a class constant improves readability and maintainability. Also, for consistency with
ModelWidgetConfigTable, consider usingSelffor the return type.Suggested refactor
-from typing import Literal +from typing import Literal, Self class ModelWidgetConfigChart(BaseModel): + """Chart widget configuration.""" + + DATA_CHART_TYPES: ClassVar[frozenset[str]] = frozenset({"line", "bar", "area", "scatter"}) + # ... existing fields ... @model_validator(mode="after") - def validate_series_for_data_charts(self) -> "ModelWidgetConfigChart": + def validate_series_for_data_charts(self) -> Self: """Validate that data-driven chart types have at least one series.""" - data_chart_types = {"line", "bar", "area", "scatter"} - if self.chart_type in data_chart_types and not self.series: + if self.chart_type in self.DATA_CHART_TYPES and not self.series: raise ValueError( f"Chart type '{self.chart_type}' requires at least one series configuration" ) return selfNote: Add
from typing import ClassVarto imports.src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
27-27: Missing hex color validation for consistency.The
colorfield description indicates it should be a hex value, but unlikeModelMetricThreshold(which validates hex colors usingHEX_COLOR_PATTERN), this field lacks validation. Invalid color strings could be accepted.Add color validation for consistency
-from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, field_validator + +import re + +HEX_COLOR_PATTERN = re.compile(r"^#([0-9a-fA-F]{3}|[0-9a-fA-F]{6}|[0-9a-fA-F]{4}|[0-9a-fA-F]{8})$") class ModelChartSeriesConfig(BaseModel): # ... existing code ... + + @field_validator("color") + @classmethod + def validate_hex_color(cls, v: str | None) -> str | None: + """Validate that color is a valid hex color code.""" + if v is not None and not HEX_COLOR_PATTERN.match(v): + raise ValueError( + f"Invalid hex color format: {v}. " + "Expected #RGB, #RRGGBB, #RGBA, or #RRGGBBAA" + ) + return vAlternatively, consider extracting the pattern to a shared constants module to avoid duplication with
ModelMetricThreshold.src/omnibase_core/models/dashboard/model_table_column_config.py (1)
27-27: Consider adding a minimum constraint forwidth.A column width in pixels should be a positive value. Currently, negative values would be accepted.
Suggested constraint
- width: int | None = Field(default=None, description="Column width in pixels") + width: int | None = Field(default=None, ge=1, description="Column width in pixels")tests/unit/models/dashboard/test_model_widget_definition.py (1)
367-398: Consider parametrizing the chart series requirement tests.The four tests for chart types requiring series (
line,bar,area,scatter) follow the same pattern and could be combined into a parametrized test for reduced duplication.Parametrized version
@pytest.mark.parametrize("chart_type", ["line", "bar", "area", "scatter"]) def test_chart_validation_data_charts_require_series(self, chart_type: str) -> None: """Test that data-driven chart types require at least one series.""" with pytest.raises(ValidationError, match="requires at least one series"): ModelWidgetConfigChart(chart_type=chart_type, series=())
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/omnibase_core/enums/__init__.pysrc/omnibase_core/enums/enum_dashboard_theme.pysrc/omnibase_core/models/dashboard/__init__.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/models/dashboard/model_widget_config_event_feed.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/models/dashboard/model_widget_definition.pytests/unit/enums/test_enum_widget_type.pytests/unit/models/dashboard/__init__.pytests/unit/models/dashboard/test_dashboard_imports.pytests/unit/models/dashboard/test_model_capability_view.pytests/unit/models/dashboard/test_model_dashboard_config.pytests/unit/models/dashboard/test_model_metric_threshold.pytests/unit/models/dashboard/test_model_widget_definition.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/unit/models/dashboard/test_model_dashboard_config.py
- src/omnibase_core/models/dashboard/model_dashboard_config.py
- tests/unit/models/dashboard/test_model_capability_view.py
- src/omnibase_core/models/dashboard/model_status_item_config.py
- tests/unit/models/dashboard/init.py
- src/omnibase_core/models/dashboard/model_widget_config_event_feed.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Use 'poetry run' for all Python commands (pytest, mypy, python -m, etc.). Never use pip or direct python execution.
Always use protocol names for service resolution (e.g., 'ProtocolEventBus', 'ProtocolLogger'), never concrete class names. Use duck typing without isinstance checks.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception. Include error context in the context parameter.
Use the @standard_error_handling decorator on async functions to eliminate 6+ lines of try/catch boilerplate in error handling.
Use PEP 604 union syntax (e.g., 'str | None') instead of legacy Optional/Union. Enforce via ruff rule UP007.
Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers.
Order exception tuples alphabetically by exception class name (e.g., except (AttributeError, TypeError, ValidationError) not except (ValueError, TypeError, AttributeError)).
Use centralized exception constants from omnibase_core.errors.exception_groups (PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS, FILE_IO_ERRORS, etc.) instead of manually listing exceptions.
Maintain 100% mypy strict mode compliance (0 errors) across all source files using type annotations. Both mypy and pyright must pass in CI.
Format Python code with Black: poetry run black src/ tests/. Sort imports with isort: poetry run isort src/ tests/. Lint with ruff: poetry run ruff check src/ tests/
Do NOT use 'from future import annotations' when runtime type introspection is needed (Pydantic models, FastAPI endpoints). Use it when forward references are needed or to prevent circular imports.
Use EnumNodeKind for architectural role (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) for pipeline routing. Use EnumNodeType for implementation type (COMPUTE_GENERIC, TRA...
Files:
src/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pysrc/omnibase_core/enums/enum_dashboard_theme.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_widget_definition.pytests/unit/enums/test_enum_widget_type.pysrc/omnibase_core/enums/__init__.pytests/unit/models/dashboard/test_dashboard_imports.pysrc/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pytests/unit/models/dashboard/test_model_widget_definition.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pytests/unit/models/dashboard/test_model_metric_threshold.pysrc/omnibase_core/models/dashboard/__init__.pysrc/omnibase_core/models/dashboard/model_widget_config_table.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Follow directory-specific naming prefixes for Python files in src/omnibase_core/. Examples: cli_, constants_, container_, decorator_, enum_, error_, factory_, mixin_, model_, node_, etc.
Use FileRegistry for loading YAML contracts: registry = FileRegistry(); contract = registry.load(Path('config/runtime_host.yaml'))
Use ModelEventEnvelope for inter-service event communication. Structure events with proper envelope wrapping for consistency.
Files:
src/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pysrc/omnibase_core/enums/enum_dashboard_theme.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_widget_definition.pysrc/omnibase_core/enums/__init__.pysrc/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/__init__.pysrc/omnibase_core/models/dashboard/model_widget_config_table.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Add 'from_attributes=True' to ConfigDict for immutable value objects nested in other Pydantic models used with pytest-xdist parallel execution.
Use Pydantic 2.11+ for all data models. Use Pydantic v2 syntax, ConfigDict, and model_validate methods.
Files:
src/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_widget_definition.pysrc/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/__init__.pysrc/omnibase_core/models/dashboard/model_widget_config_table.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% code coverage across test suite. Run tests with: poetry run pytest tests/ --cov=src/omnibase_core/
Run all tests with parallel workers: poetry run pytest tests/ uses 4 workers locally with 60s timeout and loadscope distribution. CI uses 20 parallel splits on GitHub Actions.
Use pytest markers for test categorization: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
Use pytest 8.4+ for testing. Use pytest fixtures, markers, and parallel execution with pytest-xdist.
Files:
tests/unit/enums/test_enum_widget_type.pytests/unit/models/dashboard/test_dashboard_imports.pytests/unit/models/dashboard/test_model_widget_definition.pytests/unit/models/dashboard/test_model_metric_threshold.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ for all data models. Use Pydantic v2 syntax, ConfigDict, and model_validate methods.
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ for all data models. Use Pydantic v2 syntax, ConfigDict, and model_validate methods.
Applied to files:
src/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/model_widget_config_table.py
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add 'from_attributes=True' to ConfigDict for immutable value objects nested in other Pydantic models used with pytest-xdist parallel execution.
Applied to files:
src/omnibase_core/models/dashboard/model_table_column_config.py
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to tests/**/*.py : Use pytest markers for test categorization: pytest.mark.unit, pytest.mark.integration, pytest.mark.slow, pytest.mark.smoke, pytest.mark.performance
Applied to files:
tests/unit/models/dashboard/test_model_widget_definition.py
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to tests/**/*.py : Use pytest 8.4+ for testing. Use pytest fixtures, markers, and parallel execution with pytest-xdist.
Applied to files:
tests/unit/models/dashboard/test_model_widget_definition.py
🧬 Code graph analysis (7)
src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (2)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)src/omnibase_core/models/dashboard/model_metric_threshold.py (1)
ModelMetricThreshold(18-36)
tests/unit/enums/test_enum_widget_type.py (1)
src/omnibase_core/enums/enum_widget_type.py (3)
EnumWidgetType(11-39)is_data_bound(25-31)is_aggregation(34-39)
src/omnibase_core/enums/__init__.py (3)
src/omnibase_core/enums/enum_dashboard_status.py (1)
EnumDashboardStatus(11-38)src/omnibase_core/enums/enum_dashboard_theme.py (1)
EnumDashboardTheme(11-24)src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)
src/omnibase_core/models/dashboard/model_widget_config_chart.py (3)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)src/omnibase_core/models/dashboard/model_chart_axis_config.py (1)
ModelChartAxisConfig(11-25)src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
ModelChartSeriesConfig(13-30)
tests/unit/models/dashboard/test_model_widget_definition.py (5)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
ModelWidgetConfigChart(19-56)src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
ModelWidgetConfigMetricCard(16-51)src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
ModelWidgetConfigTable(18-54)src/omnibase_core/models/dashboard/model_widget_definition.py (1)
ModelWidgetDefinition(42-66)
tests/unit/models/dashboard/test_model_metric_threshold.py (1)
src/omnibase_core/models/dashboard/model_metric_threshold.py (1)
ModelMetricThreshold(18-36)
src/omnibase_core/models/dashboard/model_widget_config_table.py (2)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(11-39)src/omnibase_core/models/dashboard/model_table_column_config.py (1)
ModelTableColumnConfig(13-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (20)
tests/unit/models/dashboard/test_model_metric_threshold.py (1)
12-105: LGTM! Comprehensive test coverage for ModelMetricThreshold.The test suite thoroughly validates:
- All supported hex color formats (#RGB, #RRGGBB, #RGBA, #RRGGBBAA)
- Case sensitivity handling
- Invalid format rejection with proper error messages
- Serialization round-trip consistency
- Model immutability and extra field forbiddance
The use of
pytest.raises((ValidationError, TypeError))at line 95 appropriately handles potential variations across Pydantic versions, aligning with the PR's resilience improvements.src/omnibase_core/enums/enum_dashboard_theme.py (1)
11-24: LGTM! Clean enum implementation with useful helper property.The enum correctly inherits from
strandEnum, provides clear member values, and includes a helpfulis_autoproperty for checking system preference mode.src/omnibase_core/models/dashboard/model_metric_threshold.py (2)
13-15: LGTM! Comprehensive hex color pattern.The regex correctly validates all standard hex color formats (#RGB, #RRGGBB, #RGBA, #RRGGBBAA) with proper case-insensitivity.
18-36: LGTM! Well-structured Pydantic model with proper validation.The model correctly:
- Uses
ConfigDictwith frozen, extra="forbid", and from_attributes=True per guidelines- Employs PEP 604 union syntax (
str | None)- Implements field-level validation with clear error messages
- Provides comprehensive field descriptions
Based on learnings, the Pydantic 2.11+ syntax and configuration are correctly applied.
tests/unit/models/dashboard/test_dashboard_imports.py (2)
11-29: LGTM! Properly structured expected exports.The expected exports are correctly defined as an immutable tuple, listing all public dashboard model names that should be available from the package.
32-48: LGTM! Comprehensive import surface validation.The test suite effectively validates:
__all__immutability (tuple type check)- Presence of all expected exports using parametrized tests for clear failure reporting
- No unexpected exports through set equality check
This ensures the public API surface remains stable and correctly documented.
src/omnibase_core/enums/__init__.py (2)
66-68: LGTM! Dashboard enum imports correctly added.The new dashboard enums (EnumDashboardStatus, EnumDashboardTheme) are properly imported and positioned within the domain-grouped organization structure.
302-303: LGTM! Widget type enum import correctly added.The EnumWidgetType import is properly placed and includes the appropriate PR reference comment (OMN-1284).
src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
16-51: LGTM!The model correctly implements the discriminated union pattern with proper validation for
trend_keywhenshow_trendis enabled. The use of frozen config, immutable tuples for thresholds, and comprehensive field descriptions follows best practices.src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
19-46: LGTM!The model correctly defines chart widget configuration with proper frozen settings, discriminator pattern, and validation that pie charts don't require series while data-driven charts do.
src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
13-30: LGTM on overall structure.The model follows Pydantic v2 patterns correctly with frozen config, proper field descriptions, and appropriate use of Literal for series_type.
src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
18-54: LGTM!Excellent implementation with proper use of
Selffor the validator return type, appropriate constraints onpage_size, and sound validation logic ensuringdefault_sort_directionis only set whendefault_sort_keyis provided. The frozen config and immutable tuple for columns align with best practices.src/omnibase_core/models/dashboard/model_table_column_config.py (1)
13-34: LGTM!The model follows Pydantic v2 patterns with proper frozen config and
from_attributes=Truefor pytest-xdist compatibility. Field descriptions are clear and the structure is well-organized. Based on learnings, the ConfigDict configuration is correct.tests/unit/models/dashboard/test_model_widget_definition.py (5)
23-34: LGTM!Good use of helper functions to create reusable chart configurations. The
_make_pie_chart_configand_make_line_chart_confighelpers clearly demonstrate the validation difference between pie charts (no series required) and data-driven charts (series required).
36-95: LGTM!Thorough test coverage for creating widgets with all five config types (chart, table, metric_card, status_grid, event_feed). Each test properly verifies the
config_kinddiscriminator and widget-specific properties.
185-194: Good resilient exception handling for frozen model test.Catching both
ValidationErrorandTypeErrormakes this test resilient to potential differences in how Pydantic signals frozen model mutations across versions.
96-184: LGTM!Excellent roundtrip serialization tests validating that
model_dumpandmodel_validatework correctly with the discriminated union. The tests for all config types from dict format ensure deserialization works as expected.
400-430: LGTM!Good coverage of the metric card
trend_keyvalidation logic, testing all three scenarios: trend enabled without key (error), trend disabled without key (ok), and trend enabled with key (ok).tests/unit/enums/test_enum_widget_type.py (1)
13-138: Excellent comprehensive test coverage!The test suite thoroughly covers all aspects of
EnumWidgetType:
- Enum structure and values
- Inheritance and type behavior
- Custom properties (
is_data_bound,is_aggregation)- Mutual exclusivity validation
- String conversion and comparison
- Error handling
- JSON serialization
All tests are properly marked with
@pytest.mark.unit. This provides strong validation for the enum implementation.src/omnibase_core/models/dashboard/__init__.py (1)
1-70: LGTM! Well-organized package exports.The
__all__tuple is properly defined as immutable (line 44) with correct type annotation, and the exports are logically organized with helpful comment sections. This provides a clean public API surface for the dashboard models.
- Convert __all__ to immutable tuples in 18 dashboard files - Add hex color validation to ModelChartSeriesConfig and ModelWidgetConfigStatusGrid - Add min_value < max_value validation to ModelChartAxisConfig - Add width minimum constraint (ge=1) to ModelTableColumnConfig - Extract DATA_CHART_TYPES as class constant with Self return type - Recategorize dashboard enums together in enums/__init__.py - Add comprehensive Google-style docstrings to all dashboard models/enums - Parametrize chart series requirement tests - Make frozen-model mutation tests resilient to Pydantic exception types - Add new test_model_validations.py with 38 validation tests
Code Review for PR #363: Dashboard Models and EnumsSummaryExcellent implementation of dashboard-related models and enums! The code demonstrates strong adherence to ONEX project conventions, comprehensive testing (98 unit tests), and thoughtful design decisions. The author has addressed prior review feedback thoroughly, including all the "nitpicks" from the automated review. ✅ Strengths1. Architecture & Design
2. Code Quality
3. Testing Excellence
4. ONEX Convention Compliance
📝 Observations1. EnumDashboardTheme Migration ✅Great improvement from 2. Alias Pattern for Reserved Keywords ✅Smart use of 3. ClassVar Pattern for Constants ✅Excellent use of 4. Mapping for Immutability ✅Use of 🔍 Minor Considerations1. Hex Color Validation CoverageThe hex color validators in Impact: Low - would prevent invalid hex colors in metric thresholds 2. Thread Safety DocumentationPer CLAUDE.md threading guidelines, consider adding a docstring note that these models are thread-safe after creation (since they're frozen Pydantic models). This would help consumers understand concurrent usage patterns. Impact: Documentation only - models are already safe 3. ModelWidgetConfigTable Default Sort DirectionThe Impact: Very low - current implementation is functional 4. Status Colors Mutability
Impact: None if intentional - just document in docstring if runtime mutation is expected 🎯 Performance & Security
📊 Test CoverageThe test suite is comprehensive:
✅ Final RecommendationAPPROVE - This PR demonstrates excellent software engineering practices and is ready to merge. The code is:
The minor considerations noted above are optional improvements that don't block merging. Great work addressing all prior review feedback! Reviewed by: Claude Code (AI Assistant) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/omnibase_core/models/dashboard/model_metric_threshold.py:
- Around line 26-31: HEX_COLOR_PATTERN is currently exported but should be
internal: rename HEX_COLOR_PATTERN to _HEX_COLOR_PATTERN and remove it from the
module export list (update __all__ to exclude it); likewise rename any other
regex helper constants defined later (the similar pattern at lines 71-80) to
start with an underscore and update all internal references to use the new names
so callers outside the module cannot rely on these non-API helpers.
🧹 Nitpick comments (6)
src/omnibase_core/enums/enum_dashboard_theme.py (1)
20-63: Use identity comparison for Enum singletons (is) inis_auto.
Enums are singletons, soself is EnumDashboardTheme.SYSTEMis the clearest check.Proposed diff
@@ @property def is_auto(self) -> bool: @@ - return self == EnumDashboardTheme.SYSTEM + return self is EnumDashboardTheme.SYSTEMsrc/omnibase_core/models/dashboard/model_widget_config_event_feed.py (1)
69-92: Enforcewidget_typeconsistency withconfig_kind(prevent mismatched configs).
Right now callers can passwidget_type=EnumWidgetType.TABLEwhileconfig_kind="event_feed"and it will validate.Proposed diff
@@ -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, model_validator @@ class ModelWidgetConfigEventFeed(BaseModel): @@ auto_scroll: bool = Field(default=True, description="Auto-scroll to new events") + + @model_validator(mode="after") + def validate_widget_type_matches_kind(self) -> "ModelWidgetConfigEventFeed": + if self.widget_type is not EnumWidgetType.EVENT_FEED: + raise ValueError("widget_type must be EVENT_FEED for event_feed config") + return selfsrc/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
81-110: Tighten invariants: enforcewidget_type, and add basic string constraints.
Prevents invalid configs likewidget_type=CHARTon ametric_cardconfig and rejects emptymetric_key/label.Proposed diff
@@ -from pydantic import BaseModel, ConfigDict, Field, model_validator +from pydantic import BaseModel, ConfigDict, Field, model_validator @@ - metric_key: str = Field(..., description="Key to extract metric value from data") - label: str = Field(..., description="Metric display label") + metric_key: str = Field( + ..., min_length=1, description="Key to extract metric value from data" + ) + label: str = Field(..., min_length=1, description="Metric display label") @@ @model_validator(mode="after") def validate_trend_key_when_show_trend(self) -> "ModelWidgetConfigMetricCard": """Validate that trend_key is set when show_trend is enabled.""" if self.show_trend and not self.trend_key: raise ValueError("trend_key must be set when show_trend is enabled") + if self.widget_type is not EnumWidgetType.METRIC_CARD: + raise ValueError("widget_type must be METRIC_CARD for metric_card config") return selfsrc/omnibase_core/models/dashboard/model_widget_config_status_grid.py (1)
100-108: Consider usingMapping[str, str]for immutability consistency.The
status_colorsfield usesdict[str, str], but the PR description mentions "changed some types for immutability (extra_config → Mapping)". For consistency with the immutability goal and to better signal intent, consider usingcollections.abc.Mapping[str, str]as the type annotation.♻️ Proposed refactor
Add the import:
+from collections.abc import Mapping from typing import LiteralUpdate the field type:
- status_colors: dict[str, str] = Field( + status_colors: Mapping[str, str] = Field( default_factory=lambda: { "healthy": "#22c55e", "warning": "#eab308", "error": "#ef4444", "unknown": "#6b7280", }, description="Status value to color mapping", )Update the validator signature:
@field_validator("status_colors") @classmethod - def validate_status_colors(cls, v: dict[str, str]) -> dict[str, str]: + def validate_status_colors(cls, v: Mapping[str, str]) -> Mapping[str, str]: """Validate that all color values are valid hex color codes."""tests/unit/models/dashboard/test_model_validations.py (2)
17-65: Consider parametrizing the axis range cases to reduce repetition.Proposed refactor sketch
+@pytest.mark.parametrize( + ("min_value", "max_value"), + [ + (0.0, 100.0), + (None, 100.0), + (0.0, None), + (None, None), + (-100.0, -50.0), + ], +) +def test_valid_ranges(self, min_value: float | None, max_value: float | None) -> None: + config = ModelChartAxisConfig(min_value=min_value, max_value=max_value) + assert config.min_value == min_value + assert config.max_value == max_value
67-129: Color validation coverage is solid; optional: parametrize valid/invalid inputs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
src/omnibase_core/enums/__init__.pysrc/omnibase_core/enums/enum_dashboard_status.pysrc/omnibase_core/enums/enum_dashboard_theme.pysrc/omnibase_core/enums/enum_widget_type.pysrc/omnibase_core/models/dashboard/__init__.pysrc/omnibase_core/models/dashboard/model_capability_view.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_dashboard_layout_config.pysrc/omnibase_core/models/dashboard/model_event_filter.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_node_view.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/models/dashboard/model_widget_config_event_feed.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/models/dashboard/model_widget_definition.pytests/unit/models/dashboard/test_model_dashboard_config.pytests/unit/models/dashboard/test_model_node_view.pytests/unit/models/dashboard/test_model_validations.pytests/unit/models/dashboard/test_model_widget_definition.py
🚧 Files skipped from review as they are similar to previous changes (11)
- src/omnibase_core/models/dashboard/model_node_view.py
- src/omnibase_core/enums/enum_widget_type.py
- src/omnibase_core/models/dashboard/model_status_item_config.py
- tests/unit/models/dashboard/test_model_widget_definition.py
- src/omnibase_core/models/dashboard/model_widget_config_chart.py
- src/omnibase_core/models/dashboard/model_dashboard_layout_config.py
- src/omnibase_core/models/dashboard/model_event_filter.py
- tests/unit/models/dashboard/test_model_node_view.py
- src/omnibase_core/enums/init.py
- src/omnibase_core/models/dashboard/model_chart_series_config.py
- src/omnibase_core/enums/enum_dashboard_status.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Use 'poetry run' for all Python commands (pytest, mypy, python -m, etc.). Never use pip or direct python execution.
Always use protocol names for service resolution (e.g., 'ProtocolEventBus', 'ProtocolLogger'), never concrete class names. Use duck typing without isinstance checks.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception. Include error context in the context parameter.
Use the @standard_error_handling decorator on async functions to eliminate 6+ lines of try/catch boilerplate in error handling.
Use PEP 604 union syntax (e.g., 'str | None') instead of legacy Optional/Union. Enforce via ruff rule UP007.
Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers.
Order exception tuples alphabetically by exception class name (e.g., except (AttributeError, TypeError, ValidationError) not except (ValueError, TypeError, AttributeError)).
Use centralized exception constants from omnibase_core.errors.exception_groups (PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS, FILE_IO_ERRORS, etc.) instead of manually listing exceptions.
Maintain 100% mypy strict mode compliance (0 errors) across all source files using type annotations. Both mypy and pyright must pass in CI.
Format Python code with Black: poetry run black src/ tests/. Sort imports with isort: poetry run isort src/ tests/. Lint with ruff: poetry run ruff check src/ tests/
Do NOT use 'from future import annotations' when runtime type introspection is needed (Pydantic models, FastAPI endpoints). Use it when forward references are needed or to prevent circular imports.
Use EnumNodeKind for architectural role (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) for pipeline routing. Use EnumNodeType for implementation type (COMPUTE_GENERIC, TRA...
Files:
src/omnibase_core/enums/enum_dashboard_theme.pytests/unit/models/dashboard/test_model_validations.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/models/dashboard/__init__.pysrc/omnibase_core/models/dashboard/model_widget_config_event_feed.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/models/dashboard/model_widget_definition.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pytests/unit/models/dashboard/test_model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_capability_view.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_table_column_config.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Follow directory-specific naming prefixes for Python files in src/omnibase_core/. Examples: cli_, constants_, container_, decorator_, enum_, error_, factory_, mixin_, model_, node_, etc.
Use FileRegistry for loading YAML contracts: registry = FileRegistry(); contract = registry.load(Path('config/runtime_host.yaml'))
Use ModelEventEnvelope for inter-service event communication. Structure events with proper envelope wrapping for consistency.
Files:
src/omnibase_core/enums/enum_dashboard_theme.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/models/dashboard/__init__.pysrc/omnibase_core/models/dashboard/model_widget_config_event_feed.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/models/dashboard/model_widget_definition.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_capability_view.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_table_column_config.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% code coverage across test suite. Run tests with: poetry run pytest tests/ --cov=src/omnibase_core/
Run all tests with parallel workers: poetry run pytest tests/ uses 4 workers locally with 60s timeout and loadscope distribution. CI uses 20 parallel splits on GitHub Actions.
Use pytest markers for test categorization: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
Use pytest 8.4+ for testing. Use pytest fixtures, markers, and parallel execution with pytest-xdist.
Files:
tests/unit/models/dashboard/test_model_validations.pytests/unit/models/dashboard/test_model_dashboard_config.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Add 'from_attributes=True' to ConfigDict for immutable value objects nested in other Pydantic models used with pytest-xdist parallel execution.
Use Pydantic 2.11+ for all data models. Use Pydantic v2 syntax, ConfigDict, and model_validate methods.
Files:
src/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_chart_axis_config.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/models/dashboard/__init__.pysrc/omnibase_core/models/dashboard/model_widget_config_event_feed.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/models/dashboard/model_widget_definition.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_capability_view.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_table_column_config.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ for all data models. Use Pydantic v2 syntax, ConfigDict, and model_validate methods.
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add 'from_attributes=True' to ConfigDict for immutable value objects nested in other Pydantic models used with pytest-xdist parallel execution.
Applied to files:
tests/unit/models/dashboard/test_model_validations.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_table_column_config.py
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ for all data models. Use Pydantic v2 syntax, ConfigDict, and model_validate methods.
Applied to files:
src/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/models/dashboard/model_table_column_config.py
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelEventEnvelope for inter-service event communication. Structure events with proper envelope wrapping for consistency.
Applied to files:
src/omnibase_core/models/dashboard/model_widget_config_event_feed.py
🧬 Code graph analysis (8)
tests/unit/models/dashboard/test_model_validations.py (4)
src/omnibase_core/models/dashboard/model_chart_axis_config.py (1)
ModelChartAxisConfig(13-40)src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
ModelChartSeriesConfig(19-47)src/omnibase_core/models/dashboard/model_table_column_config.py (1)
ModelTableColumnConfig(13-36)src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (1)
ModelWidgetConfigStatusGrid(51-120)
src/omnibase_core/models/dashboard/model_dashboard_config.py (4)
src/omnibase_core/enums/enum_dashboard_status.py (1)
EnumDashboardStatus(27-95)src/omnibase_core/enums/enum_dashboard_theme.py (1)
EnumDashboardTheme(25-63)src/omnibase_core/models/dashboard/model_dashboard_layout_config.py (1)
ModelDashboardLayoutConfig(28-66)src/omnibase_core/models/dashboard/model_widget_definition.py (1)
ModelWidgetDefinition(90-150)
src/omnibase_core/models/dashboard/__init__.py (11)
src/omnibase_core/models/dashboard/model_capability_view.py (1)
ModelCapabilityView(13-30)src/omnibase_core/models/dashboard/model_chart_axis_config.py (1)
ModelChartAxisConfig(13-40)src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
ModelChartSeriesConfig(19-47)src/omnibase_core/models/dashboard/model_dashboard_config.py (1)
ModelDashboardConfig(45-110)src/omnibase_core/models/dashboard/model_dashboard_layout_config.py (1)
ModelDashboardLayoutConfig(28-66)src/omnibase_core/models/dashboard/model_event_filter.py (1)
ModelEventFilter(27-67)src/omnibase_core/models/dashboard/model_status_item_config.py (1)
ModelStatusItemConfig(11-25)src/omnibase_core/models/dashboard/model_table_column_config.py (1)
ModelTableColumnConfig(13-36)src/omnibase_core/models/dashboard/model_widget_config_event_feed.py (1)
ModelWidgetConfigEventFeed(40-92)src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
ModelWidgetConfigTable(42-105)src/omnibase_core/models/dashboard/model_widget_definition.py (1)
ModelWidgetDefinition(90-150)
src/omnibase_core/models/dashboard/model_widget_config_event_feed.py (2)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(25-92)src/omnibase_core/models/dashboard/model_event_filter.py (1)
ModelEventFilter(27-67)
src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (2)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(25-92)src/omnibase_core/models/dashboard/model_status_item_config.py (1)
ModelStatusItemConfig(11-25)
src/omnibase_core/models/dashboard/model_widget_definition.py (5)
src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
ModelWidgetConfigChart(44-114)src/omnibase_core/models/dashboard/model_widget_config_event_feed.py (1)
ModelWidgetConfigEventFeed(40-92)src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
ModelWidgetConfigMetricCard(44-110)src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (1)
ModelWidgetConfigStatusGrid(51-120)src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
ModelWidgetConfigTable(42-105)
src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (2)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(25-92)src/omnibase_core/models/dashboard/model_metric_threshold.py (1)
ModelMetricThreshold(34-80)
src/omnibase_core/models/dashboard/model_metric_threshold.py (1)
src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
validate_hex_color(40-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code Quality
- GitHub Check: claude-review
🔇 Additional comments (14)
src/omnibase_core/models/dashboard/model_dashboard_config.py (1)
45-110: Model config and field constraints look consistent with the repo’s Pydantic-v2 “immutable + forbid extras + from_attributes” pattern.
Nothing to change here. (Based on learnings, from_attributes=True is correctly present.)src/omnibase_core/models/dashboard/model_chart_axis_config.py (1)
13-40: Min/max validation is clear and correctly scoped to “both provided”.
No changes requested.tests/unit/models/dashboard/test_model_dashboard_config.py (1)
1-208: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- Model creation paths (minimal, with defaults, with widgets)
- Refresh interval behavior (positive values and None for disabled)
- Serialization roundtrips with nested widget configs
- Immutability enforcement (handles both ValidationError and TypeError edge cases)
- Validation error paths (invalid IDs, invalid refresh intervals)
- Enum value coverage (EnumDashboardTheme, EnumDashboardStatus)
- UUID handling (creation from strings)
The test organization is clean and follows pytest best practices with class-based grouping and descriptive test names.
src/omnibase_core/models/dashboard/model_table_column_config.py (1)
13-36: LGTM! Clean model implementation.The model follows all Pydantic v2 best practices:
- Immutable configuration (frozen=True, from_attributes=True)
- Proper field validation (width ge=1 constraint)
- Clear field descriptions
- Modern type annotations (PEP 604 union syntax)
src/omnibase_core/models/dashboard/model_capability_view.py (1)
13-30: LGTM! Well-designed UI projection model.The model demonstrates excellent practices:
- Immutable configuration with frozen=True
- Uses immutable tuple for tags collection
- Clear separation of concerns (UI projection vs canonical model)
- Comprehensive field descriptions
- Modern type annotations
src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
42-105: LGTM! Excellent validation and design.The model showcases strong practices:
- Immutable collections (tuple for columns)
- Comprehensive field constraints (page_size range 1-100)
- Smart validation logic ensuring default_sort_direction requires default_sort_key
- Clear documentation with usage examples
- Proper ConfigDict settings
The model_validator at lines 98-105 effectively prevents invalid sort configurations.
tests/unit/models/dashboard/test_model_validations.py (4)
6-14: Good: tests exercise the public re-export surface (omnibase_core.models.dashboard).
204-247: Width constraint tests match thege=1contract.
249-288: Roundtrip assertions usingmodel_dump()/model_validate()look good for these models.
131-202: The testtest_empty_dict_valid()is correct. The model intentionally allows emptystatus_colorsbecause its documentation explicitly states that unmapped status values fall back to the UI's default "unknown" color. This fallback behavior is by design, so enforcing the presence of "unknown" is unnecessary.Regarding the mutable dict in a frozen model: this follows standard Pydantic behavior where
frozen=Trueprevents attribute reassignment but allows mutation of nested structures. If stricter immutability is desired, consider usingtypes.MappingProxyTypeor a custom immutable mapping type for thestatus_colorsfield, though this is an optional enhancement rather than a requirement.src/omnibase_core/models/dashboard/model_metric_threshold.py (1)
65-70: Model config matches repo guidance (frozen,extra="forbid",from_attributes=True). Based on learnings, this is the right default for nested immutable value objects.src/omnibase_core/models/dashboard/model_widget_definition.py (2)
80-87: Discriminated union wiring looks correct (config_kinddiscriminator).
Please double-check all member config models defineconfig_kindas aLiteral[...]with a matching default (otherwise union parsing will fail).
133-150: Nice:extra_configtyped asMapping[str, str](better thandictfor “frozen” models).
Optional: if you truly require string-only values without coercion, considerMapping[str, StrictStr].src/omnibase_core/models/dashboard/__init__.py (1)
55-119: No circular import risk detected. All dashboard submodules import only from siblings or external packages, never fromomnibase_core.models.dashboarditself. The earlier matches were docstring examples, not actual code, so this init-heavy re-export pattern is safe.
- Use identity comparison (is) for enum singletons in dashboard enums - Rename HEX_COLOR_PATTERN to _HEX_COLOR_PATTERN (internal helper) - Add widget_type/config_kind consistency validators to all widget configs - Add min_length=1 constraints to required string fields - Use Self return type and Mapping for immutability - Convert __all__ to tuple in typed_dict_monitoring_dashboard.py - Add @pytest.mark.unit markers to test classes - Parametrize color validation tests for better coverage - Add 24 new validation tests for consistency and constraints
Code Review: Dashboard Models and Enums (PR #363)SummaryThis PR adds comprehensive dashboard models and enums for UI presentation. Overall, this is excellent work with high-quality implementation, thorough testing (98 unit tests!), and excellent adherence to ONEX conventions. The code is production-ready with only minor suggestions below. ✅ Strengths1. Exceptional Code Quality
2. Robust Validation
3. Comprehensive Testing
4. Design Excellence
🔍 Minor Suggestions1. Consider Enum Integration for Literal Types (Low Priority)Several models use Literal types (chart_type, series_type) that could become enums if referenced elsewhere. Current approach is fine if these are purely config-level discriminators. 2. Hex Color Validation DRY (Low Priority)The hex color validation pattern is duplicated in 3 places. Consider extracting to omnibase_core/validation/validator_hex_color.py when you reach 5+ uses. 3. Type Annotation for extra_config (Very Low Priority)Using Mapping[str, str] is correct for immutable frozen models and enforces the immutability contract. 🎯 Architecture & Design Decisions
🔒 Security & PerformanceSecurity: ✅ No Concerns
Performance: ✅ Efficient
📊 Test CoverageExcellent coverage across all critical areas:
🚀 RecommendationsMerge Decision: APPROVE ✅This PR is production-ready. All suggestions are optional refinements for future work. Post-Merge Considerations
🎉 ConclusionExcellent work, @jonahgabriel! This PR demonstrates:
The code is clean, well-tested, and follows all project standards. No blocking issues found. Recommendation: APPROVE and MERGE ✅ Review generated using repository CLAUDE.md for ONEX standards compliance |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/omnibase_core/models/dashboard/model_status_item_config.py:
- Around line 11-25: The icon field on ModelStatusItemConfig currently allows
empty strings which may be invalid for the UI; update the icon Field on
ModelStatusItemConfig to forbid empty strings by either setting
Field(default=None, min_length=1, description=...) or adding a small validator
on icon to convert empty strings to None or raise a validation error (target the
ModelStatusItemConfig.icon Field/attribute or a pydantic validator named
validate_icon).
In @src/omnibase_core/models/dashboard/model_widget_config_chart.py:
- Around line 119-138: The validator
validate_widget_type_config_kind_consistency currently uses value comparison for
the enum (self.widget_type != EnumWidgetType.CHART); change this to identity
comparison (self.widget_type is not EnumWidgetType.CHART) so the enum singleton
is compared with is/is not; leave the config_kind string check against
_EXPECTED_CONFIG_KIND unchanged and return self as before.
In @src/omnibase_core/models/dashboard/model_widget_config_status_grid.py:
- Around line 90-125: The status_colors dict is mutable despite model-level
frozen; update the validate_status_colors classmethod to wrap the incoming
mapping in a MappingProxyType before returning it so the stored value is an
immutable read-only view; ensure you import MappingProxyType and accept any
Mapping[str,str] (or dict) input, validate each color with _HEX_COLOR_PATTERN as
currently done, then return MappingProxyType(validated_mapping) instead of the
original mapping.
🧹 Nitpick comments (8)
tests/unit/models/dashboard/test_model_node_view.py (1)
16-110: Excellent test coverage for core model behavior.The tests comprehensively cover:
- Minimal and full initialization
- Default values
- Pydantic v2 serialization roundtrips (using
model_dump,model_validate,model_dump_json,model_validate_json)- Frozen model immutability with appropriate exception handling
- Validation constraints (min_length=1)
The frozen model test (lines 85-94) correctly catches both
ValidationErrorandTypeErrorwith a clear explanatory comment.Optional: Consider parametrizing validation tests
You could slightly reduce duplication by parametrizing the empty string validation tests:
@pytest.mark.parametrize( "field_name,field_value", [ ("node_id", ""), ("name", ""), ], ) def test_empty_required_field_raises(self, field_name: str, field_value: str) -> None: """Test that empty required fields raise ValidationError.""" kwargs = {"node_id": "n1", "name": "test"} kwargs[field_name] = field_value with pytest.raises(ValidationError): ModelNodeView(**kwargs)This is purely optional—the current approach is clear and explicit.
tests/unit/models/dashboard/test_model_dashboard_config.py (1)
34-210: LGTM: Comprehensive test coverage with proper markers.The test suite demonstrates excellent coverage:
- All tests properly annotated with
@pytest.mark.unit- Covers defaults, validation (invalid UUID, invalid refresh), serialization round-trips, immutability, and enum handling
- Frozen model test (lines 145-154) correctly handles both
ValidationErrorandTypeErrorgiven Pydantic v2's behavior- Helper functions (
_make_pie_chart_config,_make_line_chart_config) provide clear test fixturesOptional: Consider pytest.mark.parametrize for enum tests.
Lines 173-191 iterate over enum values using loops. While readable, using
@pytest.mark.parametrizewould provide better test isolation and clearer failure reports:♻️ Optional refactor
- def test_theme_values(self) -> None: - """Test theme enum values.""" - for theme in EnumDashboardTheme: - dashboard = ModelDashboardConfig( - dashboard_id=uuid4(), - name="Test", - theme=theme, - ) - assert dashboard.theme == theme + @pytest.mark.parametrize("theme", list(EnumDashboardTheme)) + def test_theme_values(self, theme: EnumDashboardTheme) -> None: + """Test theme enum values.""" + dashboard = ModelDashboardConfig( + dashboard_id=uuid4(), + name="Test", + theme=theme, + ) + assert dashboard.theme == theme - def test_status_enum_values(self) -> None: - """Test initial_status enum values.""" - for status in EnumDashboardStatus: - dashboard = ModelDashboardConfig( - dashboard_id=uuid4(), - name="Test", - initial_status=status, - ) - assert dashboard.initial_status == status + @pytest.mark.parametrize("status", list(EnumDashboardStatus)) + def test_status_enum_values(self, status: EnumDashboardStatus) -> None: + """Test initial_status enum values.""" + dashboard = ModelDashboardConfig( + dashboard_id=uuid4(), + name="Test", + initial_status=status, + ) + assert dashboard.initial_status == statustests/unit/models/dashboard/test_model_metric_threshold.py (1)
38-52: LGTM! Good coverage of invalid hex color formats.The parametrized test covers various invalid hex format scenarios and properly validates error handling.
Optional: Consider tighter error message matching
You could make the error matching more specific to catch unintended error message changes:
- with pytest.raises(ValidationError, match="Invalid hex color format"): + with pytest.raises(ValidationError, match=r"Invalid hex color format.*Expected #RGB"):This is purely optional—the current approach works fine.
src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
128-128: Consider using identity comparison for enum singletons.All widget config files currently use
!=for EnumWidgetType comparisons (metric_card, chart, table, status_grid, event_feed). While this works correctly, usingis notwould be more idiomatic since enum members are singletons.This is a consistent pattern across the codebase. If adopting identity comparisons, apply the change uniformly to all widget config files.
src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (2)
62-75: Clarify/enforce the"unknown"fallback behavior forstatus_colors.
Docs say unknown statuses use the"unknown"color (gray by default), but the model currently permitsstatus_colors={}(and any mapping missing"unknown"), which undermines that guarantee. Either:
- enforce
"unknown"always exists (and inject the default when missing), or- update the docstring to say callers must include
"unknown"if they want a fallback.Also applies to: 104-112, 204-208
126-147: (Optional) Preferis/is notfor Enum singletons to match the stated design decision.
Not required for correctness, but it matches the “Enum members are singletons” convention and the PR summary note.src/omnibase_core/models/dashboard/model_table_column_config.py (1)
13-36: Consider renamingformatto avoid shadowing built-in names (keep JSON name via alias if needed).
Not a blocker, butformatis easy to confuse withstr.format. If you want to keep the external field name, useformat_: str | None = Field(..., alias="format", serialization_alias="format").tests/unit/models/dashboard/test_model_validations.py (1)
375-441: Avoid brittle assertions on Pydantic’s built-in error message text; assert onerrors()types instead.
match="String should have at least 1"/match="greater than or equal to 1"can change across Pydantic versions. Prefer checkingexc.value.errors()entries (e.g.,type == "string_too_short"/type == "greater_than_equal"andloc).Also applies to: 234-248
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/omnibase_core/enums/enum_dashboard_status.pysrc/omnibase_core/enums/enum_dashboard_theme.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/models/dashboard/model_widget_config_event_feed.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/models/dashboard/model_widget_config_table.pysrc/omnibase_core/types/typed_dict_monitoring_dashboard.pytests/unit/models/dashboard/test_model_dashboard_config.pytests/unit/models/dashboard/test_model_metric_threshold.pytests/unit/models/dashboard/test_model_node_view.pytests/unit/models/dashboard/test_model_validations.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/omnibase_core/enums/enum_dashboard_status.py
- src/omnibase_core/models/dashboard/model_chart_series_config.py
- src/omnibase_core/models/dashboard/model_widget_config_table.py
- src/omnibase_core/models/dashboard/model_widget_config_event_feed.py
- src/omnibase_core/models/dashboard/model_metric_threshold.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Use 'poetry run' for all Python commands (pytest, mypy, python -m, etc.). Never use pip or direct python execution.
Always use protocol names for service resolution (e.g., 'ProtocolEventBus', 'ProtocolLogger'), never concrete class names. Use duck typing without isinstance checks.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception. Include error context in the context parameter.
Use the @standard_error_handling decorator on async functions to eliminate 6+ lines of try/catch boilerplate in error handling.
Use PEP 604 union syntax (e.g., 'str | None') instead of legacy Optional/Union. Enforce via ruff rule UP007.
Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers.
Order exception tuples alphabetically by exception class name (e.g., except (AttributeError, TypeError, ValidationError) not except (ValueError, TypeError, AttributeError)).
Use centralized exception constants from omnibase_core.errors.exception_groups (PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS, FILE_IO_ERRORS, etc.) instead of manually listing exceptions.
Maintain 100% mypy strict mode compliance (0 errors) across all source files using type annotations. Both mypy and pyright must pass in CI.
Format Python code with Black: poetry run black src/ tests/. Sort imports with isort: poetry run isort src/ tests/. Lint with ruff: poetry run ruff check src/ tests/
Do NOT use 'from future import annotations' when runtime type introspection is needed (Pydantic models, FastAPI endpoints). Use it when forward references are needed or to prevent circular imports.
Use EnumNodeKind for architectural role (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) for pipeline routing. Use EnumNodeType for implementation type (COMPUTE_GENERIC, TRA...
Files:
tests/unit/models/dashboard/test_model_node_view.pysrc/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/types/typed_dict_monitoring_dashboard.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pytests/unit/models/dashboard/test_model_validations.pytests/unit/models/dashboard/test_model_metric_threshold.pytests/unit/models/dashboard/test_model_dashboard_config.pysrc/omnibase_core/enums/enum_dashboard_theme.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% code coverage across test suite. Run tests with: poetry run pytest tests/ --cov=src/omnibase_core/
Run all tests with parallel workers: poetry run pytest tests/ uses 4 workers locally with 60s timeout and loadscope distribution. CI uses 20 parallel splits on GitHub Actions.
Use pytest markers for test categorization: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
Use pytest 8.4+ for testing. Use pytest fixtures, markers, and parallel execution with pytest-xdist.
Files:
tests/unit/models/dashboard/test_model_node_view.pytests/unit/models/dashboard/test_model_validations.pytests/unit/models/dashboard/test_model_metric_threshold.pytests/unit/models/dashboard/test_model_dashboard_config.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Follow directory-specific naming prefixes for Python files in src/omnibase_core/. Examples: cli_, constants_, container_, decorator_, enum_, error_, factory_, mixin_, model_, node_, etc.
Use FileRegistry for loading YAML contracts: registry = FileRegistry(); contract = registry.load(Path('config/runtime_host.yaml'))
Use ModelEventEnvelope for inter-service event communication. Structure events with proper envelope wrapping for consistency.
Files:
src/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/types/typed_dict_monitoring_dashboard.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/enums/enum_dashboard_theme.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Add 'from_attributes=True' to ConfigDict for immutable value objects nested in other Pydantic models used with pytest-xdist parallel execution.
Use Pydantic 2.11+ for all data models. Use Pydantic v2 syntax, ConfigDict, and model_validate methods.
Files:
src/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_widget_config_metric_card.pysrc/omnibase_core/models/dashboard/model_widget_config_chart.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ for all data models. Use Pydantic v2 syntax, ConfigDict, and model_validate methods.
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic 2.11+ for all data models. Use Pydantic v2 syntax, ConfigDict, and model_validate methods.
Applied to files:
src/omnibase_core/models/dashboard/model_table_column_config.pysrc/omnibase_core/models/dashboard/model_status_item_config.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.py
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add 'from_attributes=True' to ConfigDict for immutable value objects nested in other Pydantic models used with pytest-xdist parallel execution.
Applied to files:
src/omnibase_core/models/dashboard/model_table_column_config.py
🧬 Code graph analysis (5)
tests/unit/models/dashboard/test_model_node_view.py (1)
src/omnibase_core/models/dashboard/model_node_view.py (3)
ModelNodeView(11-114)get_display_name(98-104)get_qualified_id(106-114)
src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (6)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(25-92)src/omnibase_core/models/dashboard/model_metric_threshold.py (1)
ModelMetricThreshold(34-80)src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
validate_widget_type_config_kind_consistency(120-138)src/omnibase_core/models/dashboard/model_widget_config_event_feed.py (1)
validate_widget_type_config_kind_consistency(98-118)src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (1)
validate_widget_type_config_kind_consistency(127-147)src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
validate_widget_type_config_kind_consistency(111-129)
src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (8)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(25-92)src/omnibase_core/models/dashboard/model_status_item_config.py (1)
ModelStatusItemConfig(11-25)src/omnibase_core/pipeline/replay/session_replay.py (1)
mode(118-129)src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
validate_widget_type_config_kind_consistency(120-138)src/omnibase_core/models/dashboard/model_widget_config_event_feed.py (1)
validate_widget_type_config_kind_consistency(98-118)src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
validate_widget_type_config_kind_consistency(118-138)src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
validate_widget_type_config_kind_consistency(111-129)src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
value(33-35)
tests/unit/models/dashboard/test_model_validations.py (6)
src/omnibase_core/models/dashboard/model_chart_axis_config.py (1)
ModelChartAxisConfig(13-40)src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
ModelChartSeriesConfig(19-49)src/omnibase_core/models/dashboard/model_status_item_config.py (1)
ModelStatusItemConfig(11-25)src/omnibase_core/models/dashboard/model_widget_config_event_feed.py (1)
ModelWidgetConfigEventFeed(43-118)src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (1)
ModelWidgetConfigStatusGrid(55-147)src/omnibase_core/models/dashboard/model_widget_config_table.py (1)
ModelWidgetConfigTable(45-129)
tests/unit/models/dashboard/test_model_metric_threshold.py (1)
src/omnibase_core/models/dashboard/model_metric_threshold.py (1)
ModelMetricThreshold(34-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code Quality
- GitHub Check: claude-review
🔇 Additional comments (17)
tests/unit/models/dashboard/test_model_node_view.py (2)
1-14: LGTM! Clean imports and proper test markers.The imports are minimal and appropriate. The
@pytest.mark.unitmarker is correctly applied as per coding guidelines.
112-175: Thorough testing of utility methods and validation rules.The utility method tests properly cover all branches:
get_display_name(): with explicit display_name and fallback to nameget_qualified_id(): with and without namespaceValidation tests comprehensively verify:
- Status field combinations (inactive/unhealthy)
extra='forbid'configuration- Missing required fields (both
node_idandnametested independently)All type ignores are appropriately used for intentional validation errors.
src/omnibase_core/types/typed_dict_monitoring_dashboard.py (1)
53-53: LGTM: Tuple conversion for immutable exports.Converting
__all__from a list to a tuple is correct. Tuples are immutable and more appropriate for static module exports, aligning with the PR's API/typing refinements.src/omnibase_core/enums/enum_dashboard_theme.py (1)
25-63: LGTM: Well-structured enum with correct singleton comparison.The enum implementation is solid:
- String inheritance ensures Pydantic compatibility
- The
is_autoproperty correctly uses identity comparison (is) for enum singleton checks, which is efficient and matches the PR's enum comparison improvements- Clear documentation for all members and the property
tests/unit/models/dashboard/test_model_metric_threshold.py (7)
1-10: LGTM! Clean imports and proper licensing.The module header, license, and imports follow project conventions correctly.
12-14: LGTM! Proper test class structure.The test class follows pytest conventions with the @pytest.mark.unit marker and appropriate naming.
16-30: LGTM! Comprehensive hex color validation coverage.The parametrized test thoroughly covers all valid hex color formats (3/4/6/8-digit with various cases) matching the model's validation requirements.
32-36: LGTM! Clean test for optional label field.The test correctly verifies threshold creation with an optional label.
54-59: LGTM! Proper Pydantic v2 serialization testing.The test correctly uses Pydantic v2 methods (model_dump, model_validate) and validates roundtrip serialization, aligning with project guidelines.
61-65: LGTM! Proper immutability testing.The frozen model test correctly validates that the model is immutable, catching both ValidationError and TypeError for defensive coverage.
67-74: LGTM! Correct extra field validation.The test properly validates that extra fields are forbidden per the model's ConfigDict settings.
src/omnibase_core/models/dashboard/model_widget_config_chart.py (4)
1-42: LGTM! Module structure follows conventions.The imports, exports, and module documentation follow project guidelines correctly. The use of a tuple for
__all__, Pydantic v2 syntax, and absence offrom __future__ import annotations(appropriate for Pydantic models) are all correct.
47-87: LGTM! Class configuration aligns with project standards.The frozen configuration,
from_attributes=True, and use ofClassVar[frozenset]forDATA_CHART_TYPEScorrectly implement immutability and compatibility requirements from the coding guidelines.
89-108: LGTM! Field definitions are well-structured.All fields use appropriate types (Literal discriminators, PEP 604 union syntax, immutable tuple collections) and include clear descriptions. The design correctly leverages Pydantic v2 features.
110-117: LGTM! Series validation is correctly implemented.The validator appropriately enforces the business rule that data-driven chart types require at least one series configuration. The Pydantic v2 validator pattern with
mode="after"andSelfreturn type is correct.src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (2)
84-108: LGTM: Field definitions follow best practices.The field definitions are well-structured with appropriate constraints:
- Frozen configuration ensures immutability
- Required string fields have
min_length=1constraintsthresholdsuses tuple for immutabilityprecisionhas proper bounds (0-10)- Optional fields correctly use
| Nonesyntax
110-115: LGTM: Trend validation is correctly implemented.The validator properly enforces the business rule that
trend_keymust be provided whenshow_trendis enabled, matching the validation requirements outlined in the PR objectives.
| model_config = ConfigDict(frozen=True, extra="forbid", from_attributes=True) | ||
|
|
||
| config_kind: Literal["status_grid"] = Field( | ||
| default="status_grid", description="Discriminator for widget config union" | ||
| ) | ||
| widget_type: EnumWidgetType = Field( | ||
| default=EnumWidgetType.STATUS_GRID, description="Widget type enum value" | ||
| ) | ||
| items: tuple[ModelStatusItemConfig, ...] = Field( | ||
| default=(), description="Status items to display" | ||
| ) | ||
| columns: int = Field(default=3, ge=1, le=12, description="Number of grid columns") | ||
| show_labels: bool = Field(default=True, description="Show item labels") | ||
| compact: bool = Field(default=False, description="Use compact display mode") | ||
| status_colors: Mapping[str, str] = Field( | ||
| default_factory=lambda: { | ||
| "healthy": "#22c55e", | ||
| "warning": "#eab308", | ||
| "error": "#ef4444", | ||
| "unknown": "#6b7280", | ||
| }, | ||
| description="Status value to color mapping", | ||
| ) | ||
|
|
||
| @field_validator("status_colors") | ||
| @classmethod | ||
| def validate_status_colors(cls, v: Mapping[str, str]) -> Mapping[str, str]: | ||
| """Validate that all color values are valid hex color codes.""" | ||
| for status, color in v.items(): | ||
| if not _HEX_COLOR_PATTERN.match(color): | ||
| raise ValueError( | ||
| f"Invalid hex color format for status '{status}': {color}. " | ||
| "Expected #RGB, #RRGGBB, #RGBA, or #RRGGBBAA" | ||
| ) | ||
| return v | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*/src/omnibase_core/models/dashboard/model_widget_config_status_grid.py" -type fRepository: OmniNode-ai/omnibase_core
Length of output: 140
🏁 Script executed:
cat -n src/omnibase_core/models/dashboard/model_widget_config_status_grid.pyRepository: OmniNode-ai/omnibase_core
Length of output: 6680
🏁 Script executed:
# Check if Pydantic is in the project and its version
find . -name "pyproject.toml" -type f | head -1 | xargs cat | grep -A 5 pydanticRepository: OmniNode-ai/omnibase_core
Length of output: 861
🏁 Script executed:
cd /tmp && python3 << 'EOF'
import sys
sys.path.insert(0, '/root/repo')
from typing import Mapping
from pydantic import BaseModel, ConfigDict, Field
# Test 1: Frozen model with dict field (what the current code does)
class TestModel(BaseModel):
model_config = ConfigDict(frozen=True)
status_colors: Mapping[str, str] = Field(
default_factory=lambda: {
"healthy": "#22c55e",
"warning": "#eab308",
"error": "#ef4444",
"unknown": "#6b7280",
}
)
instance = TestModel()
print("=== Test 1: Frozen model with dict Mapping field ===")
print(f"Original: {instance.status_colors}")
print(f"Type: {type(instance.status_colors)}")
# Try to mutate the underlying dict
try:
instance.status_colors["healthy"] = "#000000"
print(f"✓ MUTABLE: Successfully mutated to {instance.status_colors}")
except TypeError as e:
print(f"✗ IMMUTABLE: {e}")
# Try to reassign field (should fail with frozen)
try:
instance.status_colors = {"new": "#ffffff"}
print(f"✓ Field reassignment succeeded")
except Exception as e:
print(f"✗ Field reassignment blocked: {type(e).__name__}: {e}")
# Test 2: MappingProxyType
print("\n=== Test 2: MappingProxyType solution ===")
from types import MappingProxyType
class TestModel2(BaseModel):
model_config = ConfigDict(frozen=True)
status_colors: Mapping[str, str]
proxy = MappingProxyType({"healthy": "#22c55e", "warning": "#eab308"})
instance2 = TestModel2(status_colors=proxy)
print(f"With MappingProxyType: {instance2.status_colors}")
print(f"Type: {type(instance2.status_colors)}")
try:
instance2.status_colors["healthy"] = "#000000"
print(f"✓ MUTABLE: Mutation succeeded")
except TypeError as e:
print(f"✗ IMMUTABLE: {e}")
# Test 3: Serialization with MappingProxyType
print("\n=== Test 3: Serialization compatibility ===")
try:
dumped = instance2.model_dump()
print(f"model_dump() works: {dumped}")
except Exception as e:
print(f"model_dump() failed: {type(e).__name__}: {e}")
try:
json_str = instance2.model_dump_json()
print(f"model_dump_json() works: {json_str}")
except Exception as e:
print(f"model_dump_json() failed: {type(e).__name__}: {e}")
# Test 4: Check if field_validator receives dict or MappingProxyType
print("\n=== Test 4: Validator behavior ===")
class TestModel3(BaseModel):
model_config = ConfigDict(frozen=True)
status_colors: Mapping[str, str] = Field(
default_factory=lambda: {"healthy": "#22c55e"}
)
@staticmethod
def validate_colors_wrapper(cls, v):
print(f" Validator receives type: {type(v)}")
return v
# Create instance - let's see what type the default_factory produces
instance3 = TestModel3()
print(f"After construction, type is: {type(instance3.status_colors)}")
EOFRepository: OmniNode-ai/omnibase_core
Length of output: 155
🏁 Script executed:
cd /tmp && python3 << 'EOF'
# Direct test simulating the exact scenario from the file
from typing import Mapping
from pydantic import BaseModel, ConfigDict, Field
from types import MappingProxyType
print("Testing Pydantic v2.12.5 frozen behavior:\n")
# Test Case 1: Current implementation (dict behind Mapping)
class ModelCurrent(BaseModel):
model_config = ConfigDict(frozen=True, extra="forbid", from_attributes=True)
status_colors: Mapping[str, str] = Field(
default_factory=lambda: {
"healthy": "#22c55e",
"warning": "#eab308",
"error": "#ef4444",
"unknown": "#6b7280",
}
)
config = ModelCurrent()
print("1. CURRENT IMPLEMENTATION:")
print(f" Type of status_colors: {type(config.status_colors)}")
print(f" Value: {config.status_colors}\n")
try:
config.status_colors["healthy"] = "#000000"
print(f" ⚠️ VULNERABILITY: Dict is MUTABLE despite frozen=True")
print(f" Modified value: {config.status_colors}\n")
except TypeError as e:
print(f" ✓ SAFE: Mutation prevented - {e}\n")
# Test Case 2: With MappingProxyType
class ModelProposed(BaseModel):
model_config = ConfigDict(frozen=True, extra="forbid", from_attributes=True)
status_colors: Mapping[str, str] = Field(default_factory=lambda: MappingProxyType({
"healthy": "#22c55e",
"warning": "#eab308",
"error": "#ef4444",
"unknown": "#6b7280",
}))
config2 = ModelProposed()
print("2. PROPOSED FIX (MappingProxyType in default_factory):")
print(f" Type of status_colors: {type(config2.status_colors)}")
print(f" Value: {config2.status_colors}\n")
try:
config2.status_colors["healthy"] = "#000000"
print(f" ⚠️ Mutation succeeded")
except TypeError as e:
print(f" ✓ SAFE: Mutation prevented - {e}\n")
# Test Case 3: Serialization check
print("3. SERIALIZATION TEST:")
try:
dump = config2.model_dump()
print(f" model_dump() result type: {type(dump['status_colors'])}")
print(f" Serialization works: ✓\n")
except Exception as e:
print(f" Serialization failed: {e}\n")
EOFRepository: OmniNode-ai/omnibase_core
Length of output: 155
Make status_colors deeply immutable by wrapping the dict in MappingProxyType.
Pydantic's frozen=True prevents field reassignment, not mutation of mutable field values. Currently, the dict returned by default_factory can still be modified in-place: config.status_colors["key"] = value succeeds despite the model being frozen. Use MappingProxyType to create a read-only view.
The field validator is the right place to wrap the mapping:
Suggested fix
import re
from collections.abc import Mapping
+from types import MappingProxyType
from typing import Literal, Self
@field_validator("status_colors")
@classmethod
def validate_status_colors(cls, v: Mapping[str, str]) -> Mapping[str, str]:
"""Validate that all color values are valid hex color codes."""
- for status, color in v.items():
+ d = dict(v)
+ for status, color in d.items():
if not _HEX_COLOR_PATTERN.match(color):
raise ValueError(
f"Invalid hex color format for status '{status}': {color}. "
"Expected #RGB, #RRGGBB, #RGBA, or #RRGGBBAA"
)
- return v
+ return MappingProxyType(d)🤖 Prompt for AI Agents
In @src/omnibase_core/models/dashboard/model_widget_config_status_grid.py around
lines 90 - 125, The status_colors dict is mutable despite model-level frozen;
update the validate_status_colors classmethod to wrap the incoming mapping in a
MappingProxyType before returning it so the stored value is an immutable
read-only view; ensure you import MappingProxyType and accept any
Mapping[str,str] (or dict) input, validate each color with _HEX_COLOR_PATTERN as
currently done, then return MappingProxyType(validated_mapping) instead of the
original mapping.
- Use identity comparison (is/is not) for enum singletons across all widget configs - Wrap status_colors dict in MappingProxyType for deep immutability - Rename format fields to avoid shadowing Python built-ins (with aliases for JSON compat) - Add min_length=1 constraint to optional icon fields - Fix brittle test assertions to check error types instead of message strings
PR Review: Dashboard Models and Enums (OMN-1284)🎯 SummaryThis PR adds comprehensive dashboard infrastructure models to omnibase_core. Overall, this is high-quality work with excellent documentation, thorough testing, and solid adherence to ONEX conventions. The discriminated union pattern for widget configs is well-implemented. ✅ Strengths1. Architecture & Design
2. Documentation Quality
3. Testing Coverage
4. Conventions Adherence
🔍 Issues & Concerns1. CRITICAL: Missing EnumDashboardTheme Import
|
| Metric | Value | Assessment |
|---|---|---|
| Files Added | 27 | Appropriate for scope |
| Additions | 3,633 | High, but mostly tests & docs |
| Deletions | 1 | Clean, minimal disruption |
| Test Coverage | 98 tests | Excellent |
| Mypy Status | ✅ Passing | Type-safe |
| Docs Quality | Comprehensive | Outstanding |
✅ Recommendations
Must Fix (Blocking)
- Add
EnumDashboardThemetoenums/__init__.pyexports - This will cause import failures
Should Fix (High Priority)
- Verify theme field type consistency - Ensure it uses
EnumDashboardTheme, notLiteral - Document
ModelNodeView.node_idstring rationale - Or considerUUID | strif truly variable
Nice to Have (Low Priority)
- Add length constraints to
name,labelfields (max_length=255) - Add widget count limit to
ModelDashboardConfig.widgets(max_length=100) - Consider expanding
extra_configto support non-string values - Add architecture documentation (ADR, extension guide)
🏁 Final Verdict
Status: ✅ Approve with Required Changes
This is high-quality work that demonstrates excellent engineering practices. The models are well-designed, thoroughly tested, and properly documented.
However, the missing EnumDashboardTheme export is a critical issue that will break imports. Once that's fixed, this PR is ready to merge.
The suggested improvements are non-blocking and can be addressed in follow-up PRs.
Reviewed by: Claude Code
Review Date: 2026-01-11
ONEX Version: 0.6.3
Great work on this PR! The attention to detail in testing and documentation is exemplary. 🎉
… checks (OMN-1284) Address PR #363 review feedback: test assertions should check error types via errors() instead of matching on Pydantic's internal message strings. Changes: - test_model_metric_threshold.py: Fix hex color validation test - test_model_validations.py: Fix 16 validation tests (axis range, colors, widget type) - test_model_widget_definition.py: Fix chart series and trend_key validation tests
|
Code Review: Dashboard Models and Enums PR 363 - Overall Assessment: Excellent work! This PR adds a comprehensive dashboard model layer with high quality code, 98 unit tests, excellent documentation, sound architecture with discriminated unions and frozen models, and full ONEX compliance. - Strengths: (1) Expert discriminated union pattern for type-safe widget config (2) Comprehensive validation with custom validators for min/max, hex colors, conditional fields (3) Exemplary test suite with 548 assertions covering edge cases, parameterized tests, roundtrip serialization (4) Complete documentation with examples and cross-references (5) Full ONEX convention compliance - Security and Performance: No security concerns with input validation on all fields, hex pattern preventing injection, frozen models. Good performance via O(1) discriminated union lookup, hash caching, memory-efficient tuples. - Test Coverage 10/10: Covers enums (141 assertions), model validation (548 lines), widget definition (403 lines), dashboard config (210 lines), view models (303 lines). All edge cases covered. - Recommendation: APPROVE - Production-ready with excellent quality. Ensure CI passes before merging. |
|
Detailed Review Highlights: DISCRIMINATED UNION PATTERN: VALIDATION COVERAGE:
TEST QUALITY EXAMPLES:
DESIGN DECISIONS (All Correct):
MINOR OBSERVATIONS (Intentional, No Action):
All 32 files follow naming conventions, have complete type hints, comprehensive docs, and pass strict mypy. Test coverage exceeds 60% requirement significantly. |
|
Future Enhancement Ideas (NOT for this PR):
These are ideas for future iterations - the current implementation is complete and production-ready as-is. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/unit/models/dashboard/test_model_validations.py:
- Around line 166-249: The test test_error_message_includes_status_key should be
updated to check the validator error message for the status key instead of
expecting the error loc to include the key; when ModelWidgetConfigStatusGrid's
field validator raises ValueError on the Mapping, Pydantic sets the loc to
("status_colors",) and embeds the specific key (e.g., "fail") in the error
message, so modify the assertion to assert any error where e["type"] ==
"value_error" and the key string (like "fail") appears in e["msg"] for the
errors returned by exc_info.value.errors().
In @tests/unit/models/dashboard/test_model_widget_definition.py:
- Around line 185-257: Remove the unused "description" parameter from the
pytest.mark.parametrize call in test_layout_constraints and update the test to
assert on the ValidationError location for specificity: parametrize only
("field", "value"), build kwargs as before, then use "with
pytest.raises(ValidationError) as exc:" when constructing
ModelWidgetDefinition(**kwargs) and assert that exc.value.errors()[0]["loc"] ==
(field,) to ensure the error is for the expected field (referencing
ModelWidgetDefinition and test_layout_constraints).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/unit/models/dashboard/test_model_metric_threshold.pytests/unit/models/dashboard/test_model_validations.pytests/unit/models/dashboard/test_model_widget_definition.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/dashboard/test_model_metric_threshold.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Use 'poetry run' for all Python commands (pytest, mypy, python -m, etc.). Never use pip or direct python execution.
Always use protocol names for service resolution (e.g., 'ProtocolEventBus', 'ProtocolLogger'), never concrete class names. Use duck typing without isinstance checks.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception. Include error context in the context parameter.
Use the @standard_error_handling decorator on async functions to eliminate 6+ lines of try/catch boilerplate in error handling.
Use PEP 604 union syntax (e.g., 'str | None') instead of legacy Optional/Union. Enforce via ruff rule UP007.
Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers.
Order exception tuples alphabetically by exception class name (e.g., except (AttributeError, TypeError, ValidationError) not except (ValueError, TypeError, AttributeError)).
Use centralized exception constants from omnibase_core.errors.exception_groups (PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS, FILE_IO_ERRORS, etc.) instead of manually listing exceptions.
Maintain 100% mypy strict mode compliance (0 errors) across all source files using type annotations. Both mypy and pyright must pass in CI.
Format Python code with Black: poetry run black src/ tests/. Sort imports with isort: poetry run isort src/ tests/. Lint with ruff: poetry run ruff check src/ tests/
Do NOT use 'from future import annotations' when runtime type introspection is needed (Pydantic models, FastAPI endpoints). Use it when forward references are needed or to prevent circular imports.
Use EnumNodeKind for architectural role (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) for pipeline routing. Use EnumNodeType for implementation type (COMPUTE_GENERIC, TRA...
Files:
tests/unit/models/dashboard/test_model_widget_definition.pytests/unit/models/dashboard/test_model_validations.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% code coverage across test suite. Run tests with: poetry run pytest tests/ --cov=src/omnibase_core/
Run all tests with parallel workers: poetry run pytest tests/ uses 4 workers locally with 60s timeout and loadscope distribution. CI uses 20 parallel splits on GitHub Actions.
Use pytest markers for test categorization: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
Use pytest 8.4+ for testing. Use pytest fixtures, markers, and parallel execution with pytest-xdist.
Files:
tests/unit/models/dashboard/test_model_widget_definition.pytests/unit/models/dashboard/test_model_validations.py
🧠 Learnings (2)
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to tests/**/*.py : Use pytest markers for test categorization: pytest.mark.unit, pytest.mark.integration, pytest.mark.slow, pytest.mark.smoke, pytest.mark.performance
Applied to files:
tests/unit/models/dashboard/test_model_widget_definition.py
📚 Learning: 2026-01-08T14:36:08.342Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T14:36:08.342Z
Learning: Applies to tests/**/*.py : Use pytest 8.4+ for testing. Use pytest fixtures, markers, and parallel execution with pytest-xdist.
Applied to files:
tests/unit/models/dashboard/test_model_widget_definition.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: claude-review
🔇 Additional comments (4)
tests/unit/models/dashboard/test_model_widget_definition.py (3)
23-34: Helper config factories are clear and appropriately minimal.
Nice separation between pie/no-series vs line/series-required setup.
36-184: Good coverage for discriminated-union behavior + roundtrip serialization.
The dict-basedmodel_validatecases across all config kinds are especially useful coverage for UI payloads.
347-403: Validator tests (series-required + trend_key-required) look good and match the stated review feedback.
Usingerrors()instead of message-string matching is a good move.tests/unit/models/dashboard/test_model_validations.py (1)
23-548: Solid validation coverage and good consistency with pytest unit markers.
Theerrors()-based assertions for string min-length and field locations are in the right direction (per the PR objective and testing guidelines).
| @pytest.mark.unit | ||
| class TestModelWidgetConfigStatusGridColorValidation: | ||
| """Tests for ModelWidgetConfigStatusGrid status_colors validation.""" | ||
|
|
||
| def test_default_colors_valid(self) -> None: | ||
| """Test that default status_colors are valid.""" | ||
| config = ModelWidgetConfigStatusGrid() | ||
| assert "healthy" in config.status_colors | ||
| assert config.status_colors["healthy"] == "#22c55e" | ||
|
|
||
| def test_custom_valid_colors(self) -> None: | ||
| """Test custom valid hex colors.""" | ||
| custom_colors = { | ||
| "ok": "#00FF00", | ||
| "warn": "#FFFF00", | ||
| "fail": "#FF0000", | ||
| } | ||
| config = ModelWidgetConfigStatusGrid(status_colors=custom_colors) | ||
| assert config.status_colors == custom_colors | ||
|
|
||
| def test_custom_valid_colors_3_digit(self) -> None: | ||
| """Test custom valid 3-digit hex colors.""" | ||
| custom_colors = { | ||
| "ok": "#0F0", | ||
| "warn": "#FF0", | ||
| "fail": "#F00", | ||
| } | ||
| config = ModelWidgetConfigStatusGrid(status_colors=custom_colors) | ||
| assert config.status_colors == custom_colors | ||
|
|
||
| def test_custom_valid_colors_with_alpha(self) -> None: | ||
| """Test custom valid hex colors with alpha.""" | ||
| custom_colors = { | ||
| "ok": "#00FF00FF", | ||
| "warn": "#FFFF00AA", | ||
| } | ||
| config = ModelWidgetConfigStatusGrid(status_colors=custom_colors) | ||
| assert config.status_colors == custom_colors | ||
|
|
||
| def test_invalid_color_missing_hash(self) -> None: | ||
| """Test that color without # is invalid.""" | ||
| with pytest.raises(ValidationError) as exc_info: | ||
| ModelWidgetConfigStatusGrid( | ||
| status_colors={"ok": "00FF00"} # Missing # | ||
| ) | ||
| # Check for value_error type from our custom color validator | ||
| errors = exc_info.value.errors() | ||
| assert any(e["type"] == "value_error" for e in errors) | ||
|
|
||
| def test_invalid_color_wrong_length(self) -> None: | ||
| """Test that color with wrong length is invalid.""" | ||
| with pytest.raises(ValidationError) as exc_info: | ||
| ModelWidgetConfigStatusGrid( | ||
| status_colors={"ok": "#FF000"} # 5 digits - invalid length | ||
| ) | ||
| # Check for value_error type from our custom color validator | ||
| errors = exc_info.value.errors() | ||
| assert any(e["type"] == "value_error" for e in errors) | ||
|
|
||
| def test_invalid_color_non_hex_chars(self) -> None: | ||
| """Test that non-hex characters are invalid.""" | ||
| with pytest.raises(ValidationError) as exc_info: | ||
| ModelWidgetConfigStatusGrid( | ||
| status_colors={"ok": "#GGGGGG"} # Invalid chars | ||
| ) | ||
| # Check for value_error type from our custom color validator | ||
| errors = exc_info.value.errors() | ||
| assert any(e["type"] == "value_error" for e in errors) | ||
|
|
||
| def test_error_message_includes_status_key(self) -> None: | ||
| """Test that error message includes the status key.""" | ||
| with pytest.raises(ValidationError) as exc_info: | ||
| ModelWidgetConfigStatusGrid( | ||
| status_colors={"ok": "#00FF00", "fail": "invalid"} | ||
| ) | ||
| # Check for value_error type from our custom color validator | ||
| errors = exc_info.value.errors() | ||
| assert any(e["type"] == "value_error" for e in errors) | ||
|
|
||
| def test_empty_dict_valid(self) -> None: | ||
| """Test that empty status_colors dict is valid.""" | ||
| config = ModelWidgetConfigStatusGrid(status_colors={}) | ||
| assert config.status_colors == {} | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "test_model_validations.py" -type f 2>/dev/null | head -5Repository: OmniNode-ai/omnibase_core
Length of output: 51
🏁 Script executed:
fd "test_model_validations.py" --type fRepository: OmniNode-ai/omnibase_core
Length of output: 122
🏁 Script executed:
cat -n tests/unit/models/dashboard/test_model_validations.py | head -100Repository: OmniNode-ai/omnibase_core
Length of output: 4837
🏁 Script executed:
cat -n tests/unit/models/dashboard/test_model_validations.py | sed -n '230,244p'Repository: OmniNode-ai/omnibase_core
Length of output: 839
🏁 Script executed:
# Check if the file exists and get more context around the test
wc -l tests/unit/models/dashboard/test_model_validations.pyRepository: OmniNode-ai/omnibase_core
Length of output: 126
🏁 Script executed:
# Look for the ModelWidgetConfigStatusGrid model
fd "\.py" --type f | xargs grep -l "class ModelWidgetConfigStatusGrid" | head -3Repository: OmniNode-ai/omnibase_core
Length of output: 138
🏁 Script executed:
# Search for the model definition and its validator
rg "class ModelWidgetConfigStatusGrid" -A 30Repository: OmniNode-ai/omnibase_core
Length of output: 3635
🏁 Script executed:
# Check the status_colors field and its validator
rg "status_colors" -B 2 -A 2Repository: OmniNode-ai/omnibase_core
Length of output: 9493
🏁 Script executed:
cat -n src/omnibase_core/models/dashboard/model_widget_config_status_grid.py | sed -n '1,120p'Repository: OmniNode-ai/omnibase_core
Length of output: 5356
🏁 Script executed:
# Get the validator implementation
rg "validate_status_colors" -A 20Repository: OmniNode-ai/omnibase_core
Length of output: 2441
🏁 Script executed:
# Check the full model structure
cat -n src/omnibase_core/models/dashboard/model_widget_config_status_grid.py | tail -80Repository: OmniNode-ai/omnibase_core
Length of output: 3807
🏁 Script executed:
python3 << 'EOF'
from pydantic import BaseModel, field_validator, ValidationError
from collections.abc import Mapping
import re
# Recreate the validator scenario
_HEX_COLOR_PATTERN = re.compile(
r"^#([0-9a-fA-F]{3}|[0-9a-fA-F]{6}|[0-9a-fA-F]{4}|[0-9a-fA-F]{8})$"
)
class TestModel(BaseModel):
status_colors: Mapping[str, str]
@field_validator("status_colors")
@classmethod
def validate_status_colors(cls, v: Mapping[str, str]) -> Mapping[str, str]:
"""Validate that all color values are valid hex color codes."""
for status, color in v.items():
if not _HEX_COLOR_PATTERN.match(color):
raise ValueError(
f"Invalid hex color format for status '{status}': {color}. "
"Expected #RGB, #RRGGBB, #RRGGBB, or #RRGGBBAA"
)
return v
# Test what the error structure looks like
try:
TestModel(status_colors={"ok": "#00FF00", "fail": "invalid"})
except ValidationError as e:
errors = e.errors()
print("Number of errors:", len(errors))
for idx, error in enumerate(errors):
print(f"\nError {idx}:")
print(f" type: {error.get('type')}")
print(f" loc: {error.get('loc')}")
print(f" msg: {error.get('msg')}")
print(f" Full error dict: {error}")
EOFRepository: OmniNode-ai/omnibase_core
Length of output: 155
test_error_message_includes_status_key proposed assertion is incompatible with Pydantic's @field_validator behavior on Mapping fields.
When @field_validator raises ValueError on a dict/Mapping field, Pydantic v2 sets error loc to ("status_colors",) (field name only), not ("status_colors", "fail"). The status key information is included in the error message string (per the validator at line 124), not the loc tuple.
The proposed assertion e["loc"] == ("status_colors", "fail") would never match. To verify the status key is surfaced, assert the message contains it instead:
def test_error_message_includes_status_key(self) -> None:
"""Test that error message includes the status key."""
with pytest.raises(ValidationError) as exc_info:
ModelWidgetConfigStatusGrid(
status_colors={"ok": "#00FF00", "fail": "invalid"}
)
errors = exc_info.value.errors()
assert any(
e["type"] == "value_error" and "fail" in e["msg"]
for e in errors
)🤖 Prompt for AI Agents
In @tests/unit/models/dashboard/test_model_validations.py around lines 166 -
249, The test test_error_message_includes_status_key should be updated to check
the validator error message for the status key instead of expecting the error
loc to include the key; when ModelWidgetConfigStatusGrid's field validator
raises ValueError on the Mapping, Pydantic sets the loc to ("status_colors",)
and embeds the specific key (e.g., "fail") in the error message, so modify the
assertion to assert any error where e["type"] == "value_error" and the key
string (like "fail") appears in e["msg"] for the errors returned by
exc_info.value.errors().
…ns (OMN-1284) - Rename test_error_message_includes_status_key to test_invalid_color_value_raises_value_error to accurately reflect what is being tested (error type, not message text) - Remove unused description parameter from test_layout_constraints parameterization - Add explicit errors() assertions to verify correct field location in validation errors
PR Review: Dashboard Models and Enums (OMN-1284)Overall Assessment: ✅ ExcellentThis is a high-quality, well-architected PR that adds comprehensive dashboard models to omnibase_core. The implementation demonstrates strong adherence to ONEX conventions and Pydantic best practices. ✅ Strengths1. Exemplary Code Quality
2. Architecture & Design
3. ONEX Convention Compliance
4. Testing Excellence
5. Iterative ImprovementThe 7-commit sequence shows excellent response to review feedback:
💡 Minor Suggestions (Non-Blocking)1. ModelNodeView String IDs
node_id: str = Field( # string-id-ok: UI display identifierObservation: Uses string IDs instead of UUIDs for flexibility in UI contexts. Consideration: While the docstring references Verdict: Not a blocker - the current pattern is acceptable for view models. 2. EnumDashboardTheme Default
Observation: Good choice for default ( Enhancement: Consider documenting in 3. Widget Position Constraints
Observation: Width is constrained to Enhancement: Consider a model validator to ensure 4. Color Validation PatternMultiple files use Suggestion: Extract to a shared constant or validation utility in 🔒 Security Considerations✅ No Issues Found
🎯 Performance Considerations✅ Efficient Design
📊 Test Coverage AnalysisCoverage by Category:
Missing: Integration tests with actual dashboard rendering (out of scope for omnibase_core). 📁 File Organization✅ Excellent Structure
🐛 Potential Bugs: None FoundAll validation tests pass, mypy strict mode passes, and edge cases are handled. ✅ Best Practices Compliance
🚀 Recommendation: APPROVE ✅This PR is ready to merge pending CI completion. The code demonstrates:
Linear Issue: OMN-1284 Final NotesThis PR sets a gold standard for model layer contributions to omnibase_core. The discriminated union pattern, comprehensive validation, and test coverage make this code production-ready. Great work, @jonahgabriel! 🎉 |
Aligns ModelNodeView with the canonical architecture pattern where node_id is UUID (131 occurrences) rather than str (18 occurrences). Matches sister model ModelCapabilityView and NodeCoreBase.
Code Review - PR #363: Dashboard Models and EnumsThank you for this well-structured contribution! This PR adds comprehensive dashboard model infrastructure to omnibase_core. Here's my detailed review: ✅ Strengths1. Excellent Architecture & Design
2. Comprehensive Validation
3. Outstanding Documentation
4. Test Coverage
5. Code Quality
🔍 Issues & SuggestionsCritical Issues
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/models/dashboard/test_model_node_view.py (1)
117-154: LGTM!Good coverage of the utility methods with both positive cases and fallback behavior. The use of fixed UUIDs for
get_qualified_idtests ensures deterministic assertions.Optional: Consider adding a test for
display_name=""to verify theorfallback behavior handles empty strings (currently only testsNonefallback).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/omnibase_core/models/dashboard/model_node_view.pytests/unit/models/dashboard/test_model_node_view.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_core/models/dashboard/model_node_view.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Use 'poetry run' for all Python commands (pytest, mypy, python -m, etc.). Never use pip or direct python execution.
Always use protocol names for service resolution (e.g., 'ProtocolEventBus', 'ProtocolLogger'), never concrete class names. Use duck typing without isinstance checks.
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception. Include error context in the context parameter.
Use the @standard_error_handling decorator on async functions to eliminate 6+ lines of try/catch boilerplate in error handling.
Use PEP 604 union syntax (e.g., 'str | None') instead of legacy Optional/Union. Enforce via ruff rule UP007.
Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers.
Order exception tuples alphabetically by exception class name (e.g., except (AttributeError, TypeError, ValidationError) not except (ValueError, TypeError, AttributeError)).
Use centralized exception constants from omnibase_core.errors.exception_groups (PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS, FILE_IO_ERRORS, etc.) instead of manually listing exceptions.
Maintain 100% mypy strict mode compliance (0 errors) across all source files using type annotations. Both mypy and pyright must pass in CI.
Format Python code with Black: poetry run black src/ tests/. Sort imports with isort: poetry run isort src/ tests/. Lint with ruff: poetry run ruff check src/ tests/
Do NOT use 'from future import annotations' when runtime type introspection is needed (Pydantic models, FastAPI endpoints). Use it when forward references are needed or to prevent circular imports.
Use EnumNodeKind for architectural role (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) for pipeline routing. Use EnumNodeType for implementation type (COMPUTE_GENERIC, TRA...
Files:
tests/unit/models/dashboard/test_model_node_view.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% code coverage across test suite. Run tests with: poetry run pytest tests/ --cov=src/omnibase_core/
Run all tests with parallel workers: poetry run pytest tests/ uses 4 workers locally with 60s timeout and loadscope distribution. CI uses 20 parallel splits on GitHub Actions.
Use pytest markers for test categorization: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance
Use pytest 8.4+ for testing. Use pytest fixtures, markers, and parallel execution with pytest-xdist.
Files:
tests/unit/models/dashboard/test_model_node_view.py
🧬 Code graph analysis (1)
tests/unit/models/dashboard/test_model_node_view.py (1)
src/omnibase_core/models/dashboard/model_node_view.py (3)
ModelNodeView(13-119)get_display_name(102-108)get_qualified_id(110-119)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code Quality
- GitHub Check: claude-review
🔇 Additional comments (5)
tests/unit/models/dashboard/test_model_node_view.py (5)
1-16: LGTM!Clean file structure with appropriate SPDX headers, minimal imports, and proper use of
@pytest.mark.unitmarker as per coding guidelines.
18-66: LGTM!Comprehensive coverage of minimal creation, default values, and full field creation. The tests correctly validate both required fields (
node_id,name) and all optional fields with their expected defaults.
68-99: LGTM!Solid serialization roundtrip tests and defensive frozen model validation. The dual exception handling (
ValidationError, TypeError) in the frozen test is appropriate given Pydantic v2's behavior variations.
101-115: LGTM!Proper validation of error conditions for invalid UUID and empty name, matching the model's constraints.
156-205: LGTM!Excellent coverage of edge cases including inactive/unhealthy states, forbidden extra fields, missing required fields, and UUID type coercion. The tests thoroughly validate the model's
ConfigDict(frozen=True, extra="forbid")configuration and Pydantic's UUID handling.
…OMN-1284) - Enhanced requires_reconnection docstring in EnumDashboardStatus to clarify recoverable vs non-recoverable ERROR states - Added mutual exclusivity documentation to is_data_bound and is_aggregation properties in EnumWidgetType with Sphinx cross-references - Added widget width validation in ModelDashboardConfig to ensure widget widths do not exceed dashboard grid columns - Documented refresh_interval_seconds 1-second minimum rationale (dev/testing flexibility vs production 5-30s recommendation) - Added 3 new tests for widget width validation
Code Review - PR #363: Dashboard Models and EnumsOverall Assessment: ✅ Excellent This is a well-designed, thoroughly tested addition to omnibase_core. The dashboard models follow ONEX conventions precisely and demonstrate strong architectural design. I found no critical issues - only minor suggestions for potential enhancements. ✅ Strengths1. Architecture & Design
2. Code Quality
3. Test Coverage
4. ONEX Compliance
💡 Suggestions (Non-Blocking)1. Hex Color Validation - DRY OpportunityHex color validation is duplicated across 3 files. Consider extracting to a shared validator in omnibase_core.validation.validators. Priority: Low 2. Refresh Interval ValidationConsider adding a warning validator for sub-5s intervals in production to prevent accidental DDoS. Priority: Low 3. EnumDashboardStatus.requires_reconnectionConsider splitting ERROR state or adding a separate requires_manual_intervention property for clearer error recovery semantics. Priority: Low Reviews✅ Security: No concerns identified ✅ Compliance with CLAUDE.mdAll requirements pass:
🎯 Final Recommendation✅ APPROVE - Merge immediately after CI passes This PR represents exemplary code quality with zero critical issues. Great work! 🎉 Reviewed by: Claude Sonnet 4.5 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/omnibase_core/models/dashboard/model_dashboard_config.py (1)
133-140: Consider including widget title in validation error for better UX.The validation logic is correct, but the error message uses
widget.widget_id(a UUID), which can be hard to read. Including the widget title would make errors more actionable.♻️ Suggested improvement
if widget.width > max_cols: raise ValueError( - f"Widget '{widget.widget_id}' width ({widget.width}) exceeds " + f"Widget '{widget.title}' (id: {widget.widget_id}) width ({widget.width}) exceeds " f"dashboard grid columns ({max_cols})" )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/omnibase_core/enums/enum_dashboard_status.pysrc/omnibase_core/enums/enum_widget_type.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pytests/unit/models/dashboard/test_model_dashboard_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_core/enums/enum_widget_type.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution (poetry run pytest, poetry add, poetry install) - never use pip or python directly
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers to document intent
Order exception tuples alphabetically by exception class name when catching multiple exceptions
Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Use PEP 604 union syntax (str | None) instead of legacy Union/Optional syntax - enforced by ruff UP007
Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Maintain minimum 60% test coverage requirement across the codebase measured with pytest-cov
Files:
src/omnibase_core/enums/enum_dashboard_status.pytests/unit/models/dashboard/test_model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_dashboard_config.py
**/src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/omnibase_core/**/*.py: Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_, container_, decorator_, enum_, error_, factory_, mixin_, model_, node_, runtime_, service_*, etc.
Ensure 100% mypy strict mode compliance (0 errors) and pyright basic mode compliance across all source files in src/omnibase_core
Files:
src/omnibase_core/enums/enum_dashboard_status.pysrc/omnibase_core/models/dashboard/model_dashboard_config.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance) for test categorization
Files:
tests/unit/models/dashboard/test_model_dashboard_config.py
🧬 Code graph analysis (1)
src/omnibase_core/models/dashboard/model_dashboard_config.py (4)
src/omnibase_core/enums/enum_dashboard_status.py (1)
EnumDashboardStatus(27-114)src/omnibase_core/enums/enum_dashboard_theme.py (1)
EnumDashboardTheme(25-63)src/omnibase_core/models/dashboard/model_dashboard_layout_config.py (1)
ModelDashboardLayoutConfig(28-66)src/omnibase_core/models/dashboard/model_widget_definition.py (1)
ModelWidgetDefinition(90-150)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: claude-review
🔇 Additional comments (3)
src/omnibase_core/enums/enum_dashboard_status.py (1)
27-114: Excellent enum implementation with comprehensive documentation.The enum definition and properties are well-designed:
- Clear lifecycle states with appropriate string values
- Properties use
iscomparison correctly for enum members- The
requires_reconnectiondocumentation thoughtfully distinguishes transient vs. fatal errors- Cross-references to related properties enhance usability
src/omnibase_core/models/dashboard/model_dashboard_config.py (1)
46-117: Well-designed immutable configuration model.The model structure is solid:
- Frozen model with strict validation (extra="forbid") prevents configuration drift
- Appropriate use of default_factory for mutable defaults
- The 1-second minimum for refresh_interval_seconds is well-justified with clear production guidance
- Field constraints and descriptions are comprehensive
tests/unit/models/dashboard/test_model_dashboard_config.py (1)
34-277: Comprehensive test suite with excellent edge case coverage.The tests thoroughly validate:
- Default values and custom configurations
- Serialization round-trips preserving discriminated unions
- Immutability behavior with defensive error handling
- Validation constraints (UUID types, refresh intervals, widget widths)
- All enum values for theme and status
- Boundary conditions for widget width validation (at limit, exceeds, multiple widgets)
The three widget width validation tests (lines 212-277) provide good coverage of the validator's behavior.
…d enhancements (OMN-1284) DRY Refactoring: - Extract hex color validation to shared validator_hex_color.py - Refactor model_metric_threshold.py, model_chart_series_config.py, and model_widget_config_status_grid.py to use shared validator - Add 85 comprehensive tests for hex color validation Dashboard Enhancements: - Add warn_aggressive_refresh validator to emit UserWarning for sub-5s refresh intervals (prevents accidental DDoS in production) - Add requires_manual_intervention property to EnumDashboardStatus for clearer error recovery semantics (ERROR state only) - Add 12 new tests for warning and property behavior
Code Review: Dashboard Models and Enums (OMN-1284)SummaryThis is a high-quality PR that adds comprehensive dashboard infrastructure to omnibase_core. The implementation demonstrates excellent adherence to ONEX architectural patterns, strong type safety, and thorough testing coverage. Overall Assessment: ✅ APPROVED with minor suggestions 🎯 Strengths1. Excellent Architecture & Design
2. Strong Type Safety
3. Comprehensive Testing (98 tests!)
4. Documentation Excellence
5. Code Quality
🔍 Detailed FindingsCode Quality Issues: None Found
Potential Issues: Minor1. Aggressive Refresh Warning (src/omnibase_core/models/dashboard/model_dashboard_config.py:150-168)The
2. Thread Safety Documentation (throughout)While the models use
3. Hex Color Validation Performance (src/omnibase_core/validation/validator_hex_color.py)The regex pattern is pre-compiled (excellent!), but:
Performance Considerations✅ Good Patterns
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/unit/models/dashboard/test_model_dashboard_config.py:
- Around line 146-155: The test_frozen_model currently expects either
ValidationError or TypeError when mutating a frozen ModelDashboardConfig
instance; update the exception expectation to only ValidationError to match
project-wide tests. In the test_frozen_model function, change the pytest.raises
context to expect ValidationError for the mutation of the dashboard (instance of
ModelDashboardConfig, variable name dashboard) so the assertion aligns with
Pydantic v2 behavior used elsewhere.
In @tests/unit/validation/test_validator_hex_color.py:
- Around line 448-459: The test has an outdated comment and imports directly
from the module; update test_import_from_validation_package to import the
validators from the package instead (use "from omnibase_core.validation import
validate_hex_color, validate_hex_color_mapping, validate_hex_color_optional")
and remove or replace the obsolete comment "This test will be updated once we
add exports to __init__.py" so the test reflects the current exports in
omnibase_core.validation; keep the assertions as-is to verify callability.
🧹 Nitpick comments (1)
tests/unit/models/dashboard/test_model_dashboard_config.py (1)
280-349: Consider parameterizing the refresh interval warning tests.The warning tests are thorough and correct, but the pattern is repetitive. Consider using
@pytest.mark.parametrizeto improve maintainability while preserving clarity.♻️ Optional refactoring to reduce duplication
+ @pytest.mark.parametrize( + "interval,should_warn", + [ + (1, True), # minimum + (2, True), # low + (4, True), # boundary below threshold + (5, False), # at threshold + (10, False), # production + (30, False), # production + (60, False), # production + ], + ) + def test_refresh_interval_warnings(self, interval: int, should_warn: bool) -> None: + """Test that refresh intervals below 5s emit warnings.""" + if should_warn: + with pytest.warns(UserWarning, match="below recommended minimum"): + dashboard = ModelDashboardConfig( + dashboard_id=uuid4(), + name="Test", + refresh_interval_seconds=interval, + ) + else: + with warnings.catch_warnings(): + warnings.simplefilter("error") + dashboard = ModelDashboardConfig( + dashboard_id=uuid4(), + name="Test", + refresh_interval_seconds=interval, + ) + assert dashboard.refresh_interval_seconds == intervalThis consolidates the warning behavior while maintaining test clarity. You can keep the current separate tests if you prefer the explicit documentation of each scenario.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/omnibase_core/enums/enum_dashboard_status.pysrc/omnibase_core/models/dashboard/model_chart_series_config.pysrc/omnibase_core/models/dashboard/model_dashboard_config.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/validation/__init__.pysrc/omnibase_core/validation/validator_hex_color.pytests/unit/enums/test_enum_dashboard_status.pytests/unit/models/dashboard/test_model_dashboard_config.pytests/unit/validation/test_validator_hex_color.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/omnibase_core/models/dashboard/model_dashboard_config.py
- src/omnibase_core/models/dashboard/model_chart_series_config.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution (poetry run pytest, poetry add, poetry install) - never use pip or python directly
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers to document intent
Order exception tuples alphabetically by exception class name when catching multiple exceptions
Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Use PEP 604 union syntax (str | None) instead of legacy Union/Optional syntax - enforced by ruff UP007
Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Maintain minimum 60% test coverage requirement across the codebase measured with pytest-cov
Files:
src/omnibase_core/validation/__init__.pysrc/omnibase_core/validation/validator_hex_color.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pytests/unit/models/dashboard/test_model_dashboard_config.pytests/unit/enums/test_enum_dashboard_status.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/enums/enum_dashboard_status.pytests/unit/validation/test_validator_hex_color.py
**/src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/omnibase_core/**/*.py: Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_, container_, decorator_, enum_, error_, factory_, mixin_, model_, node_, runtime_, service_*, etc.
Ensure 100% mypy strict mode compliance (0 errors) and pyright basic mode compliance across all source files in src/omnibase_core
Files:
src/omnibase_core/validation/__init__.pysrc/omnibase_core/validation/validator_hex_color.pysrc/omnibase_core/models/dashboard/model_widget_config_status_grid.pysrc/omnibase_core/models/dashboard/model_metric_threshold.pysrc/omnibase_core/enums/enum_dashboard_status.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance) for test categorization
Files:
tests/unit/models/dashboard/test_model_dashboard_config.pytests/unit/enums/test_enum_dashboard_status.pytests/unit/validation/test_validator_hex_color.py
🧠 Learnings (1)
📚 Learning: 2026-01-11T18:12:47.296Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.296Z
Learning: Applies to **/models/model_*.py : Add from_attributes=True to ConfigDict for immutable value objects nested in other Pydantic models used with pytest-xdist parallel execution
Applied to files:
src/omnibase_core/models/dashboard/model_widget_config_status_grid.py
🧬 Code graph analysis (5)
src/omnibase_core/validation/__init__.py (1)
src/omnibase_core/validation/validator_hex_color.py (4)
HexColorValidator(59-157)validate_hex_color(163-186)validate_hex_color_mapping(215-242)validate_hex_color_optional(189-212)
src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (4)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(25-104)src/omnibase_core/models/dashboard/model_status_item_config.py (1)
ModelStatusItemConfig(11-25)src/omnibase_core/validation/validator_hex_color.py (1)
validate_hex_color_mapping(215-242)src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
validate_widget_type_config_kind_consistency(120-138)
tests/unit/enums/test_enum_dashboard_status.py (1)
src/omnibase_core/enums/enum_dashboard_status.py (5)
EnumDashboardStatus(27-155)is_operational(71-80)is_terminal(83-92)requires_reconnection(95-126)requires_manual_intervention(129-155)
src/omnibase_core/models/dashboard/model_metric_threshold.py (2)
src/omnibase_core/validation/validator_hex_color.py (1)
validate_hex_color(163-186)src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
validate_color(38-40)
tests/unit/validation/test_validator_hex_color.py (1)
src/omnibase_core/validation/validator_hex_color.py (7)
HexColorValidator(59-157)validate_hex_color(163-186)validate_hex_color_mapping(215-242)validate_hex_color_optional(189-212)validate(79-102)validate_optional(105-125)validate_mapping(128-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (11)
src/omnibase_core/validation/__init__.py (1)
174-182: LGTM!The hex color validator imports and exports are properly integrated into the validation package. The imports are correctly placed after related validators, and the
__all__entries are grouped with a clear comment referencing the ticket (OMN-1284). The export order matches the import order for consistency.Also applies to: 363-368
src/omnibase_core/enums/enum_dashboard_status.py (1)
1-155: LGTM!Well-designed enum with clear lifecycle states. The use of
(str, Enum)inheritance enables JSON serialization, and the identity comparisons (self is EnumDashboardStatus.X) are the correct approach for enum properties. The extensive docstrings provide excellent documentation for callers, particularly the nuanced explanation of whenrequires_reconnectionis appropriate for ERROR states.src/omnibase_core/models/dashboard/model_widget_config_status_grid.py (1)
51-139: LGTM!Solid implementation following established patterns from other widget configs. Key positives:
from_attributes=Truein ConfigDict for pytest-xdist compatibility (per learnings)MappingProxyTypefor immutable default status colors- Tuple type for
itemsmatches the frozen model pattern- Consistency validator pattern aligns with
ModelWidgetConfigChartsrc/omnibase_core/validation/validator_hex_color.py (2)
47-51: LGTM!The regex pattern correctly covers all standard web hex color formats (#RGB, #RRGGBB, #RGBA, #RRGGBBAA). Using
fullmatchensures the entire string must match, preventing partial matches. The pattern is pre-compiled at module level for performance and thread-safety.
59-157: LGTM!Clean design with good separation of concerns:
HexColorValidatorclass encapsulates the validation logic with class methods- Convenience functions provide a simpler API for Pydantic field validators
- The
# error-ok:comments correctly document intentional ValueError usage per Pydantic requirementsvalidate_mappingprovides helpful context in error messages with thekey_contextparameterAlso applies to: 163-242
tests/unit/enums/test_enum_dashboard_status.py (1)
13-170: LGTM!Comprehensive test suite with excellent coverage of the enum's behavior. Notable positives:
- All tests properly marked with
@pytest.mark.unitper coding guidelines- Tests cover all four enum values for each property
- Invariant tests (e.g.,
test_requires_manual_intervention_subset_of_reconnection) verify logical relationships between properties- JSON roundtrip test ensures serialization compatibility
- Edge cases like invalid value construction are covered
src/omnibase_core/models/dashboard/model_metric_threshold.py (1)
29-70: LGTM!Clean implementation following the established patterns. The model correctly:
- Uses
from_attributes=Truein ConfigDict for pytest-xdist compatibility (per learnings)- Applies the hex color validator to ensure valid color format
- Uses
str | Nonesyntax per PEP 604 (UP007) as required by coding guidelines- Follows the same validation pattern as
ModelChartSeriesConfigtests/unit/validation/test_validator_hex_color.py (1)
33-509: LGTM!Comprehensive test suite covering:
- Pattern matching for all valid/invalid color formats
- Error message content verification
- Pydantic integration with field validators
- Edge cases (unicode, whitespace, immutable mappings, thread safety)
- All test classes properly marked with
@pytest.mark.unittests/unit/models/dashboard/test_model_dashboard_config.py (3)
22-32: LGTM: Clean test helper functions.The factory functions improve test readability and reduce duplication effectively.
35-63: LGTM: Comprehensive layout config tests.The tests properly cover defaults, custom values, and serialization roundtrip with clear assertions.
69-145: LGTM: Solid coverage of core dashboard config functionality.The tests properly verify model creation, defaults, widget integration, and serialization with appropriate assertions.
…orts (OMN-1284) - Simplify frozen model mutation tests to catch only ValidationError (Pydantic v2 consistently raises ValidationError, not TypeError) - Update hex color validator test to use package-level imports - Remove outdated comment about pending exports (already added) - Add assertions for HexColorValidator and HEX_COLOR_PATTERN exports Addresses remaining PR #363 review feedback.
Code Review SummaryOverall Assessment: ✅ EXCELLENT - This PR demonstrates high-quality engineering with excellent adherence to project conventions. Ready to merge with minor recommendations for future consideration. 🌟 StrengthsArchitecture & Design
Code Quality
Testing
Conventions Adherence
💡 Recommendations (Optional - Future Enhancements)1. Refresh Interval Warning (model_dashboard_config.py:156-167)The
2. Widget Type Consistency ValidationAll widget configs validate that
3. Threshold Ordering
4. Color Validation PerformanceThe hex color validator recompiles error messages on every failure ( 🔍 Specific Code Reviewsrc/omnibase_core/enums/enum_dashboard_status.py✅ Excellent enum design with rich property methods. The Minor note: src/omnibase_core/models/dashboard/model_widget_definition.py✅ Perfect use of discriminated unions. The Question for future: The src/omnibase_core/models/dashboard/model_dashboard_config.py✅ Excellent validation chain:
Suggestion: Line 134 checks src/omnibase_core/validation/validator_hex_color.py✅ Outstanding shared validation module. Thread-safe pattern compilation, class-level validators, and convenience functions. This is a model for how to build reusable validators. Best practice highlight: The pre-compiled tests/unit/models/dashboard/test_model_validations.py✅ Comprehensive validation testing. Covers edge cases like:
Coverage: This test file alone has 548 lines - excellent investment in quality. 🛡️ Security & PerformanceSecurity✅ No security concerns identified:
Performance✅ No performance concerns:
📊 Test Coverage AnalysisTest Distribution:
Coverage Categories:
Meets project requirement: 60%+ coverage ✅ (appears to be very high coverage based on test count) 🎯 Final RecommendationsRequired: None ✅All code follows project conventions and passes quality checks. Suggested for Future PRs:
Action Items:
✅ ApprovalStatus: APPROVED ✅ This is exemplary work that demonstrates:
The dashboard models are production-ready and provide a solid foundation for the UI layer. Great job! 🎉 Linear: OMN-1284 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unit/models/dashboard/test_model_dashboard_config.py (2)
22-32: Helper functions duplicated across test files.These helper functions (
_make_pie_chart_configand_make_line_chart_config) are identical to those intest_model_widget_definition.py. While duplication is acceptable for test isolation, consider extracting them to a sharedconftest.pyfixture if more test files need them.
81-91: Inconsistent enum comparison style.Line 87 compares
dashboard.themeto the string"system", while line 88 correctly uses the enumEnumDashboardStatus.INITIALIZING. For consistency and clarity, use the enum value:- assert dashboard.theme == "system" + assert dashboard.theme == EnumDashboardTheme.SYSTEMtests/unit/validation/test_validator_hex_color.py (1)
56-82: Consider usingfullmatch()to match production code behavior.The production code in
validator_hex_color.pyusescls.PATTERN.fullmatch(value)for validation, but these tests use.match(). While this likely works correctly if the pattern includes a$anchor, usingfullmatch()would be more explicit and consistent with the source code.Suggested change
def test_valid_colors_match(self, color: str, description: str) -> None: """Test that valid hex colors match the pattern.""" - assert HEX_COLOR_PATTERN.match(color) is not None, f"Failed for {description}" + assert HEX_COLOR_PATTERN.fullmatch(color) is not None, f"Failed for {description}"def test_invalid_colors_no_match(self, color: str, description: str) -> None: """Test that invalid hex colors do not match the pattern.""" - assert HEX_COLOR_PATTERN.match(color) is None, ( + assert HEX_COLOR_PATTERN.fullmatch(color) is None, ( f"Should not match: {description}" )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/unit/models/dashboard/test_model_capability_view.pytests/unit/models/dashboard/test_model_dashboard_config.pytests/unit/models/dashboard/test_model_widget_definition.pytests/unit/validation/test_validator_hex_color.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/dashboard/test_model_capability_view.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution (poetry run pytest, poetry add, poetry install) - never use pip or python directly
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers to document intent
Order exception tuples alphabetically by exception class name when catching multiple exceptions
Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Use PEP 604 union syntax (str | None) instead of legacy Union/Optional syntax - enforced by ruff UP007
Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Maintain minimum 60% test coverage requirement across the codebase measured with pytest-cov
Files:
tests/unit/models/dashboard/test_model_dashboard_config.pytests/unit/validation/test_validator_hex_color.pytests/unit/models/dashboard/test_model_widget_definition.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance) for test categorization
Files:
tests/unit/models/dashboard/test_model_dashboard_config.pytests/unit/validation/test_validator_hex_color.pytests/unit/models/dashboard/test_model_widget_definition.py
🧠 Learnings (1)
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to tests/**/*.py : Use pytest markers (pytest.mark.unit, pytest.mark.integration, pytest.mark.slow, pytest.mark.smoke, pytest.mark.performance) for test categorization
Applied to files:
tests/unit/models/dashboard/test_model_widget_definition.py
🧬 Code graph analysis (3)
tests/unit/models/dashboard/test_model_dashboard_config.py (7)
src/omnibase_core/protocols/validation/protocol_constraint_validation_result.py (2)
warnings(85-87)errors(80-82)src/omnibase_core/enums/enum_dashboard_status.py (1)
EnumDashboardStatus(27-155)src/omnibase_core/enums/enum_dashboard_theme.py (1)
EnumDashboardTheme(25-63)src/omnibase_core/models/dashboard/model_dashboard_config.py (1)
ModelDashboardConfig(47-167)src/omnibase_core/models/dashboard/model_dashboard_layout_config.py (1)
ModelDashboardLayoutConfig(28-66)src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
ModelWidgetConfigChart(47-138)src/omnibase_core/models/dashboard/model_widget_definition.py (1)
ModelWidgetDefinition(90-150)
tests/unit/validation/test_validator_hex_color.py (1)
src/omnibase_core/validation/validator_hex_color.py (6)
HexColorValidator(59-157)validate_hex_color(163-186)validate_hex_color_optional(189-212)validate(79-102)validate_optional(105-125)validate_mapping(128-157)
tests/unit/models/dashboard/test_model_widget_definition.py (5)
src/omnibase_core/enums/enum_widget_type.py (1)
EnumWidgetType(25-104)src/omnibase_core/models/dashboard/model_chart_series_config.py (1)
ModelChartSeriesConfig(15-40)src/omnibase_core/models/dashboard/model_widget_config_chart.py (1)
ModelWidgetConfigChart(47-138)src/omnibase_core/models/dashboard/model_widget_config_metric_card.py (1)
ModelWidgetConfigMetricCard(47-141)src/omnibase_core/models/dashboard/model_widget_definition.py (1)
ModelWidgetDefinition(90-150)
🔇 Additional comments (9)
tests/unit/models/dashboard/test_model_widget_definition.py (5)
1-20: LGTM!Imports are clean and organized, using package-level imports from
omnibase_core.enumsandomnibase_core.models.dashboardas expected. The test file is properly licensed.
36-53: LGTM!Good use of
@pytest.mark.unitmarker per coding guidelines. The first test properly validates both the discriminator field (config_kind) and the enum value (widget_type).
113-183: Comprehensive discriminated union tests from dict.These tests effectively verify that the discriminated union deserialization works correctly for all five widget config types (chart, table, metric_card, status_grid, event_feed). Each test validates the
config_kinddiscriminator and type-specific fields.
236-259: Parametrized layout constraint tests.Effective use of
pytest.mark.parametrizeto test multiple boundary conditions with a single test method. The assertion at line 259 correctly verifies that errors are properly localized to the specific field.
350-405: LGTM!Thorough validation tests for chart series requirements and metric card trend configuration. The tests correctly verify:
- Non-pie chart types require series (line 351-357)
- Pie charts allow empty series (line 359-363)
show_trend=Truerequirestrend_key(line 373-384)These align with the validators defined in
ModelWidgetConfigChart.validate_series_for_data_chartsandModelWidgetConfigMetricCard.validate_trend_key_when_show_trendper the relevant code snippets.tests/unit/models/dashboard/test_model_dashboard_config.py (2)
212-277: Thorough widget-width-vs-columns validation tests.These tests comprehensively verify the
validate_widget_widthsmodel validator inModelDashboardConfig:
- Widget exceeding columns raises error (212-233)
- Widget at column limit succeeds (235-251)
- Multiple widgets with one exceeding columns (253-277)
Good verification that error messages contain the relevant width and column values.
279-358: Well-structured warning emission tests.Excellent coverage of the
warn_aggressive_refreshvalidator behavior:
- Tests for warnings at values 1-4s (below threshold)
- Tests for no warnings at 5s+ (at/above threshold)
- Uses
warnings.catch_warnings()withsimplefilter("error")to verify no warning scenarios- Verifies warning message content includes the actual interval value
- Confirms low intervals still create valid models despite warnings
tests/unit/validation/test_validator_hex_color.py (2)
333-421: Good Pydantic integration tests demonstrating real-world usage patterns.The tests effectively validate how the hex color validators integrate with Pydantic's
field_validatordecorator, covering both success and error cases. The error type assertions (e["type"] == "value_error") correctly verify Pydantic v2 error wrapping behavior.
428-512: Thorough import and edge case coverage.The import tests verify both module-level (
validator_hex_color) and package-level (omnibase_core.validation) exports are accessible, which is important for public API stability. The edge case tests (unicode handling, whitespace rejection, immutable mapping support, pattern thread-safety) demonstrate attention to real-world usage scenarios.
…lease the fleet re-pin is gated on OMN-16831 item 2 (runtime tenant stamping) is gated on "omnibase_core releases + fleet re-pins" -- recorded in omnibase_infra#2976 and omnimarket#2199. Ten PRs have merged to dev since v0.46.13 and none of them is on PyPI, so no downstream repo can bind against any of them. Minor, not patch: the delta contains a breaking public-API deletion. #1612 (OMN-16884) removed `status_colors` from `ModelWidgetConfigStatusGrid`, a model published since #363 that carries `extra="forbid"` -- any caller still passing `status_colors=` now raises. CHANGELOG.md's own convention names this case twice ("Minor bump per pre-1.0 semver convention for a breaking public-API deletion", v0.45.0; "...for breaking surface relocations", v0.46.0). pyproject sat at 0.46.14 only because an unrelated fix PR (d89b107, OMN-16037) bumped it inline; 0.46.14 was never tagged or published, so nothing is skipped by going to 0.47.0. Evidence-Ticket: OMN-16831
…lease the fleet re-pin is gated on (#1615) OMN-16831 item 2 (runtime tenant stamping) is gated on "omnibase_core releases + fleet re-pins" -- recorded in omnibase_infra#2976 and omnimarket#2199. Ten PRs have merged to dev since v0.46.13 and none of them is on PyPI, so no downstream repo can bind against any of them. Minor, not patch: the delta contains a breaking public-API deletion. #1612 (OMN-16884) removed `status_colors` from `ModelWidgetConfigStatusGrid`, a model published since #363 that carries `extra="forbid"` -- any caller still passing `status_colors=` now raises. CHANGELOG.md's own convention names this case twice ("Minor bump per pre-1.0 semver convention for a breaking public-API deletion", v0.45.0; "...for breaking surface relocations", v0.46.0). pyproject sat at 0.46.14 only because an unrelated fix PR (d89b107, OMN-16037) bumped it inline; 0.46.14 was never tagged or published, so nothing is skipped by going to 0.47.0. Evidence-Ticket: OMN-16831 Co-authored-by: jonahgabriel <jonah.neugass@gmail.com>
1 participant
Summary
Add dashboard-related models and enums to omnibase_core for UI presentation.
Design Decisions
config_kindLiteral discriminator (not duplicating enum values)EnumWidgetTypeas canonical vocabularyrefresh_interval_seconds(no runtime timestamps in config)ConfigDicton all definition modelsdashboard_idandwidget_id(per ONEX conventions)Test plan
Files
New files (27):
Linear: https://linear.app/omninode/issue/OMN-1284
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.