feat: add multi-tab support to dashboards#1283
Open
NaveenCode wants to merge 11 commits intomainfrom
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds persisted per-dashboard tabs via a new JSONField, updated schemas and service handling, a migration command to convert existing dashboards to tabs, and enhances dashboard duplication to deep-copy tabs and remap per-tab layout and filter component references. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Dashboard API
participant Service as Duplication Service
participant DB as Database
Client->>API: POST /dashboards/{id}/duplicate
API->>Service: validate request, load original dashboard
Service->>DB: fetch dashboard (includes tabs, layout_config, components)
DB-->>Service: return dashboard data
Service->>Service: deep-copy top-level layout/components and build filter_id mapping
Service->>Service: for each tab: deep-copy layout_config, rewrite "i" filter-* ids, rename component keys, update component.config.filterId
Service->>DB: save new dashboard with remapped tabs and components
DB-->>Service: saved dashboard
Service->>API: return duplicated dashboard
API-->>Client: 201 Created with new dashboard payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ddpui/schemas/dashboard_schema.py (1)
25-36: Tab schema is well-defined.The schema correctly captures all required tab fields. The comment mentions "max 50 chars" for title but no validation enforces it. Consider adding
Field(max_length=50)if this constraint should be enforced at the API level.The RUF012 warnings about mutable defaults are false positives—Pydantic safely handles mutable defaults by creating new instances per model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/schemas/dashboard_schema.py` around lines 25 - 36, DashboardTabSchema currently documents a 50-char limit for the title but does not enforce it; update the title field in DashboardTabSchema to use pydantic Field with max_length=50 (e.g., replace the plain annotation for title with a Field(max_length=50)) so the API-level validation enforces the constraint, and leave layout_config and components as-is since Pydantic handles mutable defaults per-instance.ddpui/api/dashboard_native_api.py (1)
245-282: Tab duplication logic looks correct.The filter ID remapping within tabs correctly mirrors the dashboard-level remapping. The
or []guard at line 247 properly handles pre-existing dashboards with NULL tabs.Minor optimization: the
copy.deepcopy(component_data)at line 260 is redundant since the entiretabslist was already deep-copied at line 247.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/api/dashboard_native_api.py` around lines 245 - 282, Remove the redundant deep copy of component data inside the tab loop: instead of new_component_data = copy.deepcopy(component_data) (in the loop that iterates over component_id/component_data), assign the already deep-copied component data directly (e.g., new_component_data = component_data) because original_dashboard.tabs was deep-copied earlier; update the assignment where you modify new_component_data["config"]["filterId"] accordingly so you don't perform an unnecessary deepcopy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddpui/models/dashboard.py`:
- Around line 73-77: The file fails black formatting; run the formatter and
commit the changes: run `black ddpui/models/dashboard.py` (or `pre-commit run
--all-files`) to reformat the `tabs = models.JSONField(...)` declaration and
surrounding code so the `JSONField` default and help_text conform to black
style, then stage and commit the reformatted file.
- Line 143: The to_json() output can include "tabs": None for pre-existing
dashboards which breaks Pydantic validation (DashboardResponse expects
List[DashboardTabSchema]); update the to_json() method (reference: to_json and
self.tabs) to coerce None to an empty list (e.g., use self.tabs or [] before
returning) so the returned dict always contains a list for "tabs" and satisfies
DashboardResponse/DashboardTabSchema validation.
In `@ddpui/schemas/dashboard_schema.py`:
- Line 80: The tabs field on DashboardSchema can still end up as an explicit
None if Dashboard.to_json() returns {"tabs": None}; update the Dashboard.to_json
method (symbol: Dashboard.to_json) to return self.tabs or [] so it never emits
None for tabs, and also consider setting the schema field tabs (symbol:
DashboardTabSchema / tabs on DashboardSchema) to use a default factory (empty
list) to guard against explicit nulls.
---
Nitpick comments:
In `@ddpui/api/dashboard_native_api.py`:
- Around line 245-282: Remove the redundant deep copy of component data inside
the tab loop: instead of new_component_data = copy.deepcopy(component_data) (in
the loop that iterates over component_id/component_data), assign the already
deep-copied component data directly (e.g., new_component_data = component_data)
because original_dashboard.tabs was deep-copied earlier; update the assignment
where you modify new_component_data["config"]["filterId"] accordingly so you
don't perform an unnecessary deepcopy.
In `@ddpui/schemas/dashboard_schema.py`:
- Around line 25-36: DashboardTabSchema currently documents a 50-char limit for
the title but does not enforce it; update the title field in DashboardTabSchema
to use pydantic Field with max_length=50 (e.g., replace the plain annotation for
title with a Field(max_length=50)) so the API-level validation enforces the
constraint, and leave layout_config and components as-is since Pydantic handles
mutable defaults per-instance.
🪄 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: 3d2b617a-4b67-40bb-85b1-67bd0818579b
📒 Files selected for processing (5)
ddpui/api/dashboard_native_api.pyddpui/migrations/0153_add_dashboard_tabs.pyddpui/models/dashboard.pyddpui/schemas/dashboard_schema.pyddpui/services/dashboard_service.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ddpui/management/commands/migrate_dashboards_to_tabs.py (3)
58-71: Consider using UUID for tab IDs and wrapping in a transaction.Two concerns with this migration block:
Tab ID collision:
int(time.time() * 1000)could produce duplicate IDs if multiple dashboards are processed within the same millisecond. A UUID would be safer and consistent with common practices.Transaction safety: If an error occurs mid-iteration, some dashboards will be migrated while others won't. Wrapping in
transaction.atomic()provides rollback capability on failure.Proposed fix
Add import at top:
import uuid from django.db import transactionThen wrap the migration logic:
+ with transaction.atomic(): for dashboard in all_dashboards: # Skip if already has tabs if dashboard.tabs and len(dashboard.tabs) > 0: skipped_count += 1 continue # ... (skip logic) ... if dry_run: # ... (dry run output) ... else: # Create default tab with existing data default_tab = { - "id": f"tab-{int(time.time() * 1000)}", + "id": f"tab-{uuid.uuid4().hex[:12]}", "title": "Untitled Tab 1", "layout_config": dashboard.layout_config or [], "components": dashboard.components or {}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/management/commands/migrate_dashboards_to_tabs.py` around lines 58 - 71, Replace the millisecond timestamp ID and unguarded save with a UUID and an atomic transaction: import uuid and from django.db import transaction, generate the tab id with uuid.uuid4() when building default_tab (the dict assigned to default_tab and later assigned to dashboard.tabs), and wrap the creation/update/save sequence (the block that sets dashboard.tabs, dashboard.layout_config, dashboard.components and calls dashboard.save()) inside a transaction.atomic() context so the migration for each dashboard is rolled back on error.
27-27: Use.iterator()for memory efficiency with large datasets.
Dashboard.objects.all()loads all records into memory. For a one-time migration with potentially many dashboards, using.iterator()avoids loading the entire queryset at once.Proposed fix
- all_dashboards = Dashboard.objects.all() + all_dashboards = Dashboard.objects.all().iterator()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/management/commands/migrate_dashboards_to_tabs.py` at line 27, Replace the eager queryset in the migration command so it doesn't load all Dashboard rows into memory: change the use of Dashboard.objects.all() assigned to all_dashboards to use Dashboard.objects.all().iterator() (or simply Dashboard.objects.iterator()) in the migration logic (migrate_dashboards_to_tabs.py where all_dashboards is used) so the iteration over dashboards is streamed and memory-efficient.
68-71: Consider preserving original data until migration is verified.Clearing
layout_configandcomponentsimmediately makes this migration irreversible. If an issue is discovered post-migration, the original data is lost.Consider either:
- Adding a
--preserve-originalflag that keeps the root-level data for verification purposes.- Documenting that a database backup should be taken before running this migration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/management/commands/migrate_dashboards_to_tabs.py` around lines 68 - 71, The migration currently overwrites dashboard.layout_config and dashboard.components irreversibly; add a CLI option --preserve-original to the management Command (add_arguments) so when set the code copies the existing values into backup attributes (for example store dashboard.original_layout_config and dashboard.original_components or a JSON blob like dashboard._backup_data) before assigning dashboard.tabs = [default_tab] and clearing layout_config/components, and only clear them when the flag is false; also update the Command help text to document the new flag and ensure dashboard.save() persists the backup fields so originals can be verified and restored if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddpui/management/commands/migrate_dashboards_to_tabs.py`:
- Around line 1-4: The file fails the Black pre-commit check; run the Black
formatter on ddpui/management/commands/migrate_dashboards_to_tabs.py (e.g.,
`black ddpui/management/commands/migrate_dashboards_to_tabs.py`) to reformat
imports and spacing around the module-level code so the file passes CI, then
re-run tests/commit; ensure the module-level imports (time, BaseCommand,
Dashboard) and any surrounding whitespace adhere to Black's rules and no other
code changes in functions/classes (e.g., class Command) are introduced.
---
Nitpick comments:
In `@ddpui/management/commands/migrate_dashboards_to_tabs.py`:
- Around line 58-71: Replace the millisecond timestamp ID and unguarded save
with a UUID and an atomic transaction: import uuid and from django.db import
transaction, generate the tab id with uuid.uuid4() when building default_tab
(the dict assigned to default_tab and later assigned to dashboard.tabs), and
wrap the creation/update/save sequence (the block that sets dashboard.tabs,
dashboard.layout_config, dashboard.components and calls dashboard.save()) inside
a transaction.atomic() context so the migration for each dashboard is rolled
back on error.
- Line 27: Replace the eager queryset in the migration command so it doesn't
load all Dashboard rows into memory: change the use of Dashboard.objects.all()
assigned to all_dashboards to use Dashboard.objects.all().iterator() (or simply
Dashboard.objects.iterator()) in the migration logic
(migrate_dashboards_to_tabs.py where all_dashboards is used) so the iteration
over dashboards is streamed and memory-efficient.
- Around line 68-71: The migration currently overwrites dashboard.layout_config
and dashboard.components irreversibly; add a CLI option --preserve-original to
the management Command (add_arguments) so when set the code copies the existing
values into backup attributes (for example store
dashboard.original_layout_config and dashboard.original_components or a JSON
blob like dashboard._backup_data) before assigning dashboard.tabs =
[default_tab] and clearing layout_config/components, and only clear them when
the flag is false; also update the Command help text to document the new flag
and ensure dashboard.save() persists the backup fields so originals can be
verified and restored if needed.
🪄 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: d4d8896b-bacb-432f-86c7-904d25b8cead
📒 Files selected for processing (1)
ddpui/management/commands/migrate_dashboards_to_tabs.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ddpui/schemas/dashboard_schema.py (1)
34-35: Replace mutable defaults inDashboardTabSchemawithdefault_factory.Lines 34-35 use
[]/{}as class-level defaults. UseField(default_factory=...)to avoid mutable-default pitfalls and satisfy RUF012.Suggested fix
from typing import Optional, List +from pydantic import Field @@ - layout_config: List[dict] = [] # Grid positions for this tab - components: dict = {} # Component configs for this tab + layout_config: List[dict] = Field(default_factory=list) # Grid positions for this tab + components: dict = Field(default_factory=dict) # Component configs for this tab🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/schemas/dashboard_schema.py` around lines 34 - 35, DashboardTabSchema uses class-level mutable defaults for layout_config and components; update those attributes to use pydantic Field with default_factory to avoid mutable-default pitfalls: replace layout_config: List[dict] = [] with layout_config: List[dict] = Field(default_factory=list) and replace components: dict = {} with components: dict = Field(default_factory=dict); ensure Field is imported from pydantic and keep the existing type annotations (List, dict) or adjust to Dict/Mapping if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddpui/management/commands/migrate_dashboards_to_tabs.py`:
- Around line 28-33: The code currently materializes Dashboard.objects.all()
into all_dashboards which can blow memory; change the loop to stream dashboards
in manageable chunks by replacing Dashboard.objects.all() with a streaming
queryset (e.g.,
Dashboard.objects.only('<fields_needed>').iterator(chunk_size=1000) or use
values_list/values with batch sizes) and iterate that instead in the for
dashboard in ... loop; preserve migrated_count and skipped_count logic but
ensure you only fetch the fields required for the migration (use only() or
values_list on the Dashboard model) to minimize memory use.
---
Nitpick comments:
In `@ddpui/schemas/dashboard_schema.py`:
- Around line 34-35: DashboardTabSchema uses class-level mutable defaults for
layout_config and components; update those attributes to use pydantic Field with
default_factory to avoid mutable-default pitfalls: replace layout_config:
List[dict] = [] with layout_config: List[dict] = Field(default_factory=list) and
replace components: dict = {} with components: dict =
Field(default_factory=dict); ensure Field is imported from pydantic and keep the
existing type annotations (List, dict) or adjust to Dict/Mapping if desired.
🪄 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: 81b014ee-0ef5-4b56-9df1-201fc15427e5
📒 Files selected for processing (3)
ddpui/management/commands/migrate_dashboards_to_tabs.pyddpui/models/dashboard.pyddpui/schemas/dashboard_schema.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/models/dashboard.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ddpui/tests/api_tests/test_dashboard_native_api.py (1)
670-670: Use iterator access over list materialization for first key retrieval.Line 670 unnecessarily materializes all dictionary keys into a list only to access the first element. This is inefficient and non-idiomatic.
♻️ Suggested refactor
- new_filter_key = list(new_tab.components.keys())[0] + new_filter_key = next(iter(new_tab.components))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/tests/api_tests/test_dashboard_native_api.py` at line 670, The test currently materializes all keys via list(new_tab.components.keys()) to get the first key; replace that with iterator access to avoid building a list by using next(iter(new_tab.components)) (or next(iter(new_tab.components.keys()))) and assign to new_filter_key so the first key is retrieved idiomatically and efficiently; update the reference in test_dashboard_native_api where new_filter_key is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddpui/tests/api_tests/test_dashboard_native_api.py`:
- Line 680: Run the project's formatter and pre-commit hooks (e.g., black and
pre-commit) on ddpui/tests/api_tests/test_dashboard_native_api.py and commit the
resulting changes so CI's Black hook passes; specifically ensure the assertion
line referencing
new_tab.components[f"filter-{new_filter_id}"]["config"]["filterId"] is formatted
according to Black's rules and that you staged and committed the reformatted
file before pushing.
In `@ddpui/tests/services/test_dashboard_service.py`:
- Line 39: Run the project formatter (Black) or pre-commit hooks locally and
commit the resulting changes to stop CI from reformatting this file;
specifically reformat the import line that currently reads "from
ddpui.schemas.dashboard_schema import DashboardCreate, DashboardUpdate,
DashboardTabSchema" in ddpui/tests/services/test_dashboard_service.py (and any
other affected lines noted at 513) so the file matches Black's style, then add
and commit those formatting changes.
---
Nitpick comments:
In `@ddpui/tests/api_tests/test_dashboard_native_api.py`:
- Line 670: The test currently materializes all keys via
list(new_tab.components.keys()) to get the first key; replace that with iterator
access to avoid building a list by using next(iter(new_tab.components)) (or
next(iter(new_tab.components.keys()))) and assign to new_filter_key so the first
key is retrieved idiomatically and efficiently; update the reference in
test_dashboard_native_api where new_filter_key is set.
🪄 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: 8bd42dc4-c027-47e7-85be-d900f54538bc
📒 Files selected for processing (2)
ddpui/tests/api_tests/test_dashboard_native_api.pyddpui/tests/services/test_dashboard_service.py
…eature/dashboard_tabs
siddhant3030
reviewed
Apr 3, 2026
| updated_components[new_component_id] = new_component_data | ||
|
|
||
| # Update the dashboard with the corrected layout_config and components | ||
| # Copy tabs and update filter IDs inside each tab |
Contributor
There was a problem hiding this comment.
I dont understand this. once migration is done isn't this will be just dead code?
The migration command clears root-level layout_config/components and moves everything into tabs, but the frontend save still sends both . Can we make
tabs the single source of truth and stop using root-level fields for tabbed dashboards
| """ | ||
| # Generate default tab for new dashboard | ||
| default_tab = { | ||
| "id": f"tab-{int(time.time() * 1000)}", |
Contributor
There was a problem hiding this comment.
do we really need this? dashboard id will be different right?
Contributor
Author
There was a problem hiding this comment.
Yes, the Dashboard id is different from the tab id.
Dashboard id identifies the dashboard, and Tab id identifies
which tab within that dashboard. A dashboard can have multiple tabs, so we need
a way to reference each one.
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
Added tabs JSONField to the Dashboard model to store per-tab layout and components
Created DashboardTabSchema Pydantic schema to validate tab structure (id, title, layout_config, components)
Updated DashboardUpdate schema to accept an optional tabs list for saving tab state
Updated DashboardResponse schema to return tabs in API responses
DashboardService.create_dashboard() now initializes every new dashboard with a default tab ("Untitled Tab 1") with empty layout and components
DashboardService.update_dashboard() handles saving tabs by converting Pydantic models to dicts before JSON storage
Updated dashboard copy (duplicate) logic in native API to deep-copy tabs and remap filter IDs in both layout_config and components of each tab so copied dashboards have correct references
** Tasks Completed **
Create a new dashboard — verify it is saved with a single default tab in the DB (tabs field)
Update a dashboard with multiple tabs — verify all tab data (title, layout, components) is persisted correctly
Copy/duplicate a dashboard with tabs and filters — verify filter IDs in tab layout and components are remapped to the new filter IDs
Fetch a dashboard via GET — verify tabs array is returned in the response
Existing dashboards without tabs field load correctly (empty tabs: [] returned)
Verify no regression on existing layout/components/filter endpoints
Summary by CodeRabbit
New Features
Chores
Tests