Conversation
github-actions
bot
added
the
skip-releasenotes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request consolidates enriched process state into the instance endpoint by introducing Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Frontend Client
participant Controller as InstancesController
participant Enricher as ProcessStateEnricher
participant Reader as IProcessReader
participant Auth as IAuthorizationService
participant Instance as Instance Data
Client->>Controller: GET /instances/{partyId}/{guid}
Controller->>Instance: Retrieve instance
Note over Controller: Start concurrent tasks
Controller->>Enricher: Enrich(instance, processState, user)
Enricher->>Reader: Get current task element
Enricher->>Auth: AuthorizeActions(actions)
Auth-->>Enricher: Authorization decisions
Enricher->>Reader: Get all ProcessTask elements
Reader-->>Enricher: Task list
Enricher-->>Controller: AppProcessState (enriched)
Controller->>Controller: Build InstanceResponse
Controller-->>Client: InstanceResponse with enriched process
Note over Client,Auth: Process data now included in instance response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 inconclusive)
✅ 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 |
adamhaeger
changed the title
|
🔍 Lighthouse Report
Edited |
adamhaeger
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
src/App/frontend/src/core/queries/instance/index.ts (1)
56-69: Duplicated cache-lookup logic withuseCurrentInstance.The logic to find the instance entry in the cache (lines 60-61) duplicates the pattern in
useCurrentInstance(lines 26-28). Consider extracting this into a shared helper to avoid divergence.♻️ Proposed refactor
+function findInstanceCacheEntry(queryClient: QueryClient) { + const cached = queryClient.getQueriesData<IInstance>({ queryKey: instanceQueryKeys.all() }); + return cached.find(([key, data]) => data && key.length === 2 && typeof key[1] === 'object'); +} + function useCurrentInstance(): IInstance | undefined { const queryClient = useQueryClient(); - return queryClient - .getQueriesData<IInstance>({ queryKey: instanceQueryKeys.all() }) - .find(([key, data]) => data && key.length === 2 && typeof key[1] === 'object')?.[1]; + return findInstanceCacheEntry(queryClient)?.[1]; } function useOptimisticallyUpdateInstance() { const queryClient = useQueryClient(); return (updater: (oldData: IInstance) => IInstance) => { - const cached = queryClient.getQueriesData<IInstance>({ queryKey: instanceQueryKeys.all() }); - const entry = cached.find(([key, data]) => data && key.length === 2 && typeof key[1] === 'object'); + const entry = findInstanceCacheEntry(queryClient); if (entry) { const [queryKey, oldData] = entry; if (oldData) { queryClient.setQueryData(queryKey, updater(oldData)); } } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/frontend/src/core/queries/instance/index.ts` around lines 56 - 69, The cache-lookup logic in useOptimisticallyUpdateInstance duplicates the pattern in useCurrentInstance; extract the shared logic into a small helper (e.g., findInstanceCacheEntry or getCachedCurrentInstance) that accepts the QueryClient (or a getQueriesData function) and instanceQueryKeys.all() and returns the matching [queryKey, data] tuple (or undefined), then replace the duplicated lines in both useOptimisticallyUpdateInstance and useCurrentInstance to call that helper; ensure the helper checks key.length === 2 and typeof key[1] === 'object' and preserves the generic IInstance typing so queryClient.setQueryData(queryKey, updater(oldData)) continues to work in useOptimisticallyUpdateInstance.src/App/backend/test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
3788-3792: Confirm whetherProcessStateEnrichershould be part of the public contract.Line 3788 exposes
Altinn.App.Core.Internal.Process.ProcessStateEnricheras a public API type. If this is only an internal wiring detail, making it public in an.Internal.namespace can create long-term compatibility obligations. Please verify whether it can beinternal(or moved to a non-internal public namespace if it is intentionally supported).#!/bin/bash # Verify declaration and usage footprint before deciding on API visibility. # 1) Find the concrete declaration rg -nP 'class\s+ProcessStateEnricher\b|public\s+sealed\s+class\s+ProcessStateEnricher\b' # 2) Find constructor injections/usages to assess whether internal visibility is sufficient rg -nP '\bProcessStateEnricher\b' -g '!**/*.verified.txt' -C 2 # 3) Check if any public APIs expose ProcessStateEnricher directly in signatures rg -nP 'public\s+.*ProcessStateEnricher|ProcessStateEnricher\s+\w+\s*[\),]' src/App/backend/src -C 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/backend/test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt` around lines 3788 - 3792, The review flags ProcessStateEnricher (type ProcessStateEnricher in namespace Altinn.App.Core.Internal.Process) being publicly exposed; decide whether it should be internal or intentionally part of the public contract and update its visibility accordingly: if it is only used for internal wiring (constructor ProcessStateEnricher(Altinn.App.Core.Internal.Process.IProcessReader, Altinn.App.Core.Internal.Auth.IAuthorizationService) and method Enrich(...)), change the class declaration to internal sealed class ProcessStateEnricher and remove any public API signatures that directly expose it; if it must remain public, move it out of the .Internal. namespace into a supported public namespace and ensure public APIs that reference it remain stable. Ensure all references/usages (constructor calls and Enrich usages) are updated to compile after the visibility/namespace change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App/backend/src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 647-648: PostSimplified constructs the response from the
unfiltered instance; apply the same data-element filtering used elsewhere by
calling instance.WithOnlyAccessibleDataElements(_dataElementAccessChecker)
before building the DTO. Locate PostSimplified where processState is obtained
via _processStateEnricher.Enrich and replace (or wrap) the instance passed to
InstanceResponse.From(...) with the result of
instance.WithOnlyAccessibleDataElements(_dataElementAccessChecker) so the
response uses the filtered instance consistent with Post, Get, and
DataController.
In `@src/App/backend/src/Altinn.App.Api/Controllers/ProcessController.cs`:
- Around line 149-153: Remove the unnecessary null-conditional operators when
accessing ProcessChangeResult.ProcessStateChange in StartProcess and
NextElement: replace result.ProcessStateChange?.Events with
result.ProcessStateChange.Events and replace
result.ProcessStateChange?.NewProcessState with
result.ProcessStateChange.NewProcessState, because the method checks
result.Success and the type uses [MemberNotNullWhen(true,
nameof(ProcessStateChange))]; update the accesses in the StartProcess and
NextElement methods (and mirror the pattern used at the other occurrence) so the
code consistently reflects the non-null contract.
In `@src/App/backend/src/Altinn.App.Api/Models/InstanceResponse.cs`:
- Around line 51-53: The InstanceResponse DTO was changed to expose an enriched
CoreAppProcessState via the Process property which leaks an internal/enriched
shape into the public contract; revert the Process property on InstanceResponse
back to its original, shared contract (remove the
AppProcessState/CoreAppProcessState replacement) and keep the enriched payload
behind a new separate endpoint or DTO (e.g., create a dedicated enriched
response type and endpoint that returns CoreAppProcessState), ensuring
InstanceResponse.Process uses the original public type and does not reference
CoreAppProcessState or AppProcessState.
In
`@src/App/backend/src/Altinn.App.Core/Internal/Process/ProcessStateEnricher.cs`:
- Around line 46-55: The actions list built in ProcessStateEnricher (the local
variable actions that seeds new AltinnAction("read"), new("write") and then adds
processTask.ExtensionElements?.TaskExtension?.AltinnActions) can contain
duplicate Ids which causes ToDictionary and Single calls to throw; fix by
deduplicating action IDs before calling
_authorization.AuthorizeActions(instance, user, actions) (e.g. collapse
AltinnActions into the seeded list by Id so each action.Id is unique), then pass
the deduplicated list to _authorization.AuthorizeActions and continue assigning
appProcessState.CurrentTask.Actions, HasReadAccess and HasWriteAccess from the
returned authDecisions.
In
`@src/App/backend/test/Altinn.App.Api.Tests/Controllers/DataController_RequiredActionTests.cs`:
- Around line 277-281: The batched authorization mock on authorizationClientMock
for AuthorizeActions currently grants true for all actions; update it to mirror
the single-action logic used by AuthorizeAction by inspecting the incoming
actions list and returning hasOrgAuthorization-dependent results for
customReadAction and customWriteAction (i.e., consult the same
hasOrgAuthorization flag used by AuthorizeAction and return true/false
accordingly), so ProcessStateEnricher.cs and other callers using
AuthorizeActions get consistent behavior with the single-action path.
In
`@src/App/backend/test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs`:
- Around line 136-138: The fixture registers loose mocks that return null for
critical calls; update the setup for the Mock<IProcessReader> and
Mock<IAuthorizationService> before calling .Object so they return safe defaults:
configure IProcessReader.GetAllFlowElements() to return an empty IEnumerable
(not null) and configure IAuthorizationService.AuthorizeActions(...) to return a
completed Task/result representing an empty/neutral authorization response (use
ReturnsAsync or Task.FromResult as appropriate). Keep the registration of
ProcessStateEnricher the same, but replace services.AddSingleton(new
Mock<IProcessReader>(...).Object) and services.AddSingleton(new
Mock<IAuthorizationService>(...).Object) with instances where the mocks are
created, set up (GetAllFlowElements and AuthorizeActions), then registered via
mock.Object so ProcessStateEnricher.Enrich() and its calls to
GetAllFlowElements() and AuthorizeActions() never encounter null.
In
`@src/App/backend/test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json`:
- Around line 8688-8690: The change replaces InstanceResponse.process type from
ProcessState to AppProcessState which breaks the public response contract for
the GET /instances/{instanceOwnerPartyId}/{instanceGuid} endpoint; revert
InstanceResponse.process back to ProcessState to preserve backward compatibility
and, if you need the enriched AppProcessState data, add a new field or a
separate response schema/endpoint (e.g., InstanceResponseV2 or
/instances/{...}/details) that returns AppProcessState while leaving the
original InstanceResponse and ProcessState intact.
In `@src/App/frontend/src/layout/CustomButton/CustomButtonComponent.test.tsx`:
- Around line 128-167: The test mock in CustomButtonComponent.test.tsx sets
currentTask.flowType = 'CompleteCurrentMoveToNext', but that property isn't
declared on the ITask type; either remove flowType from the mock or add it to
the ITask interface in src/App/frontend/src/types/shared.ts. Locate the mock
inside the jest.mocked(InstanceApi.getInstance).mockImplementation block and
either delete the flowType line if it’s not required for assertions, or add
flowType?: string (or a proper union type like 'CompleteCurrentMoveToNext') to
the ITask definition so the mock conforms to the interface.
In `@src/App/frontend/src/routes/task/task.loader.ts`:
- Around line 8-14: The loader is incorrectly awaiting prefetchInstanceData
which contradicts the "fire-and-forget" comment and can cause navigation
failures if queryClient.prefetchQuery rejects; change the loader so it calls
prefetchInstanceData(instanceOwnerPartyId, instanceGuid) without await and
ensure the returned promise is swallowed (e.g., call it without awaiting and
attach a noop .catch to prevent rejections from bubbling) so warm-up remains
best-effort; update references in the loader function and ensure
prefetchInstanceData/queryClient.prefetchQuery errors are not allowed to reject
the loader.
In `@src/Runtime/workflow-engine/docs/presentation-technical.md`:
- Line 93: The prose in the table cell for the row "Timeout during auto-loop
iteration N" uses the range notation "Iterations 1..N-1 persisted. Iteration N
partially executed." which looks like accidental double punctuation; update that
phrase to a clearer range format such as "Iterations 1 to N-1 persisted.
Iteration N partially executed." (or "Iterations 1–(N-1) persisted...") so the
slide reads cleanly—change the text in the table cell containing "Iterations
1..N-1 persisted. Iteration N partially executed." accordingly.
In `@src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/style.css`:
- Line 1279: Search the stylesheet for any occurrences of the deprecated
declaration "word-break: break-word" and replace each with "overflow-wrap:
break-word" (preserving any related vendor prefixes or other declarations in the
same rule); ensure the replacement is applied inside the same selector block so
layout/line-wrapping behavior remains unchanged.
---
Nitpick comments:
In
`@src/App/backend/test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt`:
- Around line 3788-3792: The review flags ProcessStateEnricher (type
ProcessStateEnricher in namespace Altinn.App.Core.Internal.Process) being
publicly exposed; decide whether it should be internal or intentionally part of
the public contract and update its visibility accordingly: if it is only used
for internal wiring (constructor
ProcessStateEnricher(Altinn.App.Core.Internal.Process.IProcessReader,
Altinn.App.Core.Internal.Auth.IAuthorizationService) and method Enrich(...)),
change the class declaration to internal sealed class ProcessStateEnricher and
remove any public API signatures that directly expose it; if it must remain
public, move it out of the .Internal. namespace into a supported public
namespace and ensure public APIs that reference it remain stable. Ensure all
references/usages (constructor calls and Enrich usages) are updated to compile
after the visibility/namespace change.
In `@src/App/frontend/src/core/queries/instance/index.ts`:
- Around line 56-69: The cache-lookup logic in useOptimisticallyUpdateInstance
duplicates the pattern in useCurrentInstance; extract the shared logic into a
small helper (e.g., findInstanceCacheEntry or getCachedCurrentInstance) that
accepts the QueryClient (or a getQueriesData function) and
instanceQueryKeys.all() and returns the matching [queryKey, data] tuple (or
undefined), then replace the duplicated lines in both
useOptimisticallyUpdateInstance and useCurrentInstance to call that helper;
ensure the helper checks key.length === 2 and typeof key[1] === 'object' and
preserves the generic IInstance typing so queryClient.setQueryData(queryKey,
updater(oldData)) continues to work in useOptimisticallyUpdateInstance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8f38351-0467-4e53-aa47-f300683715f0
📒 Files selected for processing (63)
.github/scripts/codeowners-generate.mjsREADME.mdsrc/App/backend/src/Altinn.App.Api/Controllers/InstancesController.cssrc/App/backend/src/Altinn.App.Api/Controllers/ProcessController.cssrc/App/backend/src/Altinn.App.Api/Models/InstanceResponse.cssrc/App/backend/src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/App/backend/src/Altinn.App.Core/Internal/Process/ProcessStateEnricher.cssrc/App/backend/test/Altinn.App.Api.Tests/Controllers/DataController_RequiredActionTests.cssrc/App/backend/test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cssrc/App/backend/test/Altinn.App.Api.Tests/Controllers/InstancesController_GetTests.ReturnsOkResult_Deserialized.verified.txtsrc/App/backend/test/Altinn.App.Api.Tests/Controllers/InstancesController_GetTests.ReturnsOkResult_Raw.verified.txtsrc/App/backend/test/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.PostNewInstance_Simplified_SelfIdentifiedUser.verified.txtsrc/App/backend/test/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.PostNewInstance_Simplified_ServiceOwner.verified.txtsrc/App/backend/test/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.PostNewInstance_Simplified_SystemUser.verified.txtsrc/App/backend/test/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstanceTests.PostNewInstance_Simplified_User.verified.txtsrc/App/backend/test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.jsonsrc/App/backend/test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/App/backend/test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/App/frontend/src/components/wrappers/ProcessWrapper.tsxsrc/App/frontend/src/core/queries/instance/index.tssrc/App/frontend/src/core/queries/instance/instance.queries.tssrc/App/frontend/src/features/devtools/components/PermissionsEditor/PermissionsEditor.tsxsrc/App/frontend/src/features/expressions/shared-functions.test.tsxsrc/App/frontend/src/features/formData/FormDataWriteStateMachine.tsxsrc/App/frontend/src/features/instance/InstanceContext.tsxsrc/App/frontend/src/features/instance/instanceUtils.tssrc/App/frontend/src/features/instance/useProcessNext.tsxsrc/App/frontend/src/features/instance/useProcessQuery.tssrc/App/frontend/src/features/instantiate/containers/InstantiateContainer.tsxsrc/App/frontend/src/features/pdf/PDFWrapper.test.tsxsrc/App/frontend/src/features/process/feedback/Feedback.tsxsrc/App/frontend/src/features/receipt/ReceiptContainer.test.tsxsrc/App/frontend/src/layout/CustomButton/CustomButtonComponent.test.tsxsrc/App/frontend/src/queries/queries.tssrc/App/frontend/src/queries/types.tssrc/App/frontend/src/router.tsxsrc/App/frontend/src/routes/instance/instance.loader.tssrc/App/frontend/src/routes/task/task.loader.tssrc/App/frontend/src/setupTests.tssrc/App/frontend/src/types/shared.tssrc/App/frontend/src/utils/layout/all.test.tsxsrc/App/frontend/src/utils/urls/appUrlHelper.test.tssrc/App/frontend/src/utils/urls/appUrlHelper.tssrc/Runtime/workflow-engine-app/AGENTS.mdsrc/Runtime/workflow-engine-app/README.mdsrc/Runtime/workflow-engine/.claude/skills/docker/SKILL.mdsrc/Runtime/workflow-engine/.claude/skills/format/SKILL.mdsrc/Runtime/workflow-engine/.claude/skills/k6/SKILL.mdsrc/Runtime/workflow-engine/README.mdsrc/Runtime/workflow-engine/docs/db-connections-notes.mdsrc/Runtime/workflow-engine/docs/presentation-technical.mdsrc/Runtime/workflow-engine/docs/proposals/recurring-workflows.mdsrc/Runtime/workflow-engine/docs/proposals/workflow-cancel-restart.mdsrc/Runtime/workflow-engine/docs/technical-guide.mdsrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/AGENTS.mdsrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/DASHBOARD_SPEC.mdsrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/app.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/core/state.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/filters.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/modal.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/features/url.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/modules/shared/dropdown.jssrc/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/style.css
💤 Files with no reviewable changes (5)
- src/App/frontend/src/utils/layout/all.test.tsx
- src/App/frontend/src/queries/queries.ts
- src/App/frontend/src/utils/urls/appUrlHelper.test.ts
- src/App/frontend/src/utils/urls/appUrlHelper.ts
- src/App/frontend/src/routes/instance/instance.loader.ts
| var processState = await _processStateEnricher.Enrich(instance, instance.Process, User); | ||
| var dto = InstanceResponse.From(instance, party, processState); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if WithOnlyAccessibleDataElements is used consistently across instance creation endpoints
# Expected: Understand if PostSimplified intentionally skips this check or if it's an oversight
rg -n "WithOnlyAccessibleDataElements" --type=cs -C 3Repository: Altinn/altinn-studio
Length of output: 5097
🏁 Script executed:
# Read the context around lines 647-648 to see the method signature and surrounding code
sed -n '640,655p' src/App/backend/src/Altinn.App.Api/Controllers/InstancesController.csRepository: Altinn/altinn-studio
Length of output: 723
🏁 Script executed:
# Search for PostSimplified method definition to understand its purpose
rg -n "PostSimplified" --type=cs -B 5 -A 15 src/App/backend/src/Altinn.App.Api/Controllers/InstancesController.csRepository: Altinn/altinn-studio
Length of output: 944
🏁 Script executed:
# Read the complete PostSimplified method starting from line 464
sed -n '464,650p' src/App/backend/src/Altinn.App.Api/Controllers/InstancesController.csRepository: Altinn/altinn-studio
Length of output: 7098
🏁 Script executed:
# Check for any comments or documentation in PostSimplified or around it
sed -n '450,470p' src/App/backend/src/Altinn.App.Api/Controllers/InstancesController.csRepository: Altinn/altinn-studio
Length of output: 1104
🏁 Script executed:
# Search for any other references to PostSimplified that might explain design intent
rg -n "PostSimplified" --type=cs src/App/backend/src/Altinn.App.Api/Repository: Altinn/altinn-studio
Length of output: 207
🏁 Script executed:
# Find and read the regular Post method documentation and signature
rg -n "public async Task<ActionResult<InstanceResponse>> Post\(" --type=cs -B 20 -A 5 src/App/backend/src/Altinn.App.Api/Controllers/InstancesController.cs | head -50Repository: Altinn/altinn-studio
Length of output: 1580
Apply WithOnlyAccessibleDataElements to PostSimplified for consistency with other instance endpoints.
The Post endpoint applies instance.WithOnlyAccessibleDataElements(_dataElementAccessChecker) before constructing the response (line 419), as do the Get endpoint and DataController. The PostSimplified method should apply the same filtering to ensure data elements are restricted consistently across all instance-creation endpoints. Currently, lines 647–648 construct the response directly from the unfiltered instance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App/backend/src/Altinn.App.Api/Controllers/InstancesController.cs` around
lines 647 - 648, PostSimplified constructs the response from the unfiltered
instance; apply the same data-element filtering used elsewhere by calling
instance.WithOnlyAccessibleDataElements(_dataElementAccessChecker) before
building the DTO. Locate PostSimplified where processState is obtained via
_processStateEnricher.Enrich and replace (or wrap) the instance passed to
InstanceResponse.From(...) with the result of
instance.WithOnlyAccessibleDataElements(_dataElementAccessChecker) so the
response uses the filtered instance consistent with Post, Get, and
DataController.
| AppProcessState appProcessState = await _processStateEnricher.Enrich( | ||
| instance, | ||
| result.ProcessStateChange?.NewProcessState | ||
| result.ProcessStateChange?.NewProcessState, | ||
| User | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the ProcessChangeResult type to understand if ProcessStateChange can be null when Success is true
# Search for ProcessChangeResult class definition
ast-grep --pattern $'class ProcessChangeResult {
$$$
}'
# Also check if there's nullability annotation on ProcessStateChange property
rg -n "ProcessStateChange" --type cs -C3 | head -80Repository: Altinn/altinn-studio
Length of output: 7856
🏁 Script executed:
sed -n '140,160p' src/App/backend/src/Altinn.App.Api/Controllers/ProcessController.csRepository: Altinn/altinn-studio
Length of output: 895
🏁 Script executed:
sed -n '275,295p' src/App/backend/src/Altinn.App.Api/Controllers/ProcessController.csRepository: Altinn/altinn-studio
Length of output: 740
Remove unnecessary null-conditional operators from lines 147 and 151.
Both StartProcess and NextElement methods check result.Success before accessing ProcessStateChange, and the ProcessChangeResult type uses [MemberNotNullWhen(true, nameof(ProcessStateChange))] to guarantee non-null access when Success is true.
- Line 147:
result.ProcessStateChange?.Eventsshould beresult.ProcessStateChange.Events - Line 151:
result.ProcessStateChange?.NewProcessStateshould beresult.ProcessStateChange.NewProcessState
Line 287 correctly follows this pattern without the null-conditional operator. The current inconsistency obscures the actual contract and reduces code clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App/backend/src/Altinn.App.Api/Controllers/ProcessController.cs` around
lines 149 - 153, Remove the unnecessary null-conditional operators when
accessing ProcessChangeResult.ProcessStateChange in StartProcess and
NextElement: replace result.ProcessStateChange?.Events with
result.ProcessStateChange.Events and replace
result.ProcessStateChange?.NewProcessState with
result.ProcessStateChange.NewProcessState, because the method checks
result.Success and the type uses [MemberNotNullWhen(true,
nameof(ProcessStateChange))]; update the accesses in the StartProcess and
NextElement methods (and mirror the pattern used at the other occurrence) so the
code consistently reflects the non-null contract.
| var actions = new List<AltinnAction> { new("read"), new("write") }; | ||
| actions.AddRange( | ||
| processTask.ExtensionElements?.TaskExtension?.AltinnActions ?? new List<AltinnAction>() | ||
| ); | ||
| var authDecisions = await _authorization.AuthorizeActions(instance, user, actions); | ||
| appProcessState.CurrentTask.Actions = authDecisions | ||
| .Where(a => a.ActionType == ActionType.ProcessAction) | ||
| .ToDictionary(a => a.Id, a => a.Authorized); | ||
| appProcessState.CurrentTask.HasReadAccess = authDecisions.Single(a => a.Id == "read").Authorized; | ||
| appProcessState.CurrentTask.HasWriteAccess = authDecisions.Single(a => a.Id == "write").Authorized; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/App/backend/src/Altinn.App.Core/Internal/Process/ProcessStateEnricher.cs | head -100Repository: Altinn/altinn-studio
Length of output: 4300
🏁 Script executed:
# Check the file size first
wc -l src/App/backend/src/Altinn.App.Core/Internal/Process/ProcessStateEnricher.csRepository: Altinn/altinn-studio
Length of output: 143
🏁 Script executed:
# Search for how actions are handled elsewhere in the codebase
rg "ToDictionary.*ActionType|AltinnActions" --type cs -A 2 -B 2Repository: Altinn/altinn-studio
Length of output: 8709
🏁 Script executed:
# Look for any existing validation or deduplication of action IDs
rg "action.*id|duplicate|distinct" --type cs -i | grep -i actionRepository: Altinn/altinn-studio
Length of output: 50376
🏁 Script executed:
# Find the IAuthorizationService interface and AuthorizeActions method
rg "interface IAuthorizationService|AuthorizeActions" --type cs -A 3 | head -80Repository: Altinn/altinn-studio
Length of output: 8774
🏁 Script executed:
# Check test files to see if they test duplicate action scenarios
rg "ProcessStateEnricher" --type cs -A 20 | head -100Repository: Altinn/altinn-studio
Length of output: 11591
🏁 Script executed:
# Look at the AuthorizeActions implementation
fd "IAuthorizationService.cs|AuthorizationService.cs" --type fRepository: Altinn/altinn-studio
Length of output: 469
🏁 Script executed:
# Check what MultiDecisionHelper does - seems to handle actions
cat -n src/App/backend/src/Altinn.App.Core/Helpers/MultiDecisionHelper.cs | head -150Repository: Altinn/altinn-studio
Length of output: 6646
🏁 Script executed:
# Look at the full AuthorizationService.AuthorizeActions implementation
cat -n src/App/backend/src/Altinn.App.Core/Internal/Auth/AuthorizationService.cs | grep -A 40 "public async Task<List<UserAction>> AuthorizeActions"Repository: Altinn/altinn-studio
Length of output: 1699
🏁 Script executed:
# Check the IAuthorizationService interface to see the exact return type
cat -n src/App/backend/src/Altinn.App.Core/Internal/Auth/IAuthorizationService.cs | grep -A 10 "AuthorizeActions"Repository: Altinn/altinn-studio
Length of output: 194
🏁 Script executed:
# Check if there are any tests that verify duplicate action handling
rg "read.*write|duplicate.*action" --type cs src/App/backend/test | head -40Repository: Altinn/altinn-studio
Length of output: 3811
🏁 Script executed:
# Look for test files for ProcessStateEnricher
fd "ProcessStateEnricher.*Test" --type fRepository: Altinn/altinn-studio
Length of output: 46
🏁 Script executed:
# Look for any tests that might test ProcessStateEnricher
rg "ProcessStateEnricher" --type cs -lRepository: Altinn/altinn-studio
Length of output: 440
🏁 Script executed:
# Check if there's any deduplication logic in the authorization flow
rg "Distinct|distinct|HashSet|RemoveDuplicates" --type cs src/App/backend/src/Altinn.App.Core/Internal | head -20Repository: Altinn/altinn-studio
Length of output: 765
🏁 Script executed:
# Check the UserAction class to see its structure
fd "UserAction.cs" --type f -x cat -n {}Repository: Altinn/altinn-studio
Length of output: 20346
🏁 Script executed:
# Check test data to see how tasks define their actions
rg "AltinnActions.*=.*new|new.*AltinnAction" --type cs src/App/backend/test -A 1 | head -80Repository: Altinn/altinn-studio
Length of output: 5721
🏁 Script executed:
# Look at actual BPMN or process definition examples to see if read/write can be defined by apps
fd "*.bpmn" --type f | head -5Repository: Altinn/altinn-studio
Length of output: 349
🏁 Script executed:
# Check if there are any integration tests that exercise ProcessStateEnricher
rg "ProcessStateEnricher|CurrentTask.*Actions" --type cs src/App/backend/test -B 2 -A 8 | head -100Repository: Altinn/altinn-studio
Length of output: 1547
🏁 Script executed:
# Look at InstancesControllerFixture test to understand how ProcessStateEnricher is tested
cat -n src/App/backend/test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs | grep -A 30 "ProcessStateEnricher"Repository: Altinn/altinn-studio
Length of output: 1264
🏁 Script executed:
# Check if there are unit tests specifically for ProcessStateEnricher
rg "ProcessStateEnricher" --type cs src/App/backend/test -B 5 -A 15 | head -150Repository: Altinn/altinn-studio
Length of output: 2956
🏁 Script executed:
# Examine the AltinnAction class to confirm its properties
cat -n src/App/backend/src/Altinn.App.Core/Internal/Process/Elements/AltinnExtensionProperties/AltinnAction.csRepository: Altinn/altinn-studio
Length of output: 2360
🏁 Script executed:
# Confirm that authDecisions is a Dictionary<string, bool> and thus would also fail with duplicate keys
cat -n src/App/backend/src/Altinn.App.Core/Internal/Auth/IAuthorizationClient.cs | grep -A 3 "AuthorizeActions"Repository: Altinn/altinn-studio
Length of output: 295
🏁 Script executed:
# Check if there's any deduplication or validation before calling AuthorizeActions in ProcessStateEnricher
cat -n src/App/backend/src/Altinn.App.Core/Internal/Process/ProcessStateEnricher.cs | sed -n '40,60p'Repository: Altinn/altinn-studio
Length of output: 1521
Deduplicating action IDs prevents runtime exceptions in permission mapping.
If a task's BPMN extension defines read, write, or any action ID already in the default set, the combined actions list contains duplicates. When authDecisions (a List<UserAction>) is passed to ToDictionary(a => a.Id, ...) on line 53, duplicate keys cause an ArgumentException, resulting in a 500 error on process state requests. Deduplicating before authorisation ensures each action ID appears exactly once, allowing both ToDictionary and the Single() calls on lines 54–55 to execute safely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App/backend/src/Altinn.App.Core/Internal/Process/ProcessStateEnricher.cs`
around lines 46 - 55, The actions list built in ProcessStateEnricher (the local
variable actions that seeds new AltinnAction("read"), new("write") and then adds
processTask.ExtensionElements?.TaskExtension?.AltinnActions) can contain
duplicate Ids which causes ToDictionary and Single calls to throw; fix by
deduplicating action IDs before calling
_authorization.AuthorizeActions(instance, user, actions) (e.g. collapse
AltinnActions into the seeded list by Id so each action.Id is unique), then pass
the deduplicated list to _authorization.AuthorizeActions and continue assigning
appProcessState.CurrentTask.Actions, HasReadAccess and HasWriteAccess from the
returned authDecisions.
| authorizationClientMock | ||
| .Setup(x => x.AuthorizeActions(It.IsAny<Instance>(), It.IsAny<ClaimsPrincipal>(), It.IsAny<List<string>>())) | ||
| .ReturnsAsync( | ||
| (Instance _, ClaimsPrincipal _, List<string> actions) => actions.ToDictionary(a => a, _ => true) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/App/backend/test/Altinn.App.Api.Tests/Controllers/DataController_RequiredActionTests.csRepository: Altinn/altinn-studio
Length of output: 159
🏁 Script executed:
sed -n '260,295n' src/App/backend/test/Altinn.App.Api.Tests/Controllers/DataController_RequiredActionTests.csRepository: Altinn/altinn-studio
Length of output: 46
🏁 Script executed:
# Find the test method containing these lines to get full context
rg -A 50 -B 20 "AuthorizeActions" src/App/backend/test/Altinn.App.Api.Tests/Controllers/DataController_RequiredActionTests.cs | head -100Repository: Altinn/altinn-studio
Length of output: 1295
🏁 Script executed:
sed -n '200,285n' src/App/backend/test/Altinn.App.Api.Tests/Controllers/DataController_RequiredActionTests.csRepository: Altinn/altinn-studio
Length of output: 46
🏁 Script executed:
# Let's read the entire file to understand the test structure
cat -n src/App/backend/test/Altinn.App.Api.Tests/Controllers/DataController_RequiredActionTests.csRepository: Altinn/altinn-studio
Length of output: 13548
🏁 Script executed:
# Find if AuthorizeActions is actually called in production code
rg "AuthorizeActions" src/App/backend --type cs -g "!test" -A 3 -B 3Repository: Altinn/altinn-studio
Length of output: 8385
🏁 Script executed:
# Check if there are any other test files that test AuthorizeActions consistency
rg "AuthorizeActions" src/App/backend/test --type cs -lRepository: Altinn/altinn-studio
Length of output: 418
🏁 Script executed:
# Check how AuthorizeActions is mocked in AuthorizationMock.cs
cat -n src/App/backend/test/Altinn.App.Api.Tests/Mocks/AuthorizationMock.csRepository: Altinn/altinn-studio
Length of output: 3365
Align the batched authorisation mock with the single-action path.
The AuthorizeActions mock currently returns true for all actions unconditionally, whereas AuthorizeAction respects the hasOrgAuthorization parameter for customReadAction and customWriteAction. This inconsistency contradicts the mock pattern used elsewhere (e.g., AuthorizationMock.cs) and can hide regressions as production code in ProcessStateEnricher.cs actively uses the batched API to authorise process task actions.
Suggested adjustment
authorizationClientMock
.Setup(x => x.AuthorizeActions(It.IsAny<Instance>(), It.IsAny<ClaimsPrincipal>(), It.IsAny<List<string>>()))
.ReturnsAsync(
- (Instance _, ClaimsPrincipal _, List<string> actions) => actions.ToDictionary(a => a, _ => true)
+ (Instance _, ClaimsPrincipal _, List<string> actions) =>
+ actions.ToDictionary(
+ action => action,
+ action => action switch
+ {
+ "customReadAction" => hasOrgAuthorization,
+ "customWriteAction" => hasOrgAuthorization,
+ _ => true,
+ }
+ )
);📝 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.
| authorizationClientMock | |
| .Setup(x => x.AuthorizeActions(It.IsAny<Instance>(), It.IsAny<ClaimsPrincipal>(), It.IsAny<List<string>>())) | |
| .ReturnsAsync( | |
| (Instance _, ClaimsPrincipal _, List<string> actions) => actions.ToDictionary(a => a, _ => true) | |
| ); | |
| authorizationClientMock | |
| .Setup(x => x.AuthorizeActions(It.IsAny<Instance>(), It.IsAny<ClaimsPrincipal>(), It.IsAny<List<string>>())) | |
| .ReturnsAsync( | |
| (Instance _, ClaimsPrincipal _, List<string> actions) => | |
| actions.ToDictionary( | |
| action => action, | |
| action => action switch | |
| { | |
| "customReadAction" => hasOrgAuthorization, | |
| "customWriteAction" => hasOrgAuthorization, | |
| _ => true, | |
| } | |
| ) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/App/backend/test/Altinn.App.Api.Tests/Controllers/DataController_RequiredActionTests.cs`
around lines 277 - 281, The batched authorization mock on
authorizationClientMock for AuthorizeActions currently grants true for all
actions; update it to mirror the single-action logic used by AuthorizeAction by
inspecting the incoming actions list and returning hasOrgAuthorization-dependent
results for customReadAction and customWriteAction (i.e., consult the same
hasOrgAuthorization flag used by AuthorizeAction and return true/false
accordingly), so ProcessStateEnricher.cs and other callers using
AuthorizeActions get consistent behavior with the single-action path.
...d/test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json
Show resolved
Hide resolved
| jest.mocked(InstanceApi.getInstance).mockImplementation(async () => ({ | ||
| ...getInstanceDataMock(), | ||
| process: { | ||
| started: '2024-01-03T06:52:49.716640678Z', | ||
| ended: null, | ||
| validated: null, | ||
| flowType: 'CompleteCurrentMoveToNext', | ||
| endEvent: null, | ||
| startEvent: '2024-01-03T06:52:49.716640678Z', | ||
| currentTask: { | ||
| userActions: [ | ||
| { | ||
| id: 'read', | ||
| authorized: true, | ||
| type: 'ProcessAction', | ||
| }, | ||
| { | ||
| id: 'write', | ||
| authorized: true, | ||
| type: 'ProcessAction', | ||
| }, | ||
| { | ||
| id: 'complete', | ||
| authorized: false, | ||
| type: 'ProcessAction', | ||
| }, | ||
| ...(actionAuthorization ?? []), | ||
| ], | ||
| read: true, | ||
| write: true, | ||
| flow: 2, | ||
| started: '2024-01-03T06:37:22.7573522Z', | ||
| elementId: 'Task_1', | ||
| elementType: 'Task', | ||
| name: 'Utfylling', | ||
| altinnTaskType: 'data', | ||
| ended: null, | ||
| validated: null, | ||
| flowType: 'CompleteCurrentMoveToNext', | ||
| }, | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if flowType is defined in the ITask interface or related types
rg -n 'flowType' --type=ts src/App/frontend/src/types/Repository: Altinn/altinn-studio
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for flowType in the entire frontend codebase
rg -n 'flowType' src/App/frontend/ --type=ts --type=tsxRepository: Altinn/altinn-studio
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Find and examine the ITask interface definition
fd -e ts -e tsx 'shared' src/App/frontend/ | head -20Repository: Altinn/altinn-studio
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Look for the ITask interface definition
rg -n 'interface ITask|type ITask' src/App/frontend/ --type=ts --type=tsx -A 15Repository: Altinn/altinn-studio
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Read the shared.ts file to find ITask interface
cat -n src/App/frontend/src/types/shared.ts | head -100Repository: Altinn/altinn-studio
Length of output: 3055
🏁 Script executed:
#!/bin/bash
# Search for flowType without type filter
rg -n 'flowType' src/App/frontend/Repository: Altinn/altinn-studio
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Check the test file around line 164
sed -n '150,170p' src/App/frontend/src/layout/CustomButton/CustomButtonComponent.test.tsxRepository: Altinn/altinn-studio
Length of output: 568
🏁 Script executed:
#!/bin/bash
# Continue reading shared.ts to find ITask and IProcess interfaces
sed -n '100,300p' src/App/frontend/src/types/shared.tsRepository: Altinn/altinn-studio
Length of output: 4978
🏁 Script executed:
#!/bin/bash
# Search for IProcess interface
rg -n 'interface IProcess|type IProcess' src/App/frontend/Repository: Altinn/altinn-studio
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Check if there are any TypeScript configuration files that might affect strict checking
find src/App/frontend -maxdepth 2 -name 'tsconfig*.json' -o -name '.eslintignore' | head -10Repository: Altinn/altinn-studio
Length of output: 172
🏁 Script executed:
#!/bin/bash
# Look for any recent changes or TODOs related to flowType or task properties
rg -n 'flowType|TODO|FIXME' src/App/frontend/src/layout/CustomButton/CustomButtonComponent.test.tsx -B 2 -A 2Repository: Altinn/altinn-studio
Length of output: 192
Remove flowType property from the mock or add it to the ITask interface.
The mock object includes flowType: 'CompleteCurrentMoveToNext' (line 164), but this property is not defined in the ITask interface at src/App/frontend/src/types/shared.ts. Either remove this property from the mock if it is not needed, or add it to the interface if it is a valid task property.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App/frontend/src/layout/CustomButton/CustomButtonComponent.test.tsx`
around lines 128 - 167, The test mock in CustomButtonComponent.test.tsx sets
currentTask.flowType = 'CompleteCurrentMoveToNext', but that property isn't
declared on the ITask type; either remove flowType from the mock or add it to
the ITask interface in src/App/frontend/src/types/shared.ts. Locate the mock
inside the jest.mocked(InstanceApi.getInstance).mockImplementation block and
either delete the flowType line if it’s not required for assertions, or add
flowType?: string (or a proper union type like 'CompleteCurrentMoveToNext') to
the ITask definition so the mock conforms to the interface.
| return async function loader({ params }: LoaderFunctionArgs) { | ||
| const { instanceOwnerPartyId, instanceGuid } = params; | ||
| const instanceId = instanceOwnerPartyId && instanceGuid ? `${instanceOwnerPartyId}/${instanceGuid}` : undefined; | ||
|
|
||
| // Fire-and-forget: warm the cache without blocking route rendering. | ||
| // Instance and process data should already be cached from the parent instance loader. | ||
| if (instanceOwnerPartyId && instanceGuid) { | ||
| prefetchInstanceData(queryClient, { instanceOwnerPartyId, instanceGuid }); | ||
| } | ||
| if (instanceId) { | ||
| queryClient.prefetchQuery(processQueries.processState(instanceId)); | ||
| await prefetchInstanceData(queryClient, { instanceOwnerPartyId, instanceGuid }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) task loader implementation"
fd 'task.loader.ts$' src/App/frontend -x sh -c 'nl -ba "$1" | sed -n "7,24p"' sh {}
echo
echo "2) prefetchInstanceData definition and nearby logic"
rg -n --type=ts -C5 '\bprefetchInstanceData\b' src/App/frontend/src/core/queries/instance
echo
echo "3) Check whether instance prefetch helpers catch errors internally"
rg -n --type=ts -C4 '\b(prefetchQuery|fetchQuery)\b|\btry\b|\bcatch\b' src/App/frontend/src/core/queries/instance
echo
echo "4) Route error boundary wiring (impact if loader rejects)"
rg -n --type=ts -C3 '\berrorElement\b|\btaskLoader\b' src/App/frontend/src/routesRepository: Altinn/altinn-studio
Length of output: 4661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Complete task loader file ==="
cat -n src/App/frontend/src/routes/task/task.loader.ts
echo
echo "=== 2) Check for errorElement in route configuration ==="
rg -n --type=ts 'taskLoader|errorElement' src/App/frontend/src/routes -B2 -A2
echo
echo "=== 3) TanStack Query usage - check instanceDataQuery definition ==="
rg -n --type=ts 'instanceDataQuery' src/App/frontend/src/core/queries/instance -B2 -A5
echo
echo "=== 4) Check queryOptions usage pattern ==="
rg -n --type=ts 'queryOptions' src/App/frontend/src/core/queries/instanceRepository: Altinn/altinn-studio
Length of output: 4241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Route configuration using taskLoader ==="
rg -n 'taskLoader' src/App/frontend/src/routes -B3 -A3 --type=ts
echo
echo "=== Check for parent route error boundary ==="
rg -n 'errorElement' src/App/frontend/src/routes -B2 -A2 --type=tsRepository: Altinn/altinn-studio
Length of output: 754
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find all imports of taskLoader ==="
rg -n 'from.*task\.loader|import.*taskLoader' src/App/frontend --type=ts --type=tsx
echo
echo "=== Look for route configuration files ==="
fd -e ts -e tsx 'route' src/App/frontend/src/routes | head -20
echo
echo "=== Check main route config or App routes ==="
fd -e ts -e tsx '(routes|App)' src/App/frontend/src | grep -E '(routes\.|App\.route|main.*route)' | head -10Repository: Altinn/altinn-studio
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find all imports of taskLoader ==="
rg -n 'taskLoader' src/App/frontend --type=typescript
echo
echo "=== Look for route configuration or index files ==="
find src/App/frontend/src/routes -name '*.ts' -o -name '*.tsx' | head -20
echo
echo "=== Search for Router or route setup in main App files ==="
find src/App/frontend/src -maxdepth 2 -name '*[Rr]oute*' -o -name 'App*'Repository: Altinn/altinn-studio
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find all references to taskLoader ==="
rg 'taskLoader' src/App/frontend
echo
echo "=== List route directory structure ==="
find src/App/frontend/src/routes -type f | head -30Repository: Altinn/altinn-studio
Length of output: 1129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Router configuration around taskLoader ==="
rg -n -B5 -A5 'taskLoader' src/App/frontend/src/router.tsx
echo
echo "=== Check for errorElement in route configuration ==="
rg -n -B2 -A2 'errorElement' src/App/frontend/src/router.tsx
echo
echo "=== Check task route file ==="
cat -n src/App/frontend/src/routes/task/task.route.tsx | head -50Repository: Altinn/altinn-studio
Length of output: 1432
🏁 Script executed:
#!/bin/bash
set -euo pipefochip
echo "=== Full router.tsx to check errorElement anywhere ==="
cat -n src/App/frontend/src/router.tsx | grep -i 'error'
echo
echo "=== Check task.route.tsx for ErrorBoundary ==="
cat src/App/frontend/src/routes/task/task.route.tsx
echo
echo "=== Verify prefetchInstanceData can reject ==="
cat -n src/App/frontend/src/core/queries/instance/instance.queries.ts | sed -n '19,35p'Repository: Altinn/altinn-studio
Length of output: 119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full router.tsx to check errorElement anywhere ==="
cat -n src/App/frontend/src/router.tsx | grep -i 'error'
echo
echo "=== Check task.route.tsx for ErrorBoundary ==="
cat src/App/frontend/src/routes/task/task.route.tsx
echo
echo "=== Verify prefetchInstanceData can reject ==="
cat -n src/App/frontend/src/core/queries/instance/instance.queries.ts | sed -n '19,35p'Repository: Altinn/altinn-studio
Length of output: 118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check task.route.tsx ==="
cat src/App/frontend/src/routes/task/task.route.tsx
echo
echo "=== Check full router.tsx for errorElement (using rg with no filters) ==="
rg -i 'errorElement' src/App/frontend/src/router.tsx || echo "No errorElement found"
echo
echo "=== Verify instance data query definition and retry behaviour ==="
sed -n '19,35p' src/App/frontend/src/core/queries/instance/instance.queries.tsRepository: Altinn/altinn-studio
Length of output: 1314
Loader comment and implementation are inconsistent; blocking prefetch can fail navigation
Line 14 uses await making the prefetch blocking, while lines 11–12 describe "fire-and-forget" non-blocking behaviour. prefetchInstanceData returns queryClient.prefetchQuery() which can reject if the fetch fails (even after retries), but the loader has no error handling. With no error boundary in the route configuration, a rejected prefetch will break navigation instead of gracefully skipping the warm-up.
Restore best-effort behaviour by removing the await:
Suggested fix
- // Fire-and-forget: warm the cache without blocking route rendering.
- // Instance and process data should already be cached from the parent instance loader.
+ // Best-effort warm-up: do not block route rendering.
+ // Instance and process data should already be cached from the parent instance loader.
if (instanceOwnerPartyId && instanceGuid) {
- await prefetchInstanceData(queryClient, { instanceOwnerPartyId, instanceGuid });
+ void prefetchInstanceData(queryClient, { instanceOwnerPartyId, instanceGuid }).catch(() => {
+ // Ignore warm-up failures; downstream queries can still fetch as needed.
+ });
}📝 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.
| return async function loader({ params }: LoaderFunctionArgs) { | |
| const { instanceOwnerPartyId, instanceGuid } = params; | |
| const instanceId = instanceOwnerPartyId && instanceGuid ? `${instanceOwnerPartyId}/${instanceGuid}` : undefined; | |
| // Fire-and-forget: warm the cache without blocking route rendering. | |
| // Instance and process data should already be cached from the parent instance loader. | |
| if (instanceOwnerPartyId && instanceGuid) { | |
| prefetchInstanceData(queryClient, { instanceOwnerPartyId, instanceGuid }); | |
| } | |
| if (instanceId) { | |
| queryClient.prefetchQuery(processQueries.processState(instanceId)); | |
| await prefetchInstanceData(queryClient, { instanceOwnerPartyId, instanceGuid }); | |
| return async function loader({ params }: LoaderFunctionArgs) { | |
| const { instanceOwnerPartyId, instanceGuid } = params; | |
| // Best-effort warm-up: do not block route rendering. | |
| // Instance and process data should already be cached from the parent instance loader. | |
| if (instanceOwnerPartyId && instanceGuid) { | |
| void prefetchInstanceData(queryClient, { instanceOwnerPartyId, instanceGuid }).catch(() => { | |
| // Ignore warm-up failures; downstream queries can still fetch as needed. | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App/frontend/src/routes/task/task.loader.ts` around lines 8 - 14, The
loader is incorrectly awaiting prefetchInstanceData which contradicts the
"fire-and-forget" comment and can cause navigation failures if
queryClient.prefetchQuery rejects; change the loader so it calls
prefetchInstanceData(instanceOwnerPartyId, instanceGuid) without await and
ensure the returned promise is swallowed (e.g., call it without awaiting and
attach a noop .catch to prevent rejections from bubbling) so warm-up remains
best-effort; update references in the loader function and ensure
prefetchInstanceData/queryClient.prefetchQuery errors are not allowed to reject
the loader.
| | Service task crashes (PDF, signing, payment) | In-memory changes lost. Instance stays at previous state. | | ||
| | Storage API call times out | State unknown. May or may not have persisted. No idempotency. | | ||
| | Pod killed mid-processing | Instance lock held for 5 minutes. Data state unclear. | | ||
| | Timeout during auto-loop iteration N | Iterations 1..N-1 persisted. Iteration N partially executed. | |
There was a problem hiding this comment.
Fix punctuation in range notation for slide readability.
Line 93 uses 1..N-1, which reads as accidental double punctuation in prose. Use a clearer range format (for example 1 to N-1 or 1–(N-1)).
✏️ Proposed text fix
-| Timeout during auto-loop iteration N | Iterations 1..N-1 persisted. Iteration N partially executed. |
+| Timeout during auto-loop iteration N | Iterations 1 to N-1 persisted. Iteration N partially executed. |📝 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.
| | Timeout during auto-loop iteration N | Iterations 1..N-1 persisted. Iteration N partially executed. | | |
| | Timeout during auto-loop iteration N | Iterations 1 to N-1 persisted. Iteration N partially executed. | |
🧰 Tools
🪛 LanguageTool
[typographical] ~93-~93: Two consecutive dots
Context: ...-loop iteration N | Iterations 1..N-1 persisted. Iteration N partially exe...
(DOUBLE_PUNCTUATION)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Runtime/workflow-engine/docs/presentation-technical.md` at line 93, The
prose in the table cell for the row "Timeout during auto-loop iteration N" uses
the range notation "Iterations 1..N-1 persisted. Iteration N partially
executed." which looks like accidental double punctuation; update that phrase to
a clearer range format such as "Iterations 1 to N-1 persisted. Iteration N
partially executed." (or "Iterations 1–(N-1) persisted...") so the slide reads
cleanly—change the text in the table cell containing "Iterations 1..N-1
persisted. Iteration N partially executed." accordingly.
| color: var(--text); | ||
| line-height: 1.5; | ||
| word-break: break-word; | ||
| } |
There was a problem hiding this comment.
Replace deprecated CSS property.
The word-break: break-word property is deprecated. Use overflow-wrap: break-word instead. As per coding guidelines identified by Stylelint: this property-value combination is deprecated.
🔄 Proposed fix
.error-entry-message {
color: var(--text);
line-height: 1.5;
- word-break: break-word;
+ overflow-wrap: break-word;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Runtime/workflow-engine/src/WorkflowEngine.Core/wwwroot/style.css` at
line 1279, Search the stylesheet for any occurrences of the deprecated
declaration "word-break: break-word" and replace each with "overflow-wrap:
break-word" (preserving any related vendor prefixes or other declarations in the
same rule); ensure the replacement is applied inside the same selector block so
layout/line-wrapping behavior remains unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Runtime/workflow-engine/docs/presentation-technical.md (1)
93-93:⚠️ Potential issue | 🟡 MinorFix punctuation in range notation for slide readability.
Line 93 uses
1..N-1, which reads as accidental double punctuation in prose. Use a clearer range format (for example1 to N-1or1–(N-1)).✏️ Proposed text fix
-| Timeout during auto-loop iteration N | Iterations 1..N-1 persisted. Iteration N partially executed. | +| Timeout during auto-loop iteration N | Iterations 1 to N-1 persisted. Iteration N partially executed. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/docs/presentation-technical.md` at line 93, The table cell containing the range "Iterations 1..N-1 persisted. Iteration N partially executed." should use clearer range punctuation; replace "1..N-1" with a more readable form such as "1 to N-1" or "1–(N-1)" in the row that starts with "Timeout during auto-loop iteration N" so the slide reads cleanly.
🧹 Nitpick comments (3)
src/Runtime/workflow-engine/docs/presentation-technical.md (1)
184-184: Consider using h2 headings to maintain proper hierarchy.Multiple h3 subsection headings appear directly under h1 section headings throughout the document (lines 184, 190, 197, 239, 246, 255, 261, 267, 273), skipping the h2 level. Whilst this appears to be an intentional visual style for the Marp presentation, Markdown best practices recommend incrementing heading levels by only one at a time (h1 → h2 → h3).
If you prefer to maintain the current visual styling, you can safely ignore this suggestion, as Marp will render it correctly. Alternatively, you could convert these h3 headings to h2 for better Markdown conformance.
Also applies to: 190-190, 197-197, 239-239, 246-246, 255-255, 261-261, 267-267, 273-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Runtime/workflow-engine/docs/presentation-technical.md` at line 184, The document uses h3 headings (###) directly under h1 sections which skips the h2 level; update the listed subsection headings (e.g., "How work is picked up" and the other headings at the noted locations) from h3 to h2 (change ### to ##) so heading hierarchy increments by one level (h1 → h2 → h3) and conforms to Markdown best practices; if you must keep Marp visuals intact, only apply this change to the logical document source headings (the heading tokens) rather than visual wrappers.src/App/frontend/src/features/instance/useProcessNext.tsx (2)
10-10: Remove unuseduseOptimisticallyUpdateProcessimport and variable.
useOptimisticallyUpdateProcessis imported and instantiated on line 46, but the resultingoptimisticallyUpdateProcessfunction is never called within this file. This appears to be leftover code from the previous implementation where process state was managed separately.🧹 Proposed cleanup
Remove the import on line 10:
-import { useOptimisticallyUpdateProcess } from 'src/features/instance/useProcessQuery';Remove the unused variable on line 46:
- const optimisticallyUpdateProcess = useOptimisticallyUpdateProcess();Also applies to: 46-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/frontend/src/features/instance/useProcessNext.tsx` at line 10, Remove the unused optimistic updater: delete the import useOptimisticallyUpdateProcess and the instantiation that assigns optimisticallyUpdateProcess (the unused variable) so no unused import/variable remains; locate the import statement for useOptimisticallyUpdateProcess and the line where optimisticallyUpdateProcess is created and remove both, leaving the rest of useProcessNext.tsx unchanged.
70-70: Type castas BackendValidationIssue[]could be replaced with runtime validation or proper typing.The coding guidelines recommend avoiding type casting (
as type). This cast assumes the API response shape without validation. If the backend contract changes, this could lead to runtime issues.Consider adding a type guard or using a validation library, or at minimum documenting why this cast is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/frontend/src/features/instance/useProcessNext.tsx` at line 70, Replace the unsafe cast of error.response.data['validationIssues'] to BackendValidationIssue[] in useProcessNext by validating the payload at runtime: implement a type guard (e.g., isBackendValidationIssueArray(obj): obj is BackendValidationIssue[]) or use a runtime validator (zod/yup) that checks error.response.data.validationIssues is an array and each item matches the BackendValidationIssue shape, then only return the validated value; if validation fails, return a safe fallback (null or an empty array) or log/throw a clear error; alternatively, if you can guarantee the contract, add a short inline comment explaining why the cast is safe and cite BackendValidationIssue as the referenced type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App/frontend/src/features/process/feedback/Feedback.tsx`:
- Around line 22-47: The callback passed to useBackoff can reject if
InstanceApi.getInstance(...) fails, causing unhandled promise rejections; wrap
the body of the useCallback (the call to InstanceApi.getInstance and the
subsequent logic in function callback) in a try-catch, catch errors from
InstanceApi.getInstance, handle/log the error (e.g., console.error or a provided
logger) and return early so the promise resolves gracefully; keep navigateToTask
and the rest of the success-path unchanged.
---
Duplicate comments:
In `@src/Runtime/workflow-engine/docs/presentation-technical.md`:
- Line 93: The table cell containing the range "Iterations 1..N-1 persisted.
Iteration N partially executed." should use clearer range punctuation; replace
"1..N-1" with a more readable form such as "1 to N-1" or "1–(N-1)" in the row
that starts with "Timeout during auto-loop iteration N" so the slide reads
cleanly.
---
Nitpick comments:
In `@src/App/frontend/src/features/instance/useProcessNext.tsx`:
- Line 10: Remove the unused optimistic updater: delete the import
useOptimisticallyUpdateProcess and the instantiation that assigns
optimisticallyUpdateProcess (the unused variable) so no unused import/variable
remains; locate the import statement for useOptimisticallyUpdateProcess and the
line where optimisticallyUpdateProcess is created and remove both, leaving the
rest of useProcessNext.tsx unchanged.
- Line 70: Replace the unsafe cast of error.response.data['validationIssues'] to
BackendValidationIssue[] in useProcessNext by validating the payload at runtime:
implement a type guard (e.g., isBackendValidationIssueArray(obj): obj is
BackendValidationIssue[]) or use a runtime validator (zod/yup) that checks
error.response.data.validationIssues is an array and each item matches the
BackendValidationIssue shape, then only return the validated value; if
validation fails, return a safe fallback (null or an empty array) or log/throw a
clear error; alternatively, if you can guarantee the contract, add a short
inline comment explaining why the cast is safe and cite BackendValidationIssue
as the referenced type.
In `@src/Runtime/workflow-engine/docs/presentation-technical.md`:
- Line 184: The document uses h3 headings (###) directly under h1 sections which
skips the h2 level; update the listed subsection headings (e.g., "How work is
picked up" and the other headings at the noted locations) from h3 to h2 (change
### to ##) so heading hierarchy increments by one level (h1 → h2 → h3) and
conforms to Markdown best practices; if you must keep Marp visuals intact, only
apply this change to the logical document source headings (the heading tokens)
rather than visual wrappers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd0b558d-a407-483f-a707-2be45038b70c
📒 Files selected for processing (7)
src/App/frontend/src/components/wrappers/ProcessWrapper.tsxsrc/App/frontend/src/core/queries/instance/index.tssrc/App/frontend/src/features/instance/useProcessNext.tsxsrc/App/frontend/src/features/process/feedback/Feedback.tsxsrc/App/frontend/src/routes/task/task.loader.tssrc/Runtime/workflow-engine/docs/presentation-technical.mdsrc/Runtime/workflow-engine/docs/technical-guide.md
✅ Files skipped from review due to trivial changes (1)
- src/Runtime/workflow-engine/docs/technical-guide.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/App/frontend/src/routes/task/task.loader.ts
- src/App/frontend/src/core/queries/instance/index.ts
- src/App/frontend/src/components/wrappers/ProcessWrapper.tsx
| const callback = useCallback(async () => { | ||
| const result = await reFetchProcessData(); | ||
| if (!result.data) { | ||
| if (!instanceOwnerPartyId || !instanceGuid) { | ||
| return; | ||
| } | ||
|
|
||
| // Fetch instance data directly from the API instead of through React Query. | ||
| // This avoids updating the shared query cache, which would cause ProcessWrapper | ||
| // to briefly see new process data while the URL still points to the feedback task, | ||
| // resulting in a flash of the NavigationError. | ||
| const instance = await InstanceApi.getInstance({ instanceOwnerPartyId, instanceGuid }); | ||
| const process = instance.process; | ||
|
|
||
| let navigateTo: undefined | string; | ||
| if (result.data.ended) { | ||
| if (process.ended) { | ||
| navigateTo = TaskKeys.ProcessEnd; | ||
| } else if ( | ||
| result.data.currentTask?.elementId && | ||
| result.data.currentTask.elementId !== previousData?.currentTask?.elementId | ||
| process.currentTask?.elementId && | ||
| process.currentTask.elementId !== previousProcess?.currentTask?.elementId | ||
| ) { | ||
| navigateTo = result.data.currentTask.elementId; | ||
| navigateTo = process.currentTask.elementId; | ||
| } | ||
|
|
||
| if (navigateTo) { | ||
| optimisticallyUpdateProcess(result.data); | ||
| await reFetchInstanceData(); | ||
| navigateToTask(navigateTo); | ||
| } | ||
| }, [ | ||
| navigateToTask, | ||
| optimisticallyUpdateProcess, | ||
| previousData?.currentTask?.elementId, | ||
| reFetchInstanceData, | ||
| reFetchProcessData, | ||
| ]); | ||
| }, [navigateToTask, previousProcess?.currentTask?.elementId, instanceOwnerPartyId, instanceGuid]); |
There was a problem hiding this comment.
Add error handling for API failures.
The callback can reject (e.g., network errors from InstanceApi.getInstance()), but the useBackoff hook calls callback().then() on line 77 without a .catch(). This will result in unhandled promise rejections and may cause issues if the API is temporarily unavailable.
Consider wrapping the API call in a try-catch or adding error handling to ensure the backoff continues gracefully:
🛡️ Proposed fix
const callback = useCallback(async () => {
if (!instanceOwnerPartyId || instanceGuid) {
return;
}
+ let instance;
+ try {
- // Fetch instance data directly from the API instead of through React Query.
- // This avoids updating the shared query cache, which would cause ProcessWrapper
- // to briefly see new process data while the URL still points to the feedback task,
- // resulting in a flash of the NavigationError.
- const instance = await InstanceApi.getInstance({ instanceOwnerPartyId, instanceGuid });
+ // Fetch instance data directly from the API instead of through React Query.
+ // This avoids updating the shared query cache, which would cause ProcessWrapper
+ // to briefly see new process data while the URL still points to the feedback task,
+ // resulting in a flash of the NavigationError.
+ instance = await InstanceApi.getInstance({ instanceOwnerPartyId, instanceGuid });
+ } catch {
+ // Silently ignore errors - the backoff will retry
+ return;
+ }
const process = instance.process;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App/frontend/src/features/process/feedback/Feedback.tsx` around lines 22
- 47, The callback passed to useBackoff can reject if
InstanceApi.getInstance(...) fails, causing unhandled promise rejections; wrap
the body of the useCallback (the call to InstanceApi.getInstance and the
subsequent logic in function callback) in a try-catch, catch errors from
InstanceApi.getInstance, handle/log the error (e.g., console.error or a provided
logger) and return early so the promise resolves gracefully; keep navigateToTask
and the rest of the success-path unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/App/frontend/src/components/wrappers/ProcessWrapper.tsx (1)
35-36: Consider importing the mutation key prefix fromuseProcessNext.tsxto avoid duplication.The
'processNext'string is duplicated here and ingetProcessNextMutationKey(). If that function's prefix changes, this hardcoded value would silently become stale.♻️ Suggested refactor
In
src/App/frontend/src/features/instance/useProcessNext.tsx:export const processNextMutationKeyPrefix = ['processNext'] as const; export function getProcessNextMutationKey(action?: IActionType) { if (!action) { return processNextMutationKeyPrefix; } return [...processNextMutationKeyPrefix, action] as const; }Then in this file:
-const processNextMutationKeyPrefix = ['processNext'] as const; +import { processNextMutationKeyPrefix } from 'src/features/instance/useProcessNext';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App/frontend/src/components/wrappers/ProcessWrapper.tsx` around lines 35 - 36, Remove the duplicated local declaration of processNextMutationKeyPrefix in ProcessWrapper.tsx and import the canonical processNextMutationKeyPrefix (and/or getProcessNextMutationKey if needed) from src/features/instance/useProcessNext.tsx; replace references to the local const with the imported symbol so the mutation key prefix is single-sourced (ensure any usages still work with the imported type/assertion and delete the local const declaration).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/App/frontend/src/components/wrappers/ProcessWrapper.tsx`:
- Around line 35-36: Remove the duplicated local declaration of
processNextMutationKeyPrefix in ProcessWrapper.tsx and import the canonical
processNextMutationKeyPrefix (and/or getProcessNextMutationKey if needed) from
src/features/instance/useProcessNext.tsx; replace references to the local const
with the imported symbol so the mutation key prefix is single-sourced (ensure
any usages still work with the imported type/assertion and delete the local
const declaration).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3a75563-42ec-43f0-b9fd-9a3de113660e
📒 Files selected for processing (1)
src/App/frontend/src/components/wrappers/ProcessWrapper.tsx
olemartinorg
left a comment
olemartinorg
left a comment
There was a problem hiding this comment.
Looks good! 🥳 There's the breaking things in the backend API I think we should discuss or get an external approval on, but apart from that this seems good to go! 👏
|
|
||
| if (instance) { | ||
| changeInstance(() => instance); | ||
| changeInstance((prev) => ({ ...prev, ...instance, process: prev?.process })); |
There was a problem hiding this comment.
The PATCH endpoint should probably also include the enriched process data? Unless that's bad performance-wise. Seems odd that we overwrite the simple process state we get back here.
| throw new Error('Missing task in process data. Cannot navigate to task.'); | ||
| } | ||
| navigateToTask(task); | ||
| await reFetchInstanceData(); |
There was a problem hiding this comment.
I believe it would have been better to introduce a new mode on process/next that returns the full instance data instead of fetching it afterwards.
There was a problem hiding this comment.
Also, if we're refetching after navigation that means we have a varying short period here where the taskId from the URL is actually newer than what we're seeing in the process state. That's opposite of what we had before, and might cause weird things to happen. Also would have been prevented if we could just insert new instance data and immediately navigate.
|
|
||
| return (updater: (oldData: IInstance) => IInstance) => { | ||
| const cached = queryClient.getQueriesData<IInstance>({ queryKey: instanceQueryKeys.all() }); | ||
| const entry = cached.find(([key, data]) => data && key.length === 2 && typeof key[1] === 'object'); |
There was a problem hiding this comment.
Dette er nokså likt som det vi snakket om tidligere - som vi valgte å vente med å løse den gangen.
Finner cache-entry basert på key-lengde og type. Hvis query-key-strukturen endres, ryker dette oppsettet. Kunne vi brukt en eksplisitt cache key? 🤔 😊
There was a problem hiding this comment.
jess som vi snakket om så har jeg en plan på å fikse dette men det krever litt mer rydding så vil bare merge denne først :)
| // Fetch instance data directly from the API instead of through React Query. | ||
| // This avoids updating the shared query cache, which would cause ProcessWrapper | ||
| // to briefly see new process data while the URL still points to the feedback task, | ||
| // resulting in a flash of the NavigationError. | ||
| const instance = await InstanceApi.getInstance({ instanceOwnerPartyId, instanceGuid }); |
There was a problem hiding this comment.
Det var fint med en beskrivelse her om hvorfor det gikk direkte mot apiet i det minste, selv om det er litt trist - fordi det oppleves litt som en workaround til et underliggende problem.
There was a problem hiding this comment.
Dette er absoutt en workaround, jeg tror løsningen er å fjerne ProcessWrapper.
Har laget et issue for det her:
| const { instanceOwnerPartyId, instanceGuid } = parseInstanceId(instanceId); | ||
| navigate(buildInstanceUrl(instanceOwnerPartyId, instanceGuid)); | ||
| if (data) { | ||
| navigate(`/instance/${data.id}`); |
There was a problem hiding this comment.
I en tidligere review av en annen PR så kom jeg med forslaget om å lage en metode som heter navigateToInstance for å slippe å ha løse lenker rundt om kring. Mener å huske at du valgte å implementere det - burde vi oppdatert det både her og i elseif? 😊
| const loaderData = useLoaderData<InstanceLoaderResult>(); | ||
|
|
||
| if (loaderData?.error) { | ||
| return <DisplayError error={loaderData.error} />; | ||
| } |
There was a problem hiding this comment.
Burde vi flytte dette ut i en egen InstanceLoaderComponent som vi kunne brukt direkte i errorElement i React Router? 🤔 😊
There was a problem hiding this comment.
Enig, fikser i neste PR :)
| // Always fetch fresh instance data when navigating to a task. | ||
| // This ensures the cached process state matches the task in the URL, | ||
| // preventing a flash of the wrong-task error during transitions | ||
| // (e.g., when Feedback polling detects an external process change). |
There was a problem hiding this comment.
Fint at det er en kommentar her, men fremstår som et problem/utfordring vi burde lage en sak på, så vi får løst det bedre. Hva tenker du? :)
There was a problem hiding this comment.
Kommentaren er ikke relevant lengre så jeg fjerner bare :)
| if (instanceId) { | ||
| queryClient.prefetchQuery(processQueries.processState(instanceId)); | ||
| try { | ||
| await fetchFreshInstanceData(queryClient, { instanceOwnerPartyId, instanceGuid }); |
There was a problem hiding this comment.
Vil dette skape en hvit side mens vi laster appen? Det er vel ingen spinner mens vi laster inn nå, så brukeren ser noe først når det går bra eller feiler? 😊
There was a problem hiding this comment.
Enig, laget et issue på det:
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/App/backend/test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt`:
- Line 1023: Document the breaking API change: update the repository release
notes/CHANGELOG and any migration guide to explicitly state that the
InstanceResponse.Process property type changed from ProcessState to
Altinn.App.Core.Internal.Process.Elements.AppProcessState (referenced in the
public API snapshot and validated by PublicApi_ShouldNotChange_Unintentionally),
explain that GET /process behavior is removed/merged into InstanceResponse, list
client-side migration steps (how to map old ProcessState usages to
AppProcessState, any new properties or behavioral differences), and add a clear
migration example and version bump so consumers can adapt their strongly-typed
clients.
🪄 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: d6ded1b0-527c-4a04-b9e8-d5f07dcb216a
📒 Files selected for processing (7)
src/App/backend/src/Altinn.App.Api/Controllers/InstancesController.cssrc/App/backend/src/Altinn.App.Api/Controllers/ProcessController.cssrc/App/backend/src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/App/backend/test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cssrc/App/backend/test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.jsonsrc/App/backend/test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/App/backend/test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
✅ Files skipped from review due to trivial changes (1)
- src/App/backend/test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/App/backend/src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
- src/App/backend/test/Altinn.App.Api.Tests/OpenApi/OpenApiSpecChangeDetection.SaveJsonSwagger.verified.json
- src/App/backend/test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
| public required string Org { get; init; } | ||
| public required System.Collections.Generic.IReadOnlyDictionary<string, string> PresentationTexts { get; init; } | ||
| public required Altinn.Platform.Storage.Interface.Models.ProcessState Process { get; init; } | ||
| public required Altinn.App.Core.Internal.Process.Elements.AppProcessState Process { get; init; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify AppProcessState inheritance and check for migration documentation
# Check AppProcessState inherits from ProcessState
rg -n "class AppProcessState" --type=cs -A 2
# Check for any changelog or migration documentation mentioning this change
fd -i "changelog\|migration\|breaking" -e md -e txt | xargs -I{} grep -l -i "process\|instance" {} 2>/dev/null || echo "No migration docs found - ensure this breaking change is documented"Repository: Altinn/altinn-studio
Length of output: 980
🏁 Script executed:
#!/bin/bash
# Search for changelog-related files with broader patterns
echo "=== Looking for changelog/release notes files ==="
find . -type f \( -iname "*.md" -o -iname "*.txt" \) | rg -i "change|release|news|history" | head -20
echo ""
echo "=== Checking root-level documentation files ==="
ls -la | grep -i "change\|release\|news\|history"
echo ""
echo "=== Checking AppProcessState implementation ==="
cat -n src/App/backend/src/Altinn.App.Core/Internal/Process/Elements/AppProcessState.cs | head -30
echo ""
echo "=== Checking public API test context ==="
head -50 src/App/backend/test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtRepository: Altinn/altinn-studio
Length of output: 7712
Ensure this breaking API change is documented in user-facing release notes.
The InstanceResponse.Process property type changed from ProcessState to AppProcessState, which is a breaking change for strongly-typed clients. The snapshot test file confirms this change is intentional (as verified by the PublicApi_ShouldNotChange_Unintentionally test framework), and AppProcessState is properly documented in code as an "extended representation" that inherits from ProcessState.
However, no user-facing documentation (CHANGELOG, migration guide, or release notes) was found in the repository. Since this eliminates the separate GET /process API call and changes the response type, API consumers need explicit guidance on how to update their clients. Ensure this breaking change is documented in release notes or migration guides.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/App/backend/test/Altinn.App.Api.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt`
at line 1023, Document the breaking API change: update the repository release
notes/CHANGELOG and any migration guide to explicitly state that the
InstanceResponse.Process property type changed from ProcessState to
Altinn.App.Core.Internal.Process.Elements.AppProcessState (referenced in the
public API snapshot and validated by PublicApi_ShouldNotChange_Unintentionally),
explain that GET /process behavior is removed/merged into InstanceResponse, list
client-side migration steps (how to map old ProcessState usages to
AppProcessState, any new properties or behavioral differences), and add a clear
migration example and version bump so consumers can adapt their strongly-typed
clients.
3 participants
Description
Extracts process state enrichment logic from
ProcessControllerinto a sharedProcessStateEnricherservice and updates the form bootstrap endpoint to return the fullInstanceobject and enrichedAppProcessState(with authorized actions, read/write access, element types, and process task metadata).Changes
ProcessStateEnricherservice — ExtractedConvertAndAuthorizeActionsandAuthorizeActionsfromProcessControllerinto a reusable service inAltinn.App.Core/Internal/Process/FormBootstrapResponseinto separate types for each endpoint:InstanceFormBootstrapResponse— addsInstance,Process(enrichedAppProcessState), andValidationIssuesStatelessFormBootstrapResponse— only shared properties (layouts, data models, static options)FormBootstrapService— injectsProcessStateEnricher, runs process enrichment in parallel with other bootstrap tasksProcessController— delegates toProcessStateEnricherinstead of private methodsThis eliminates the need for the frontend to make separate GET
/processand GET instance calls when bootstrapping a form.Related issue(s)
Verification
Summary by CodeRabbit
Release Notes
New Features
Improvements