Honour the id specified when importing a layout - #4330
Merged
rajithacharith merged 1 commit intoJul 24, 2026
Conversation
rajithacharith
requested review from
ThaminduDilshan and
brionmario
as code owners
July 24, 2026 09:16
Contributor
📝 WalkthroughWalkthroughLayout creation now accepts an optional ID, preserves provided IDs, generates UUIDv7 values when absent, and propagates YAML IDs through declarative imports. Handlers, importer interfaces, mocks, and tests were updated for the new request type. ChangesLayout ID-aware creation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Importer
participant LayoutService
participant LayoutStore
Importer->>LayoutService: CreateLayout(request with YAML ID)
LayoutService->>LayoutService: Preserve ID or generate UUIDv7
LayoutService->>LayoutStore: CreateLayout(id, store request)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/system/importer/service_adapters.go`:
- Around line 620-632: Update the applicable declarative layouts guide under
docs/content/guides/ to document that YAML id values are preserved during layout
import, including behavior for explicit-ID creation and upsert operations.
Ensure the documentation accurately reflects the import behavior implemented
around CreateLayoutRequestWithID and UpdateLayoutRequest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f1f37862-e2e2-4d08-a2c7-38672c6303fd
⛔ Files ignored due to path filters (1)
backend/tests/mocks/design/layoutmock/LayoutMgtServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (9)
backend/internal/design/layout/mgt/LayoutMgtServiceInterface_mock_test.gobackend/internal/design/layout/mgt/handler.gobackend/internal/design/layout/mgt/handler_test.gobackend/internal/design/layout/mgt/init_test.gobackend/internal/design/layout/mgt/model.gobackend/internal/design/layout/mgt/service.gobackend/internal/design/layout/mgt/service_test.gobackend/internal/system/importer/service.gobackend/internal/system/importer/service_adapters.go
Comment on lines
+620
to
632
| createReq := layoutmgt.CreateLayoutRequestWithID{ | ||
| ID: req.ID, | ||
| Handle: req.Handle, | ||
| DisplayName: req.DisplayName, | ||
| Description: req.Description, | ||
| Layout: layoutBytes, | ||
| } | ||
| updateReq := layoutmgt.UpdateLayoutRequest{ | ||
| Handle: req.Handle, | ||
| DisplayName: req.DisplayName, | ||
| Description: req.Description, | ||
| Layout: layoutBytes, | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.
Missing documentation:
- Declarative layout YAML IDs: document that
idis preserved during layout import, including explicit-ID create and upsert behavior, in the applicable guide underdocs/content/guides/.
As per path instructions, declarative user-facing changes require corresponding documentation updates under docs/.
🤖 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 `@backend/internal/system/importer/service_adapters.go` around lines 620 - 632,
Update the applicable declarative layouts guide under docs/content/guides/ to
document that YAML id values are preserved during layout import, including
behavior for explicit-ID creation and upsert operations. Ensure the
documentation accurately reflects the import behavior implemented around
CreateLayoutRequestWithID and UpdateLayoutRequest.
Source: Path instructions
UdeshAthukorala
previously approved these changes
Jul 24, 2026
rajithacharith
added
trigger-pr-builder
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
rajithacharith
force-pushed
the
fix-layout-import-id
branch
from
July 24, 2026 10:12
3d2f73e to
ca3e2bf
Compare
UdeshAthukorala
previously approved these changes
Jul 24, 2026
rajithacharith
force-pushed
the
fix-layout-import-id
branch
from
July 24, 2026 11:49
ca3e2bf to
ea8ab84
Compare
rajithacharith
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/internal/system/importer/service_test.go (1)
1830-1996: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd dry-run test coverage for layout import.
The new tests cover upsert/create/update/error paths but none exercise
DryRun: true, even thoughimportLayout's dry-run branch (GetLayoutexistence check, no actual mutation) is new/changed logic with its own bug surface separate from the real-run path.🤖 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 `@backend/internal/system/importer/service_test.go` around lines 1830 - 1996, Add dry-run coverage alongside the layout import tests, targeting ImportResources with DryRun set to true and exercising both an existing layout and a missing layout. Verify the dry-run results reflect the expected operation/status while fakeLayoutService records no created or updated mutations, covering importLayout’s GetLayout existence-check branch.backend/internal/system/importer/service_adapters.go (1)
620-673: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider a generic import helper to avoid duplicating this branching across resource types.
importLayoutduplicates the dry-run/upsert/create/update branching already present inimportThemealmost verbatim, and this pattern will likely repeat for future declarative resource types now that the sharedimportDesignResourcehelper is gone. A small Go-generics-based helper (parameterized over create/update request types and the create/get/update calls) could collapse this into one implementation.🤖 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 `@backend/internal/system/importer/service_adapters.go` around lines 620 - 673, Introduce a reusable generic import helper for the branching currently duplicated by importLayout and importTheme, covering dry-run, upsert lookup, update, not-found fallback to create, and direct create flows. Parameterize the helper over each resource’s request and response types and service operations, then refactor importLayout and importTheme to delegate to it while preserving their existing outcomes and error handling.
🤖 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 `@backend/internal/system/importer/service_adapters.go`:
- Around line 620-673: Introduce a reusable generic import helper for the
branching currently duplicated by importLayout and importTheme, covering
dry-run, upsert lookup, update, not-found fallback to create, and direct create
flows. Parameterize the helper over each resource’s request and response types
and service operations, then refactor importLayout and importTheme to delegate
to it while preserving their existing outcomes and error handling.
In `@backend/internal/system/importer/service_test.go`:
- Around line 1830-1996: Add dry-run coverage alongside the layout import tests,
targeting ImportResources with DryRun set to true and exercising both an
existing layout and a missing layout. Verify the dry-run results reflect the
expected operation/status while fakeLayoutService records no created or updated
mutations, covering importLayout’s GetLayout existence-check branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d3e66c56-5e74-4a41-ab5e-b2e4188bda10
⛔ Files ignored due to path filters (1)
backend/tests/mocks/design/layoutmock/LayoutMgtServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (10)
backend/internal/design/layout/mgt/LayoutMgtServiceInterface_mock_test.gobackend/internal/design/layout/mgt/handler.gobackend/internal/design/layout/mgt/handler_test.gobackend/internal/design/layout/mgt/init_test.gobackend/internal/design/layout/mgt/model.gobackend/internal/design/layout/mgt/service.gobackend/internal/design/layout/mgt/service_test.gobackend/internal/system/importer/service.gobackend/internal/system/importer/service_adapters.gobackend/internal/system/importer/service_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/internal/design/layout/mgt/handler.go
- backend/internal/system/importer/service.go
- backend/internal/design/layout/mgt/service.go
- backend/internal/design/layout/mgt/handler_test.go
- backend/internal/design/layout/mgt/model.go
- backend/internal/design/layout/mgt/LayoutMgtServiceInterface_mock_test.go
- backend/internal/design/layout/mgt/service_test.go
UdeshAthukorala
approved these changes
Jul 24, 2026
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.
Purpose
When a layout is imported via the declarative import flow, the
idspecified in the YAML was silently discarded and a fresh UUID was generated instead. The imported layout therefore never got the intended id, which breaks references to it (e.g.layoutIdon organization units) and makes imports non-idempotent by id.Themes and every other importable resource (organization unit, entity type, role, user, agent) already honor the incoming
id; layout was the only outlier.Approach
Mirror the theme pattern for layouts:
CreateLayoutRequestWithIDservice request type that carries an optionalid.CreateLayoutnow honorslayout.IDwhen non-empty and only generates a UUID when it is empty, then builds a plainCreateLayoutRequestfor the store (the store signature is unchanged).ID: req.IDthrough toCreateLayout.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
🤖 Generated with Claude Code
Summary by CodeRabbit