Skip to content

Dashboard: Fix duplicate rendering after inline custom dashboard saves - #270

Merged
GurinderRawala merged 1 commit into
coderabbit_micro_frontendfrom
codex/dashboard-portal-ownership
Sep 30, 2026
Merged

GurinderRawala merged 1 commit into
coderabbit_micro_frontendfrom
codex/dashboard-portal-ownership

Conversation

@GurinderRawala

@GurinderRawala GurinderRawala commented Sep 30, 2026 •

Copy link
Copy Markdown

Summary

Fix duplicate custom dashboard rendering after an inline panel resize/color edit is saved. A host portal now has exactly one dashboard owner: explicitly assigning its container to a new UID removes the previous dashboard and its Redux store atomically. Separate containers (including drill-down drawers) remain independent.

Root cause

The custom-dashboard host preserves its portal container while switching between published and draft dashboard UIDs. The MFE registry was keyed only by UID and retained both entries. Both portals rendered into the same existing DOM container, so the stale-container poller never removed either. Saving left the duplicate dashboard mounted, with the top instance's variable filter row empty.

Reproduced on local Adoption & Knowledge, without visiting a panel editor: drag the first panel's resize handle, wait for the draft, then click Save changes. Before this fix there were two dashboard roots / 18 panels instead of one / 9; after save the top filter submenu had height 0 while the other was 44px.

Validation

  • New render regression fails before the fix with two roots instead of one; passes after it. Covers repeated published → draft → published transitions and active-owner selection.
  • Same-UID partial updates retain their store and refresh revision; replacing one container preserves the other container's store. Existing two-portal and drawer/time-service lifecycle tests remain green.
  • node node_modules/jest/bin/jest.js --runInBand --watch=false public/app/fn-app/fn-dashboard-page/fn-dashboard.test.tsx public/app/fn-app/create-mfe.test.tsx: 23 tests pass.
  • Scoped ESLint, Prettier and git diff --check pass.
  • Local Grafana webpack development build succeeds (typechecker disabled for this existing development build; no full-repository typecheck claimed).
  • Browser acceptance on localhost:5173 with Grafana localhost:3003: two consecutive inline resize → Save changes cycles retain one dashboard / nine panels and the visible Repository Name, Username and Teams filters. No editor navigation. First panel restored to its original 804×220px dimensions.

Scope / rollout

Client-side Grafana MFE registry only. No API, credential, BigQuery, RLS, persistence, schema or production changes. Explicit nonempty portal assignments transfer ownership; UID-only refresh/state patches do not evict another portal. Deploy through the normal authorized Grafana release path, then verify inline save and independent drill-down containers. Revert this commit if needed. This PR is not an authorization to deploy or merge.

Agent handoff
  • User request: Investigate and fix duplicate dashboards/hidden top filters after save; specifically test Adoption inline resizing or color changes without entering the edit page; submit a PR.
  • Goal: One dashboard instance per host container through draft and publish transitions.
  • Current state: Bug reproduced through the normal UI and a failing regression; fix and local validation completed.
  • Changes: public/app/store/configureMfeStore.ts evicts a prior UID/store only when another UID explicitly claims the same nonempty container. public/app/fn-app/fn-dashboard-page/fn-dashboard.test.tsx covers replacement, repeated transitions, retained stores and cleanup.
  • Decision: Enforce ownership in the MFE registry, rather than force host reloads or remove unrelated dashboards.
  • Service boundaries / external I/O: None changed.
  • Guardrails: Do not touch unrelated local Grafana Go dependencies, color-edit changes or mono running services. Do not release autonomously.
  • Risks / open questions: Full repository typecheck and CI still pending. Local Grafana has pre-existing unrelated changes; the committed fix and unit tests are isolated on upstream coderabbit_micro_frontend base 96a06f3d20de1fe227ad53f9cd0143b1e7e2de39. Browser testing used the existing local stack plus this exact store patch.
  • Next steps: Review current-head CI and reviewer feedback; verify normal Grafana rollout only after authorized release.
  • Verification: Commands and browser results above; no production recovery claim.
  • Sources: User-reported local Adoption & Knowledge dashboard and this PR's code/tests; related host dashboard work is https://github.com/coderabbitai/mono/pull/50282.
  • First command: git status --short in /Users/gurindersingh/Work/coderabbitai/grafana-dashboard-save.
  • First files: The two changed paths above.
  • First pending task: Check this PR's review and CI results against its current head.

Summary by CodeRabbit

  • Bug Fixes
    • Improved dashboard replacement when updates switch between draft and published versions in the same portal.
    • Dashboard updates now avoid leaving a duplicate dashboard in a portal, while preserving the state and identity of dashboards in other portals.
    • Updates that only refresh a dashboard’s UID continue to preserve its existing state.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 7d5a8eee-4dfc-4e7f-ad60-a991594ce92d

