cleanup: remove root-level layout_config and components from Dashboar…#1360
Merged
Conversation
…d (DALGO-1296) - Drop layout_config and components columns from Dashboard model (migration 0161) - Remove fields from DashboardResponse and FrozenDashboardConfig schemas - Update report_service, dashboard_service, chart_service, and duplicate API to iterate tabs - Add cleanup_frozen_dashboard_root_fields management command for ReportSnapshot JSON cleanup - Delete obsolete migrate_dashboards_to_tabs and migrate_report_snapshots_to_tabs commands - Update all test fixtures and assertions to use tabs structure
|
Warning Review limit reached
More reviews will be available in 50 minutes and 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR removes root-level ChangesDashboard Tab-Based Structure Migration
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1360 +/- ##
==========================================
+ Coverage 59.52% 59.58% +0.05%
==========================================
Files 138 138
Lines 16582 16544 -38
==========================================
- Hits 9870 9857 -13
+ Misses 6712 6687 -25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddpui/services/dashboard_service.py`:
- Around line 1053-1056: The component-validation loop is currently outside the
"for tab in dashboard.tabs or []" loop causing only the last tab's components to
be validated; move the loop that iterates over components (the "for
component_id, component_config in components.items():" block) inside the tab
loop so each tab's components are validated, ensure "components =
tab.get('components') or {}" is set per tab before that inner loop, and keep
using the same identifiers (components, component_id, component_config,
component_type) to preserve existing logic.
In `@ddpui/tests/models/test_dashboard_models.py`:
- Line 145: The fixture uses a non-production chart component shape ("chart-1":
{"type": "chart", "chart_id": 1}); update the component to match the tab
component contract by replacing the root chart_id with a config object using
config.chartId (e.g., under the "chart-1" component keep "type": "chart" and add
"config": {"chartId": 1}) so tests reflect the real production schema used by
services.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3504812d-f6fe-47e2-9dff-76ce875ea57f
📒 Files selected for processing (20)
ddpui/api/dashboard_native_api.pyddpui/core/reports/report_service.pyddpui/management/commands/cleanup_frozen_dashboard_root_fields.pyddpui/management/commands/migrate_dashboards_to_tabs.pyddpui/management/commands/migrate_report_snapshots_to_tabs.pyddpui/migrations/0161_remove_dashboard_root_layout_components.pyddpui/models/dashboard.pyddpui/schemas/dashboard_schema.pyddpui/schemas/report_schema.pyddpui/services/chart_service.pyddpui/services/dashboard_service.pyddpui/tests/api_tests/test_charts_api.pyddpui/tests/api_tests/test_dashboard_native_api.pyddpui/tests/api_tests/test_public_report_api.pyddpui/tests/api_tests/test_report_api.pyddpui/tests/core/reports/test_report_service.pyddpui/tests/models/test_dashboard_models.pyddpui/tests/schema_tests/test_dashboard_schema.pyddpui/tests/services/test_chart_service.pyddpui/tests/services/test_dashboard_service.py
💤 Files with no reviewable changes (9)
- ddpui/management/commands/migrate_report_snapshots_to_tabs.py
- ddpui/models/dashboard.py
- ddpui/schemas/report_schema.py
- ddpui/management/commands/migrate_dashboards_to_tabs.py
- ddpui/schemas/dashboard_schema.py
- ddpui/api/dashboard_native_api.py
- ddpui/tests/api_tests/test_dashboard_native_api.py
- ddpui/tests/schema_tests/test_dashboard_schema.py
- ddpui/tests/services/test_dashboard_service.py
…nts and align test fixture with production schema
…leanup/remove-dashboard-root-layout-fields
Ishankoradia
approved these changes
Jun 4, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ddpui/core/reports/report_service.py (1)
97-97: ⚡ Quick winRename unused loop variable to
_comp_id.The variable
comp_idis not used in the loop body. Following Python convention, prefix unused variables with underscore.♻️ Proposed fix
- for tab in dashboard.tabs or []: - for comp_id, component in (tab.get("components") or {}).items(): + for tab in dashboard.tabs or []: + for _comp_id, component in (tab.get("components") or {}).items(): comp_type = component.get("type")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/core/reports/report_service.py` at line 97, The loop in report_service.py uses an unused loop variable comp_id in "for comp_id, component in (tab.get('components') or {}).items()"; rename comp_id to _comp_id to follow Python convention for unused variables and avoid linter warnings. Update the loop header where this occurs (the for ... in ... .items() over tab.get("components")) and ensure no other references to comp_id remain in the loop body.ddpui/schemas/report_schema.py (1)
78-79: 💤 Low valueConsider using
Field(default_factory=list)for mutable defaults.While Pydantic handles
List[...] = []safely, the explicitField(default_factory=list)pattern is clearer and silences static analysis warnings.♻️ Proposed refactor
- metric_type_tag: Optional[str] = None - program_tags: List[str] = [] - periods: List[Dict[str, Any]] = [] + metric_type_tag: Optional[str] = None + program_tags: List[str] = Field(default_factory=list) + periods: List[Dict[str, Any]] = Field(default_factory=list)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/schemas/report_schema.py` around lines 78 - 79, Replace the mutable default list assignments for program_tags and periods with Pydantic Field default factories: import Field from pydantic and change the declarations of program_tags: List[str] and periods: List[Dict[str, Any]] to use Field(default_factory=list) so each model instance gets its own list; keep the existing type annotations intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ddpui/core/reports/report_service.py`:
- Line 97: The loop in report_service.py uses an unused loop variable comp_id in
"for comp_id, component in (tab.get('components') or {}).items()"; rename
comp_id to _comp_id to follow Python convention for unused variables and avoid
linter warnings. Update the loop header where this occurs (the for ... in ...
.items() over tab.get("components")) and ensure no other references to comp_id
remain in the loop body.
In `@ddpui/schemas/report_schema.py`:
- Around line 78-79: Replace the mutable default list assignments for
program_tags and periods with Pydantic Field default factories: import Field
from pydantic and change the declarations of program_tags: List[str] and
periods: List[Dict[str, Any]] to use Field(default_factory=list) so each model
instance gets its own list; keep the existing type annotations intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 620b5080-2f13-43c9-8d6c-8eb8c3bc17d9
📒 Files selected for processing (7)
ddpui/core/reports/report_service.pyddpui/migrations/0163_remove_dashboard_root_layout_components.pyddpui/models/dashboard.pyddpui/models/report.pyddpui/schemas/report_schema.pyddpui/tests/core/reports/test_report_service.pyddpui/tests/services/test_kpi_service.py
💤 Files with no reviewable changes (1)
- ddpui/models/report.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/models/dashboard.py
Ishankoradia
approved these changes
Jun 5, 2026
…/github.com/DalgoT4D/DDP_backend into cleanup/remove-dashboard-root-layout-fields
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
layout_configandcomponentsfields from the Dashboard systemdashboard.tabs[].layout_configanddashboard.tabs[].componentscleanup_frozen_dashboard_root_fieldsmanagement command to clean up existing ReportSnapshot JSON blobsMigration steps (on deploy)
python manage.py cleanup_frozen_dashboard_root_fieldspython manage.py migrate ddpui 0161Test plan
uv run pytest ddpui/tests/ --ignore=ddpui/tests/integration_tests)layout_config/componentsin API responseSummary by CodeRabbit
New Features
Refactor
Chores
Tests