chore: global settings endpoints v9#18893
Conversation
lassopicasso
added
squad/utforming
📝 WalkthroughWalkthroughThis PR introduces a complete HTTP API and backing service to manage global layout set settings (validation-on-navigation and task-navigation configuration) in Altinn Studio Designer. The changes persist settings to a global Settings.json file in the repository layouts folder, accessed through a new repository layer and coordinated by a dedicated service. ChangesGlobal Layout Set Settings API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Designer/backend/src/Designer/Services/LayoutsetsService.cs (1)
28-32: 🏗️ Heavy liftAvoid splitting
AltinnRepoEditingContextinto org/repo/developer repeatedly.The service repeatedly unpacks context fields to call the factory. Prefer a factory overload that accepts
AltinnRepoEditingContextdirectly to keep call sites consistent and reduce parameter fan-out.As per coding guidelines, "Instead of passing the combination of
org,repo/appanddeveloper/userparameters into functions separately, look for suitable classes that join them into one, e.g. AltinnRepoEditingContext".Also applies to: 45-49, 84-88, 100-104, 118-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Designer/backend/src/Designer/Services/LayoutsetsService.cs` around lines 28 - 32, The code repeatedly unpacks AltinnRepoEditingContext into org/repo/developer when calling _altinnGitRepositoryFactory.GetAltinnAppGitRepository; add (or use) an overload on the factory that takes AltinnRepoEditingContext directly and update all call sites (the calls at the blocks creating AltinnAppGitRepository in LayoutsetsService methods) to pass the AltinnRepoEditingContext instance instead of individual fields, so callers use _altinnGitRepositoryFactory.GetAltinnAppGitRepository(altinnRepoEditingContext); keep existing behavior and tests by forwarding the new overload to the existing implementation that extracts Org/Repo/Developer internally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/Designer/backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs`:
- Around line 620-627: GetGlobalSettingsFile currently calls
ReadTextByRelativePathAsync and deserializes unconditionally which throws if
Settings.json is missing; update GetGlobalSettingsFile to first check for the
file (use GetPathToGlobalSettingsFile and the repository/file-exists helper or
catch the file-not-found error from ReadTextByRelativePathAsync), and if the
file does not exist return a new default UiSettings instance instead of
attempting to read/deserialize; ensure you still use s_jsonOptions when
deserializing existing content and preserve CancellationToken handling.
---
Nitpick comments:
In `@src/Designer/backend/src/Designer/Services/LayoutsetsService.cs`:
- Around line 28-32: The code repeatedly unpacks AltinnRepoEditingContext into
org/repo/developer when calling
_altinnGitRepositoryFactory.GetAltinnAppGitRepository; add (or use) an overload
on the factory that takes AltinnRepoEditingContext directly and update all call
sites (the calls at the blocks creating AltinnAppGitRepository in
LayoutsetsService methods) to pass the AltinnRepoEditingContext instance instead
of individual fields, so callers use
_altinnGitRepositoryFactory.GetAltinnAppGitRepository(altinnRepoEditingContext);
keep existing behavior and tests by forwarding the new overload to the existing
implementation that extracts Org/Repo/Developer internally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 177770e3-6cec-4719-8569-784a76afb97c
📒 Files selected for processing (4)
src/Designer/backend/src/Designer/Controllers/LayoutsetsController.cssrc/Designer/backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cssrc/Designer/backend/src/Designer/Services/Interfaces/ILayoutsetsService.cssrc/Designer/backend/src/Designer/Services/LayoutsetsService.cs
| public async Task<UiSettings> GetGlobalSettingsFile(CancellationToken cancellationToken = default) | ||
| { | ||
| string globalSettingsFilePath = GetPathToGlobalSettingsFile(); | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| string fileContent = await ReadTextByRelativePathAsync(globalSettingsFilePath, cancellationToken); | ||
| UiSettings globalSettingsFile = JsonSerializer.Deserialize<UiSettings>(fileContent, s_jsonOptions); | ||
| return globalSettingsFile; | ||
| } |
There was a problem hiding this comment.
Handle missing global settings file before reading.
GetGlobalSettingsFile reads Settings.json unconditionally. On first write scenario (file not created yet), this throws and breaks both save endpoints that expect default/empty settings to be possible.
Proposed fix
public async Task<UiSettings> GetGlobalSettingsFile(CancellationToken cancellationToken = default)
{
string globalSettingsFilePath = GetPathToGlobalSettingsFile();
cancellationToken.ThrowIfCancellationRequested();
+ if (!FileExistsByRelativePath(globalSettingsFilePath))
+ {
+ return new UiSettings();
+ }
string fileContent = await ReadTextByRelativePathAsync(globalSettingsFilePath, cancellationToken);
UiSettings globalSettingsFile = JsonSerializer.Deserialize<UiSettings>(fileContent, s_jsonOptions);
return globalSettingsFile;
}📝 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.
| public async Task<UiSettings> GetGlobalSettingsFile(CancellationToken cancellationToken = default) | |
| { | |
| string globalSettingsFilePath = GetPathToGlobalSettingsFile(); | |
| cancellationToken.ThrowIfCancellationRequested(); | |
| string fileContent = await ReadTextByRelativePathAsync(globalSettingsFilePath, cancellationToken); | |
| UiSettings globalSettingsFile = JsonSerializer.Deserialize<UiSettings>(fileContent, s_jsonOptions); | |
| return globalSettingsFile; | |
| } | |
| public async Task<UiSettings> GetGlobalSettingsFile(CancellationToken cancellationToken = default) | |
| { | |
| string globalSettingsFilePath = GetPathToGlobalSettingsFile(); | |
| cancellationToken.ThrowIfCancellationRequested(); | |
| if (!FileExistsByRelativePath(globalSettingsFilePath)) | |
| { | |
| return new UiSettings(); | |
| } | |
| string fileContent = await ReadTextByRelativePathAsync(globalSettingsFilePath, cancellationToken); | |
| UiSettings globalSettingsFile = JsonSerializer.Deserialize<UiSettings>(fileContent, s_jsonOptions); | |
| return globalSettingsFile; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/Designer/backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs`
around lines 620 - 627, GetGlobalSettingsFile currently calls
ReadTextByRelativePathAsync and deserializes unconditionally which throws if
Settings.json is missing; update GetGlobalSettingsFile to first check for the
file (use GetPathToGlobalSettingsFile and the repository/file-exists helper or
catch the file-not-found error from ReadTextByRelativePathAsync), and if the
file does not exist return a new default UiSettings instance instead of
attempting to read/deserialize; ensure you still use s_jsonOptions when
deserializing existing content and preserve CancellationToken handling.
There was a problem hiding this comment.
should i return null instead in this case?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of GetGlobalSettingsFile in the codebase
rg -n "GetGlobalSettingsFile" --type cs -C 5
Length of output: 6351
@lassopicasso Yes, returning null actually works equally well here — and arguably fits the existing caller patterns even better. Looking at all four call sites in LayoutsetsService:
| Call site | Handling of the returned value |
|---|---|
GetGlobalValidationOnNavigationSettings |
globalSettingsFile?.ValidationOnNavigation — null-safe |
SaveGlobalValidationOnNavigationSettings |
globalSettingsFile ??= new UiSettings(); — explicit null fallback |
GetGlobalTaskNavigationSettings |
globalSettingsFile?.TaskNavigation?.ToList() ?? [] — null-safe |
UpdateGlobalTaskNavigationSettings |
globalSettingsFile ??= new UiSettings(); — explicit null fallback |
Every caller already guards against null with either the ?. operator or ??=, so returning null when the file doesn't exist is semantically clean — it accurately signals "the file doesn't exist yet" and lets the service layer decide what the default should be. The proposed fix would then be:
public async Task<UiSettings> GetGlobalSettingsFile(CancellationToken cancellationToken = default)
{
string globalSettingsFilePath = GetPathToGlobalSettingsFile();
cancellationToken.ThrowIfCancellationRequested();
+ if (!FileExistsByRelativePath(globalSettingsFilePath))
+ {
+ return null;
+ }
string fileContent = await ReadTextByRelativePathAsync(globalSettingsFilePath, cancellationToken);
UiSettings globalSettingsFile = JsonSerializer.Deserialize<UiSettings>(fileContent, s_jsonOptions);
return globalSettingsFile;
}Either approach (null or new UiSettings()) will work, but null is the more idiomatic choice for a repository-layer method — it separates "no file" from "empty settings object" clearly.
🧠 Learnings used
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 17174
File: src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs:1-8
Timestamp: 2025-12-11T09:10:06.960Z
Learning: In C# 12, you can declare a class, interface, or struct with an empty body using a trailing semicolon as a shorthand (e.g., internal sealed class X : Y;). When reviewing StudioGateway.Api code under src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs and other C# files, verify the language version is set to at least 12.0+ and that any empty-body types use the single semicolon shorthand only when semantically appropriate (i.e., the type has no members). If a type previously had members, continue using a normal body.
Learnt from: danielskovli
Repo: Altinn/altinn-studio PR: 18623
File: src/Runtime/workflow-engine/src/WorkflowEngine.Models/Step.cs:47-57
Timestamp: 2026-04-29T10:40:01.618Z
Learning: When reviewing C# `record` types, do not require an `override` keyword on a manually declared `Equals(DerivedType? other)` method for a `sealed record` (e.g., `sealed record Step : PersistentItem`). In this case, the compiler uses the explicitly declared `Equals(DerivedType? other)` and does not synthesize a different one; it also is not overriding/inheriting an `Equals` method from the base record signature, so flagging “missing override” for this specific `Equals(DerivedType? other)` form is incorrect. Only flag override issues when the method actually overrides/inherits a base member with a matching signature.
github-actions
Bot
added
skip-releasenotes
1 participant
Description
Verification
Summary by CodeRabbit
Release Notes