📥 Commits

Reviewing files that changed from the base of the PR and between 96a06f3 and a70e0ed.

📒 Files selected for processing (2)
  • public/app/fn-app/fn-dashboard-page/fn-dashboard.test.tsx
  • public/app/store/configureMfeStore.ts
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

Included review availability: This review used your included allowance. Your plan provides up to 100 included reviews per hour; 96 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (2)
  • GitHub Check: build-and-test
  • GitHub Check: Build
🧰 Additional context used
📓 Path-based instructions (2)
Do not allow use of `eslint-disable`, `@ts-expect-error`, or `@ts-ignore` unless there's a clear, inline comment explaining why it's necessary.

⚙️ CodeRabbit configuration file

Files:

  • public/app/store/configureMfeStore.ts
We are operating at scale.

⚙️ CodeRabbit configuration file

Files:

  • public/app/store/configureMfeStore.ts
🔇 Additional comments (2)
public/app/store/configureMfeStore.ts (1)

58-65: Ownership eviction loop is correct and bounded.

The loop runs only when the patch has both uid and an explicit portalContainerID. It deletes entries from state.dashboards while iterating Object.entries(...). Object.entries returns a snapshot array, so the deletes are safe. The loop is O(n) over dashboards per container-assigning update. The dashboard count per host is small, so the cost is acceptable.

The eviction runs before setGrafanaStore. The new UID's store is therefore created after the old store is removed. This matches the tests.

LGTM!

public/app/fn-app/fn-dashboard-page/fn-dashboard.test.tsx (1)

109-111: LGTM!

Also applies to: 145-187


📝 Walkthrough

Walkthrough

When an update assigns a nonempty portal container to a dashboard UID, the MFE registry removes other dashboards and their Grafana stores that use that container. UID-only updates do not remove other dashboards. Tests cover repeated draft and published UID transitions, same-UID updates, store retention, and teardown cleanup.

Suggested reviewers: harjotgill

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to a70e0

The change prevents duplicate dashboards during container ownership transitions without disturbing unrelated dashboards. No actionable merge-blocking risk remains, subject to normal CI checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a70e0

The change contains duplicate rendering within a dashboard container and preserves independent containers. A pending refresh can still recreate state for a displaced dashboard, so ownership cleanup is not complete across asynchronous transitions. No new externally reachable security vulnerability was established.

Retained concerns

  • Low · reliability · inferred: Portal replacement deletes the old dashboard and store without retiring its pending refresh. A later successful completion can recreate the old UID with default state. If the default portal exists, that entry is renderable rather than automatically discarded, weakening ownership and cleanup containment across asynchronous transitions.
Security review details

Security Blast Radius

  • observed — The new destructive operation affects all other dashboard/store entries in the same browser registry whose container ID matches the claim. Different container IDs are excluded, and the eviction branch performs no backend operation.

Security Findings and Attack Paths

  • inferred — A caller able to submit explicit lifecycle UID/container properties can now evict matching registry entries. Such callers previously could place dashboards into those containers without eviction. The source establishes this capability change, but not an independently untrusted caller or a cross-tenant attack path.

Trust Boundaries and Controls

  • observed — UID-only completion cannot directly evict a replacement with another UID. Refresh initiation requires a mounted model with the requested UID and restores rendering identity synchronously before awaiting completion. Runtime-property merging also rejects a mismatched UID; these are identity safeguards, not host authorization checks.

Resilience and Maintainability Implications

  • observed — Unmount invalidates pending refresh generations, unmounts the original React root, and cancels in-flight backend requests. Container ownership replacement does not invoke those lifecycle controls, so its cleanup guarantees differ from full unmount.

Hardening Proposals

  • proposed — Bind refresh completion to a dashboard ownership generation, or reject completion patches for an evicted owner, so replacement cannot recreate default-state dashboard entries through stale asynchronous work.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: fixing duplicate dashboard rendering after inline custom dashboard saves.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Comment Severity Gate ✅ Passed No supplied CodeRabbit findings remain outstanding. The current review produced zero actionable findings, and no posted CodeRabbit review threads were returned. Therefore, no unresolved Critical or Ma…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


A rabbit watched the dashboards trade,
One portal kept one place to stay.
Draft came by, then published too,
The old store left; the new one grew.
Two separate portals stayed apart,
And tests kept watch with careful heart.

Comment @coderabbitai help to get the list of available commands.

@GurinderRawala
GurinderRawala merged commit 8139d67 into coderabbit_micro_frontend Sep 30, 2026
4 checks passed
@GurinderRawala
GurinderRawala deleted the codex/dashboard-portal-ownership branch September 30, 2026 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant