feat: instance contextual layout fetching #1544
Conversation
Jondyr
changed the title
📝 WalkthroughWalkthroughAdds a frontend feature flag, a new app-implementable interface for per-instance layouts, two instance-scoped layout endpoints in ResourceController wired via AppImplementationFactory, and accompanying tests, OpenAPI and metadata snapshot updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used📓 Path-based instructions (1)**/*.cs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (16)📓 Common learnings📚 Learning: 2025-08-22T13:46:43.017ZApplied to files:
📚 Learning: 2025-09-21T08:19:43.290ZApplied to files:
📚 Learning: 2025-08-22T13:35:09.565ZApplied to files:
📚 Learning: 2025-11-28T08:07:52.106ZApplied to files:
📚 Learning: 2025-09-18T10:23:05.995ZApplied to files:
📚 Learning: 2025-08-29T10:45:57.158ZApplied to files:
📚 Learning: 2025-09-25T08:15:25.624ZApplied to files:
📚 Learning: 2025-11-28T08:07:52.106ZApplied to files:
📚 Learning: 2025-08-29T10:45:57.158ZApplied to files:
📚 Learning: 2025-09-29T08:34:53.864ZApplied to files:
📚 Learning: 2025-09-29T08:34:53.864ZApplied to files:
📚 Learning: 2025-10-17T07:45:15.474ZApplied to files:
📚 Learning: 2025-09-21T09:04:43.977ZApplied to files:
📚 Learning: 2025-09-07T20:03:48.030ZApplied to files:
📚 Learning: 2025-09-18T10:12:21.841ZApplied to files:
🧬 Code graph analysis (2)src/Altinn.App.Core/Internal/App/FrontendFeatures.cs (1)
src/Altinn.App.Api/Controllers/ResourceController.cs (2)
⏰ 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). (5)
🔇 Additional comments (1)
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 |
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
from
03c5d90 to
1f10c35
Compare
Jondyr
added
feature
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
2 times, most recently
from
e8df578 to
e56150a
Compare
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
2 times, most recently
from
21399d0 to
3dffd17
Compare
ivarne
left a comment
ivarne
left a comment
There was a problem hiding this comment.
Siden dette er en applikasjons konfigurasjon for frontend, så ville jeg droppet FeatureManager og heller bare bedt brukerene gå direkte til appsettings.json og legge til "FrontendSettings": {"AddInstanceIdentifierToLayoutRequests": true} uten at det krever noe videre konfigurasjon i backend.
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
from
3dffd17 to
271ca38
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
Fixed
Show fixed
Hide fixed
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
Fixed
Show fixed
Hide fixed
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
from
f307242 to
301807c
Compare
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
from
69d0c68 to
733ec02
Compare
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
from
7ff630d to
ec066af
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Altinn.App.Api/Controllers/ResourceController.cs (2)
105-105: Add error handling for invalid GUID format.
Guid.Parse(instanceId)will throw aFormatExceptionif theinstanceIdis not a valid GUID, resulting in an unhandled 500 error. Consider using a route constraint or TryParse with proper error response.Apply this diff to add proper validation:
+ [ProducesResponseType(StatusCodes.Status400BadRequest)] [ProducesResponseType(typeof(string), StatusCodes.Status200OK, "application/json")] [HttpGet] - [Route("{org}/{app}/instances/{instanceOwnerPartyId:int}/{instanceId}/layouts/{layoutSetId}")] + [Route("{org}/{app}/instances/{instanceOwnerPartyId:int}/{instanceGuid:guid}/layouts/{layoutSetId}")] public async Task<ActionResult> GetInstanceLayouts( string org, string app, int instanceOwnerPartyId, - string instanceId, + Guid instanceGuid, string layoutSetId ) { ICustomLayoutForInstance? customLayoutService = _appImplementationFactory.Get<ICustomLayoutForInstance>(); if (customLayoutService is not null) { string? customLayout = await customLayoutService.GetCustomLayoutForInstance( layoutSetId, instanceOwnerPartyId, - Guid.Parse(instanceId) + instanceGuid );Using the
:guidroute constraint validates the format at the routing level and automatically returns 404 for invalid formats, while the method receives a strongly-typedGuidparameter.
100-110: Fall back to default layouts when custom service returns null.
ICustomLayoutForInstance.GetCustomLayoutForInstancereturnsstring?, meaning implementations can returnnullto indicate "use the default layout". Currently, the code returnsOk(customLayout)even whencustomLayoutis null, sending an empty 200 response instead of falling back.Apply this diff to handle null results properly:
if (customLayoutService is not null) { string? customLayout = await customLayoutService.GetCustomLayoutForInstance( layoutSetId, instanceOwnerPartyId, - Guid.Parse(instanceId) + instanceGuid ); - return Ok(customLayout); + if (!string.IsNullOrEmpty(customLayout)) + { + return Ok(customLayout); + } } string layouts = _appResourceService.GetLayoutsForSet(layoutSetId); return Ok(layouts);
🧹 Nitpick comments (1)
test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json (1)
5215-5221: Specify UUID format for the instance identifier.Add
"format": "uuid"to the instance id parameter to match the rest of the spec and improve generated client types.{ - "name": "instanceId", + "name": "instanceGuid", "in": "path", "description": "The instance id", "required": true, "schema": { - "type": "string" + "type": "string", + "format": "uuid" } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Altinn.App.Api/Controllers/ResourceController.cs(3 hunks)test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs(1 hunks)test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json(1 hunks)test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(2 hunks)test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
🧠 Learnings (15)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.jsontest/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txttest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.jsontest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txttest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txt
📚 Learning: 2025-09-18T10:23:05.995Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json:7263-0
Timestamp: 2025-09-18T10:23:05.995Z
Learning: In PR #1357, ApplicationMetadata.apiScopes (now ApiScopesConfiguration) is currently unused at runtime; the schema change is acceptable without a transition mechanism for now.
Applied to files:
test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txt
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/test/**/*.cs : Prefer xUnit asserts over FluentAssertions in tests
Applied to files:
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
🧬 Code graph analysis (2)
src/Altinn.App.Api/Controllers/ResourceController.cs (3)
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (1)
Task(18-21)src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs (1)
Task(17-17)src/Altinn.App.Core/Interface/IAppResources.cs (2)
Task(38-38)Task(135-135)
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (2)
test/Altinn.App.Api.Tests/Program.cs (1)
Program(162-162)test/Altinn.App.Api.Tests/Data/TestData.cs (1)
PrepareInstance(147-174)
⏰ 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). (5)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Analyze (csharp)
- GitHub Check: Static code analysis
🔇 Additional comments (3)
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (1)
1-75: LGTM!The test file is well-structured with proper xUnit assertions, correct endpoint paths, and appropriate test coverage for both scenarios (with and without
ICustomLayoutForInstance). TheJsonDocumentis properly disposed, and tests follow the coding guidelines.src/Altinn.App.Api/Controllers/ResourceController.cs (2)
1-1: New import looks good.The
using Altinn.App.Core.Features;directive is correctly added to support theAppImplementationFactorytype.
15-26: Constructor changes follow established patterns.The injection of
IServiceProviderand resolution ofAppImplementationFactoryfollows the same pattern used in other controllers (e.g.,EventsReceiverController).
| "/{org}/{app}/instances/{instanceOwnerPartyId}/{instanceId}/layouts/{layoutSetId}": { | ||
| "get": { | ||
| "tags": [ | ||
| "Resource" | ||
| ], | ||
| "summary": "Endpoint for layouts with instance context.\nUses ICustomLayoutForInstance if implemented with IAppResources as fallback.", | ||
| "parameters": [ | ||
| { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check controller route and action parameters naming for the new endpoint
echo "=== Searching for GetInstanceLayouts or related methods ==="
rg -n -C3 -g '!**/bin/**' -g '!**/obj/**' 'GetInstanceLayouts|layouts' src/Altinn.App.Api/Controllers/ResourceController.cs 2>/dev/null || echo "Pattern not found or file missing"
echo ""
echo "=== Searching for instance layout endpoint ==="
rg -n -C5 -g '!**/bin/**' -g '!**/obj/**' 'layouts.*layoutSetId' src/Altinn.App.Api/Controllers/ 2>/dev/null || echo "Pattern not found"
echo ""
echo "=== Inventory of instance path params across controllers ==="
rg -n 'instance(Guid|Id)' src/Altinn.App.Api/Controllers/ 2>/dev/null | head -30 || echo "Pattern search completed"
echo ""
echo "=== Check ResourceController.cs file location and size ==="
find . -name 'ResourceController.cs' -type f | grep -v bin | grep -v objRepository: Altinn/app-lib-dotnet
Length of output: 11490
Align parameter naming and add route constraint: use instanceGuid:guid instead of instanceId.
Other instance endpoints (ValidateController, SigningController, UserDefinedMetadataController) consistently use instanceGuid with a :guid route constraint. This new endpoint diverges by using instanceId without the format constraint. While the code internally parses it as a GUID (line 105: Guid.Parse(instanceId)), the inconsistency weakens API clarity and generated client SDKs. Align the route to {org}/{app}/instances/{instanceOwnerPartyId:int}/{instanceGuid:guid}/layouts/{layoutSetId} and update the method parameter accordingly.
🤖 Prompt for AI Agents
In
test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json
around lines 5179-5186, the route uses {instanceId} without a GUID constraint
which is inconsistent with other controllers; change the route segment to
{instanceGuid:guid} and add the {instanceOwnerPartyId:int} constraint as shown
({org}/{app}/instances/{instanceOwnerPartyId:int}/{instanceGuid:guid}/layouts/{layoutSetId}),
then update the corresponding method signature/parameter name from instanceId to
instanceGuid (and accept a Guid instead of parsing a string) so generated
clients and documentation reflect the GUID constraint and consistent naming.
|
/publish |
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
from
140042b to
fad3f05
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json (1)
5179-5245: Align instance parameter naming and type with existing instance APIsThis new endpoint diverges from the rest of the instance APIs:
- Other non‑deprecated instance endpoints consistently use
instanceGuid(notinstanceId) as the path/parameter name.- Those parameters are typed as
stringwith"format": "uuid"in the OpenAPI schema.- Here the path segment and parameter are named
instanceIdand lack theuuidformat, which weakens client generation and is inconsistent withICustomLayoutForInstance(which takes aGuid instanceGuid).To keep the public surface coherent and future‑proof, consider updating the controller and regenerating this spec so the path and parameter mirror the established pattern, e.g.:
- "/{org}/{app}/instances/{instanceOwnerPartyId}/{instanceId}/layouts/{layoutSetId}": { + "/{org}/{app}/instances/{instanceOwnerPartyId}/{instanceGuid}/layouts/{layoutSetId}": { ... - { - "name": "instanceId", + { + "name": "instanceGuid", "in": "path", - "description": "The instance id", + "description": "The instance guid", "required": true, "schema": { - "type": "string" + "type": "string", + "format": "uuid" } },(Adjusting the C# action signature/route and re‑recording the snapshot is preferable to hand‑editing the JSON.)
src/Altinn.App.Api/Controllers/ResourceController.cs (2)
105-105: Add error handling for GUID parsing.
Guid.Parse(instanceId)will throw aFormatExceptionifinstanceIdis not a valid GUID format, resulting in an unhandled exception and a 500 response. The route parameter has no:guidconstraint, so invalid strings can reach this method.Apply this diff to add proper validation:
+ if (!Guid.TryParse(instanceId, out var instanceGuid)) + { + return BadRequest("Invalid instance ID format"); + } + ICustomLayoutForInstance? customLayoutService = _appImplementationFactory.Get<ICustomLayoutForInstance>(); if (customLayoutService is not null) { string? customLayout = await customLayoutService.GetCustomLayoutForInstance( layoutSetId, instanceOwnerPartyId, - Guid.Parse(instanceId) + instanceGuid );
100-111: Handle null/empty custom layout responses.
ICustomLayoutForInstance.GetCustomLayoutForInstancereturnsstring?, allowing implementations to return null/empty to indicate "use the default layout." Currently, the code returnsOk(customLayout)even when null, sending an empty response to the client instead of falling back to the default layout from_appResourceService.Apply this diff to fall back when no custom layout is provided:
if (customLayoutService is not null) { string? customLayout = await customLayoutService.GetCustomLayoutForInstance( layoutSetId, instanceOwnerPartyId, Guid.Parse(instanceId) ); - return Ok(customLayout); + if (!string.IsNullOrWhiteSpace(customLayout)) + { + return Ok(customLayout); + } } string layouts = _appResourceService.GetLayoutsForSet(layoutSetId); return Ok(layouts);
🧹 Nitpick comments (1)
src/Altinn.App.Core/Features/FeatureFlags.cs (1)
16-21: Improve the XML documentation for the new feature flag.The TODO comment indicates that the summary needs improvement. The current description is functional but could be more specific about the behavior change and benefits.
Consider expanding the documentation to clarify:
- What "add instance identifier" means (layouts are fetched with instance context)
- The relationship with
ICustomLayoutForInstance- When apps should enable this feature
Apply this diff to improve the documentation:
- // TODO: write a better summary here /// <summary> - /// Enabling this feature changes backend endpoint used for layouts to - /// add instance identifier. + /// When enabled, the frontend will fetch layouts using an instance-specific endpoint + /// that includes the instance owner party ID and instance GUID. Apps can implement + /// <see cref="ICustomLayoutForInstance"/> to provide custom layouts based on instance data. + /// If not implemented, the standard layout retrieval is used. /// </summary> public const string AddInstanceIdentifierToLayoutRequests = "AddInstanceIdentifierToLayoutRequests";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Altinn.App.Api/Controllers/ResourceController.cs(3 hunks)src/Altinn.App.Core/Features/FeatureFlags.cs(1 hunks)src/Altinn.App.Core/Internal/App/FrontendFeatures.cs(1 hunks)src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs(1 hunks)test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json(1 hunks)test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Internal/App/AppMetadataTest.GetApplicationMetadata_deserialize_serialize_unmapped_properties.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Internal/App/FrontendFeaturesTest.cs(2 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(2 hunks)test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/PartyTypesAllowed/_snapshots/SubunitOnlyAppTests.ApplicationMetadata_0.verified.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Custom_0_Metadata.verified.txt
- test/Altinn.App.Integration.Tests/PartyTypesAllowed/_snapshots/SubunitOnlyAppTests.ApplicationMetadata_0.verified.txt
- src/Altinn.App.Core/Internal/App/FrontendFeatures.cs
- src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cstest/Altinn.App.Core.Tests/Internal/App/FrontendFeaturesTest.cssrc/Altinn.App.Core/Features/FeatureFlags.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cstest/Altinn.App.Core.Tests/Internal/App/FrontendFeaturesTest.cs
🧠 Learnings (16)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.jsontest/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txttest/Altinn.App.Core.Tests/Internal/App/AppMetadataTest.GetApplicationMetadata_deserialize_serialize_unmapped_properties.verified.txttest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.jsonsrc/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cstest/Altinn.App.Core.Tests/Internal/App/FrontendFeaturesTest.cstest/Altinn.App.Core.Tests/Internal/App/AppMetadataTest.GetApplicationMetadata_deserialize_serialize_unmapped_properties.verified.txttest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Features/FeatureFlags.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.jsontest/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cstest/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txttest/Altinn.App.Core.Tests/Internal/App/FrontendFeaturesTest.cstest/Altinn.App.Core.Tests/Internal/App/AppMetadataTest.GetApplicationMetadata_deserialize_serialize_unmapped_properties.verified.txttest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Features/FeatureFlags.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cstest/Altinn.App.Core.Tests/Internal/App/FrontendFeaturesTest.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cstest/Altinn.App.Core.Tests/Internal/App/AppMetadataTest.GetApplicationMetadata_deserialize_serialize_unmapped_properties.verified.txt
📚 Learning: 2025-08-27T06:58:28.126Z
Learnt from: cammiida
Repo: Altinn/app-lib-dotnet PR: 1451
File: src/Altinn.App.Api/Controllers/DataTagsController.cs:0-0
Timestamp: 2025-08-27T06:58:28.126Z
Learning: In the Altinn app-lib-dotnet codebase, validation endpoints follow a pattern where validation only runs when the ignoredValidators query parameter is provided (not null). This is a conscious design choice for consistency across validation endpoints.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/test/**/*.cs : Prefer xUnit asserts over FluentAssertions in tests
Applied to files:
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
📚 Learning: 2025-08-22T13:35:09.565Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Authentication_auth=OldUser.verified.txt:16-38
Timestamp: 2025-08-22T13:35:09.565Z
Learning: Test data in Altinn App Integration Tests snapshots (like SSNs, addresses, phone numbers in BasicAppTests.Authentication snapshots) are synthetic/fake values created for testing purposes, not real PII that needs to be scrubbed from version control.
Applied to files:
test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txttest/Altinn.App.Core.Tests/Internal/App/AppMetadataTest.GetApplicationMetadata_deserialize_serialize_unmapped_properties.verified.txt
📚 Learning: 2025-09-18T10:23:05.995Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json:7263-0
Timestamp: 2025-09-18T10:23:05.995Z
Learning: In PR #1357, ApplicationMetadata.apiScopes (now ApiScopesConfiguration) is currently unused at runtime; the schema change is acceptable without a transition mechanism for now.
Applied to files:
test/Altinn.App.Integration.Tests/CustomScopes/_snapshots/CustomScopesTests.Metadata_Standard_0_Metadata.verified.txt
🧬 Code graph analysis (2)
src/Altinn.App.Api/Controllers/ResourceController.cs (3)
src/Altinn.App.Api/Controllers/EventsReceiverController.cs (1)
Route(12-86)src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs (1)
Task(17-17)src/Altinn.App.Core/Internal/App/IAppResources.cs (3)
Task(28-28)Task(110-110)GetLayoutsForSet(127-127)
test/Altinn.App.Core.Tests/Internal/App/FrontendFeaturesTest.cs (1)
src/Altinn.App.Core/Features/FeatureFlags.cs (1)
FeatureFlags(8-22)
⏰ 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). (5)
- GitHub Check: Analyze (csharp)
- GitHub Check: Static code analysis
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
🔇 Additional comments (8)
test/Altinn.App.Core.Tests/Internal/App/FrontendFeaturesTest.cs (2)
20-20: LGTM!The addition of the new feature flag to the expected features dictionary correctly reflects the default disabled state and follows the established pattern.
38-44: LGTM!The test correctly verifies that the new feature flag is included and enabled when configured via FeatureManagement. The Moq setup follows the existing pattern for
JsonObjectInDataResponse.test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
1151-1155: Public API snapshot correctly reflects new feature flag and interfaceThe added
FeatureFlags.AddInstanceIdentifierToLayoutRequestsconstant andICustomLayoutForInstanceinterface (withGetCustomLayoutForInstance(string layoutSetId, int instanceOwnerPartyId, Guid instanceGuid)) match the PR’s described behavior. From a public‑API perspective, this snapshot update looks consistent and intentional; no further changes needed here.Also applies to: 2937-2940
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (4)
1-8: LGTM! Clean and appropriate imports.All necessary namespaces are included for the integration tests.
11-22: LGTM! Well-structured test setup.The test class follows the established integration test pattern, and the private
CustomLayoutForInstanceimplementation provides a simple, deterministic mock for testing the custom layout path.
24-47: LGTM! Test correctly validates custom layout service integration.The test properly registers the custom service via DI, exercises the new endpoint, and uses xUnit assertions as required by coding guidelines. Past review issues have been addressed.
49-74: LGTM! Fallback path validation is appropriate.The test correctly validates that when no custom layout service is registered, the endpoint falls back to the default layout retrieval mechanism and returns valid JSON structure.
src/Altinn.App.Api/Controllers/ResourceController.cs (1)
1-1: LGTM! DI integration follows established patterns.The AppImplementationFactory injection via IServiceProvider matches the pattern used in EventsReceiverController. The field is properly marked readonly and initialized in the constructor.
Also applies to: 15-15, 21-26
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
from
76553c1 to
dcfe19c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Altinn.App.Api/Controllers/ResourceController.cs (1)
99-110: Address previously flagged issues in this method.Two critical issues were flagged in previous reviews but remain unresolved:
Line 105:
Guid.Parse(instanceId)will throwFormatExceptionon invalid input, resulting in an unhandled 500 response instead of a proper 400 Bad Request.Line 107: Returning
Ok(customLayout)without checking for null/empty means clients receive a 200 with no layout when the custom service returns null (indicating "use default"). The code should fall through to line 109 in that case.Apply this diff to address both issues:
+ if (!Guid.TryParse(instanceId, out var instanceGuid)) + { + return BadRequest("Invalid instanceId format"); + } + ICustomLayoutForInstance? customLayoutService = _appImplementationFactory.Get<ICustomLayoutForInstance>(); if (customLayoutService is not null) { string? customLayout = await customLayoutService.GetCustomLayoutForInstance( layoutSetId, instanceOwnerPartyId, - Guid.Parse(instanceId) + instanceGuid ); - return Ok(customLayout); + if (!string.IsNullOrWhiteSpace(customLayout)) + { + return Ok(customLayout); + } } string layouts = _appResourceService.GetLayoutsForSet(layoutSetId); return Ok(layouts);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Api/Controllers/ResourceController.cs(4 hunks)src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Api/Controllers/ResourceController.cs
🧠 Learnings (10)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
🧬 Code graph analysis (1)
src/Altinn.App.Api/Controllers/ResourceController.cs (2)
src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs (2)
Task(17-17)Task(25-25)test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (1)
Task(18-21)
🪛 GitHub Actions: Build and Test on windows, macos and ubuntu
src/Altinn.App.Api/Controllers/ResourceController.cs
[error] 141-141: CS1573: Parameter 'instanceOwnerPartyId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do).
🪛 GitHub Check: Run dotnet build and test (macos-latest)
src/Altinn.App.Api/Controllers/ResourceController.cs
[failure] 143-143:
Parameter 'layoutSetId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
[failure] 142-142:
Parameter 'instanceId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
[failure] 141-141:
Parameter 'instanceOwnerPartyId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
[failure] 143-143:
Parameter 'layoutSetId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
[failure] 142-142:
Parameter 'instanceId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
[failure] 141-141:
Parameter 'instanceOwnerPartyId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
🪛 GitHub Check: Run dotnet build and test (ubuntu-latest)
src/Altinn.App.Api/Controllers/ResourceController.cs
[failure] 143-143:
Parameter 'layoutSetId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
[failure] 142-142:
Parameter 'instanceId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
[failure] 141-141:
Parameter 'instanceOwnerPartyId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
[failure] 143-143:
Parameter 'layoutSetId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
[failure] 142-142:
Parameter 'instanceId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
[failure] 141-141:
Parameter 'instanceOwnerPartyId' has no matching param tag in the XML comment for 'ResourceController.GetLayoutSettings(string, string, int, string, string)' (but other parameters do)
⏰ 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: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
src/Altinn.App.Api/Controllers/ResourceController.cs (1)
1-1: LGTM! Constructor pattern follows established conventions.The AppImplementationFactory injection via IServiceProvider follows the same pattern used in EventsReceiverController, and the field is properly declared as readonly.
Also applies to: 15-15, 21-26
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Altinn.App.Api/Controllers/ResourceController.cs (2)
99-111: Add input validation and null-fallback logic.This endpoint has two unresolved critical issues:
- Line 105:
Guid.Parse(instanceId)throwsFormatExceptionon invalid input, resulting in a 500 error instead of a proper 400 Bad Request.- Line 107: When
customLayoutis null (signaling "use default"), the code returnsOk(null)instead of falling back to the default layout retrieval at line 109.Apply this diff:
+ if (!Guid.TryParse(instanceId, out var instanceGuid)) + { + return BadRequest("Invalid instanceId format"); + } + ICustomLayoutForInstance? customLayoutService = _appImplementationFactory.Get<ICustomLayoutForInstance>(); if (customLayoutService is not null) { string? customLayout = await customLayoutService.GetCustomLayoutForInstance( layoutSetId, instanceOwnerPartyId, - Guid.Parse(instanceId) + instanceGuid ); - return Ok(customLayout); + if (!string.IsNullOrWhiteSpace(customLayout)) + { + return Ok(customLayout); + } } string layouts = _appResourceService.GetLayoutsForSet(layoutSetId); return Ok(layouts);
149-161: Add input validation and null-fallback logic.This endpoint has the same two critical issues as
GetInstanceLayouts:
- Line 155:
Guid.Parse(instanceId)throwsFormatExceptionon invalid input, resulting in a 500 error instead of a proper 400 Bad Request.- Line 157: When
customLayoutSettingsis null (signaling "use default"), the code returnsOk(null)instead of falling back to the default settings retrieval at line 159.Apply this diff:
+ if (!Guid.TryParse(instanceId, out var instanceGuid)) + { + return BadRequest("Invalid instanceId format"); + } + ICustomLayoutForInstance? customLayoutService = _appImplementationFactory.Get<ICustomLayoutForInstance>(); if (customLayoutService is not null) { string? customLayoutSettings = await customLayoutService.GetCustomLayoutSettingsForInstance( layoutSetId, instanceOwnerPartyId, - Guid.Parse(instanceId) + instanceGuid ); - return Ok(customLayoutSettings); + if (!string.IsNullOrWhiteSpace(customLayoutSettings)) + { + return Ok(customLayoutSettings); + } } string? settings = _appResourceService.GetLayoutSettingsStringForSet(layoutSetId); return Ok(settings);
🧹 Nitpick comments (1)
src/Altinn.App.Api/Controllers/ResourceController.cs (1)
21-25: Consider injecting AppImplementationFactory directly.The code uses the service locator pattern (resolving
AppImplementationFactoryfromIServiceProvider) instead of constructor injection. This hides the dependency and makes testing more difficult.If there's no circular dependency concern, apply this diff:
/// <param name="appResourcesService">The execution service</param> - /// <param name="serviceProvider">The service provider</param> - public ResourceController(IAppResources appResourcesService, IServiceProvider serviceProvider) + /// <param name="appImplementationFactory">The app implementation factory</param> + public ResourceController(IAppResources appResourcesService, AppImplementationFactory appImplementationFactory) { _appResourceService = appResourcesService; - _appImplementationFactory = serviceProvider.GetRequiredService<AppImplementationFactory>(); + _appImplementationFactory = appImplementationFactory; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Api/Controllers/ResourceController.cs(4 hunks)src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Api/Controllers/ResourceController.cs
🧠 Learnings (10)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
🧬 Code graph analysis (1)
src/Altinn.App.Api/Controllers/ResourceController.cs (2)
src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs (2)
Task(17-17)Task(25-25)test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (1)
Task(18-21)
⏰ 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: Static code analysis
- GitHub Check: Analyze (csharp)
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
from
dcfe19c to
3265bc4
Compare
|
/publish |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Altinn.App.Api/Controllers/ResourceController.cs (2)
99-110: Critical and major issues remain unaddressed from previous review.This endpoint still has two unresolved issues previously flagged:
Line 105:
Guid.Parse(instanceId)throwsFormatExceptionon invalid input, resulting in a 500 error instead of a proper 400 Bad Request.Line 107: Returning
Ok(customLayout)without null/empty checks means clients receive a 200 response with no layout data whencustomLayoutServicereturns null (which signals "use default"). The code should fall through to line 109 instead.Apply this diff to add proper validation and null-fallback:
+ if (!Guid.TryParse(instanceId, out var instanceGuid)) + { + return BadRequest("Invalid instanceId format"); + } + ICustomLayoutForInstance? customLayoutService = _appImplementationFactory.Get<ICustomLayoutForInstance>(); if (customLayoutService is not null) { string? customLayout = await customLayoutService.GetCustomLayoutForInstance( layoutSetId, instanceOwnerPartyId, - Guid.Parse(instanceId) + instanceGuid ); - return Ok(customLayout); + if (!string.IsNullOrWhiteSpace(customLayout)) + { + return Ok(customLayout); + } } string layouts = _appResourceService.GetLayoutsForSet(layoutSetId); return Ok(layouts);
149-160: Critical and major issues remain unaddressed from previous review.This endpoint has the same two unresolved issues as
GetInstanceLayouts:
Line 155:
Guid.Parse(instanceId)throwsFormatExceptionon invalid input, causing a 500 error instead of a proper 400 Bad Request.Line 157: Returning
Ok(customLayoutSettings)without null/empty checks means clients receive a 200 response with no settings whencustomLayoutServicereturns null (signaling "use default"). The code should continue to the fallback at line 159.Apply this diff to add proper validation and null-fallback:
+ if (!Guid.TryParse(instanceId, out var instanceGuid)) + { + return BadRequest("Invalid instanceId format"); + } + ICustomLayoutForInstance? customLayoutService = _appImplementationFactory.Get<ICustomLayoutForInstance>(); if (customLayoutService is not null) { string? customLayoutSettings = await customLayoutService.GetCustomLayoutSettingsForInstance( layoutSetId, instanceOwnerPartyId, - Guid.Parse(instanceId) + instanceGuid ); - return Ok(customLayoutSettings); + if (!string.IsNullOrWhiteSpace(customLayoutSettings)) + { + return Ok(customLayoutSettings); + } } string? settings = _appResourceService.GetLayoutSettingsStringForSet(layoutSetId); return Ok(settings);
🧹 Nitpick comments (1)
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (1)
33-130: Consider adding error case tests.The current test suite covers happy paths well, but lacks tests for error scenarios such as:
- Invalid
instanceIdformat (not a valid GUID)- Missing or invalid
layoutSetId- Authorization failures
Adding these tests would improve coverage and help catch the
Guid.Parseissues flagged in the controller.Example test for invalid GUID:
[Fact] public async Task GetInstanceLayouts_InvalidInstanceId_ReturnsBadRequest() { string org = "tdd"; string app = "contributer-restriction"; int instanceOwnerPartyId = 500600; string invalidInstanceId = "not-a-guid"; string layoutSetId = "default"; using HttpClient client = GetRootedUserClient(org, app, 1337, instanceOwnerPartyId); var response = await client.GetAsync( $"/{org}/{app}/instances/{instanceOwnerPartyId}/{invalidInstanceId}/layouts/{layoutSetId}" ); Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.App.Api/Controllers/ResourceController.cs(4 hunks)src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
🧠 Learnings (11)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/test/**/*.cs : Prefer xUnit asserts over FluentAssertions in tests
Applied to files:
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
🧬 Code graph analysis (2)
src/Altinn.App.Api/Controllers/ResourceController.cs (3)
src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs (2)
Task(17-17)Task(25-25)src/Altinn.App.Core/Internal/App/IAppResources.cs (4)
Task(28-28)Task(110-110)GetLayoutsForSet(127-127)GetLayoutSettingsStringForSet(145-145)src/Altinn.App.Core/Interface/IAppResources.cs (4)
Task(38-38)Task(135-135)GetLayoutsForSet(152-152)GetLayoutSettingsStringForSet(165-165)
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (2)
src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs (2)
Task(17-17)Task(25-25)test/Altinn.App.Api.Tests/Data/TestData.cs (1)
PrepareInstance(147-174)
⏰ 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). (5)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
🔇 Additional comments (6)
src/Altinn.App.Api/Controllers/ResourceController.cs (1)
15-15: Constructor changes follow established patterns.The addition of
AppImplementationFactoryviaIServiceProviderfollows the same pattern used inEventsReceiverControllerand properly usesGetRequiredServiceto ensure the dependency is available.Also applies to: 21-25
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (5)
16-31: Test helper implementation is appropriate.The
CustomLayoutForInstanceclass provides a simple, verifiable implementation for testing. Returning the instance GUID as a string makes it easy to verify the controller correctly invokes the custom service.
33-56: Test correctly verifies custom layout service integration.The test properly registers
ICustomLayoutForInstancevia DI, calls the instance-context endpoint, and verifies the custom service response is returned. Uses xUnit assertions as per coding guidelines.
58-83: Test correctly verifies fallback to standard layouts.The test validates the fallback behavior when no custom service is registered, properly checking the JSON structure of the default layouts response. Uses xUnit assertions as per coding guidelines.
85-108: Test correctly verifies custom layout settings integration.The test properly validates the custom service integration for layout settings, following the same pattern as the layouts test. Uses xUnit assertions as per coding guidelines.
110-130: Test correctly verifies fallback to standard layout settings.The test validates the fallback behavior for layout settings when no custom service is registered, properly checking the JSON object structure. Uses xUnit assertions as per coding guidelines.
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
from
3265bc4 to
1b4246a
Compare
|
/publish |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Altinn.App.Api/Controllers/ResourceController.cs (2)
99-111: Unaddressed issues: Missing Guid validation and null-fallback logic.Two issues from previous reviews remain unaddressed:
Guid.Parse(instanceId)at line 105 throwsFormatExceptionon invalid input, resulting in a 500 instead of 400.- When
customLayoutService.GetCustomLayoutForInstancereturns null (indicating "use default"), the code returnsOk(customLayout)with a null body instead of falling through to the fallback.Apply this diff:
+ if (!Guid.TryParse(instanceId, out var instanceGuid)) + { + return BadRequest("Invalid instanceId format"); + } + ICustomLayoutForInstance? customLayoutService = _appImplementationFactory.Get<ICustomLayoutForInstance>(); if (customLayoutService is not null) { string? customLayout = await customLayoutService.GetCustomLayoutForInstance( layoutSetId, instanceOwnerPartyId, - Guid.Parse(instanceId) + instanceGuid ); - return Ok(customLayout); + if (!string.IsNullOrEmpty(customLayout)) + { + return Ok(customLayout); + } } string layouts = _appResourceService.GetLayoutsForSet(layoutSetId); return Ok(layouts);
149-161: Same issues present in GetInstanceLayoutSettings.The same two issues exist here: unsafe
Guid.Parseat line 155 and missing null-fallback logic at line 157.Apply this diff:
+ if (!Guid.TryParse(instanceId, out var instanceGuid)) + { + return BadRequest("Invalid instanceId format"); + } + ICustomLayoutForInstance? customLayoutService = _appImplementationFactory.Get<ICustomLayoutForInstance>(); if (customLayoutService is not null) { string? customLayoutSettings = await customLayoutService.GetCustomLayoutSettingsForInstance( layoutSetId, instanceOwnerPartyId, - Guid.Parse(instanceId) + instanceGuid ); - return Ok(customLayoutSettings); + if (!string.IsNullOrEmpty(customLayoutSettings)) + { + return Ok(customLayoutSettings); + } } string? settings = _appResourceService.GetLayoutSettingsStringForSet(layoutSetId); return Ok(settings);
🧹 Nitpick comments (1)
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (1)
16-31: Test helper implementation is simple and effective.The
CustomLayoutForInstanceclass provides a straightforward test double. Consider making itsealedper coding guidelines for classes that don't need inheritance.- private class CustomLayoutForInstance : ICustomLayoutForInstance + private sealed class CustomLayoutForInstance : ICustomLayoutForInstance
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Altinn.App.Api/Controllers/ResourceController.cs(4 hunks)src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs(1 hunks)test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs(1 hunks)test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json(2 hunks)test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(1 hunks)test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs
- test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
🧠 Learnings (13)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
src/Altinn.App.Api/Controllers/ResourceController.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/test/**/*.cs : Prefer xUnit asserts over FluentAssertions in tests
Applied to files:
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🧬 Code graph analysis (2)
src/Altinn.App.Api/Controllers/ResourceController.cs (3)
src/Altinn.App.Api/Controllers/EventsReceiverController.cs (1)
Route(12-86)src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs (2)
Task(17-17)Task(25-25)src/Altinn.App.Core/Interface/IAppResources.cs (4)
Task(38-38)Task(135-135)GetLayoutsForSet(152-152)GetLayoutSettingsStringForSet(165-165)
test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (2)
src/Altinn.App.Core/Internal/App/ICustomLayoutForInstance.cs (2)
Task(17-17)Task(25-25)test/Altinn.App.Api.Tests/Data/TestData.cs (1)
PrepareInstance(147-174)
🔇 Additional comments (5)
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (2)
1151-1155: Public API snapshot – no action hereThis file is a generated public API snapshot used by
PublicApiTests. The addedAddInstanceIdentifierToLayoutRequestsconstant is correctly captured, and no manual changes or review actions are needed in this snapshot itself. Any design review should target the actualFeatureFlagssource file instead.
2937-2941: Public API snapshot forICustomLayoutForInstance– review in source, not snapshotThese lines just record the new
ICustomLayoutForInstanceinterface in the public API snapshot. Per project guidelines, this file should not be edited directly or used as the basis for API design feedback; review and changes belong in the corresponding source interface file.src/Altinn.App.Api/Controllers/ResourceController.cs (1)
1-26: Constructor and field additions look good.The injection of
IServiceProviderand resolution ofAppImplementationFactoryfollows the established pattern seen in other controllers likeEventsReceiverController. This is the correct approach for obtaining app-specific implementations.test/Altinn.App.Api.Tests/Controllers/ResourceController_CustomLayoutTests.cs (2)
33-83: Good test coverage for both custom service and fallback paths.The tests properly verify:
- Custom service returns the expected value when registered
- Fallback to
IAppResourcesreturns valid JSON structure when no custom service existsUsing xUnit asserts as per coding guidelines.
85-130: Layout settings tests look good.Tests appropriately cover both the custom service path and the fallback path for layout settings.
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
from
1b4246a to
9fb5195
Compare
Jondyr
force-pushed
the
feat/instance-contextual-layout-fetching
branch
from
9fb5195 to
0bfe1ba
Compare
|
3 participants
Description
Related PR: Altinn/app-frontend-react#3818
Adds new service
ICustomLayoutForInstance, and endpoint inResourcesController.When used together with the featureFlag
AddInstanceIdentifierToLayoutRequests, the frontend uses this new endpoint to fetch layouts. If the app implementsICustomLayoutForInstancethis service is called instead ofIAppResources, giving access to instance information when returning layouts.Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.