Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a frontend feature flag, a new app-implementable interface for per-instance layouts, two instance-scoped layout endpoints in ResourceController wired via AppImplementationFactory, changes to data identifier null-handling and PDF filename logic, and accompanying tests, OpenAPI and metadata snapshot updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
03c5d90 to
1f10c35
Compare
e8df578 to
e56150a
Compare
21399d0 to
3dffd17
Compare
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.
3dffd17 to
271ca38
Compare
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
f307242 to
301807c
Compare
69d0c68 to
733ec02
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.
1b4246a to
9fb5195
Compare
9fb5195 to
0bfe1ba
Compare
b443d20 to
668ffdc
Compare
|
/publish |
ca5dfe8 to
33b9d3f
Compare
|
/publish |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Altinn.App.Core/Features/FeatureFlags.cs`:
- Around line 16-21: The XML summary for the public constant
AddInstanceIdentifierToLayoutRequests contains a TODO; replace it with a
concise, concrete summary describing the flag's behavior (e.g., that enabling
this feature changes the backend layout endpoint to include the instance
identifier in requests), update the <summary> tag text accordingly so generated
API docs are complete, and ensure wording is clear and present-tense without
TODO markers.
🧹 Nitpick comments (2)
src/Altinn.App.Api/Controllers/ResourceController.cs (2)
22-25: Consider injectingAppImplementationFactorydirectly.Using
IServiceProvider.GetRequiredService<T>()in the constructor is a service locator pattern. Prefer direct constructor injection for better testability and clearer dependencies.♻️ Proposed refactor
- /// <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; }
90-90: Consider adding GUID route constraint for consistency and early validation.The route uses
{instanceId}as a string, but existing instance endpoints in this codebase use{instanceGuid:guid}with a GUID constraint. Adding the constraint would reject invalid GUIDs at the routing level (returning 404) and maintain consistency with other instance-scoped endpoints.♻️ Proposed change
- [Route("{org}/{app}/instances/{instanceOwnerPartyId:int}/{instanceId}/layouts/{layoutSetId}")] + [Route("{org}/{app}/instances/{instanceOwnerPartyId:int}/{instanceGuid:guid}/layouts/{layoutSetId}")]Note: This would also require renaming the parameter in the method signature from
instanceIdtoinstanceGuidand updating the XML documentation accordingly.
| // TODO: write a better summary here | ||
| /// <summary> | ||
| /// Enabling this feature changes backend endpoint used for layouts to | ||
| /// add instance identifier. | ||
| /// </summary> | ||
| public const string AddInstanceIdentifierToLayoutRequests = "AddInstanceIdentifierToLayoutRequests"; |
There was a problem hiding this comment.
Replace the TODO with a concrete summary before release.
Leaving a TODO in a public API constant’s XML doc makes the documentation incomplete and leaks into generated docs.
📝 Suggested update
- // TODO: write a better summary here
- /// <summary>
- /// Enabling this feature changes backend endpoint used for layouts to
- /// add instance identifier.
- /// </summary>
+ /// <summary>
+ /// When enabled, layout fetches use instance-scoped endpoints so layout requests
+ /// include the instance identifier.
+ /// </summary>
public const string AddInstanceIdentifierToLayoutRequests = "AddInstanceIdentifierToLayoutRequests";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: write a better summary here | |
| /// <summary> | |
| /// Enabling this feature changes backend endpoint used for layouts to | |
| /// add instance identifier. | |
| /// </summary> | |
| public const string AddInstanceIdentifierToLayoutRequests = "AddInstanceIdentifierToLayoutRequests"; | |
| /// <summary> | |
| /// When enabled, layout fetches use instance-scoped endpoints so layout requests | |
| /// include the instance identifier. | |
| /// </summary> | |
| public const string AddInstanceIdentifierToLayoutRequests = "AddInstanceIdentifierToLayoutRequests"; |
🤖 Prompt for AI Agents
In `@src/Altinn.App.Core/Features/FeatureFlags.cs` around lines 16 - 21, The XML
summary for the public constant AddInstanceIdentifierToLayoutRequests contains a
TODO; replace it with a concise, concrete summary describing the flag's behavior
(e.g., that enabling this feature changes the backend layout endpoint to include
the instance identifier in requests), update the <summary> tag text accordingly
so generated API docs are complete, and ensure wording is clear and
present-tense without TODO markers.
|
/publish |
(cherry picked from commit cea36ab)
|



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
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.