Add remaining Maskinporten consumer functionality - #2211
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughSplits Maskinporten resource delegation into supplier- and consumer-specific APIs and wiring: backend contracts, HTTP client and mocks, service calls, controller routes and tests, RTK Query endpoints/tags, and frontend hooks/components/pages, plus routing and localization changes. ChangesMaskinporten Supplier/Consumer Delegation Refactor
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
github-actions
Bot
added
the
kind/feature-request
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/features/amUI/maskinporten/ScopeSearch.tsx (1)
43-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty supplier string may bypass skip condition.
The
suppliervariable (line 32) defaults to an empty string whentoParty?.orgNumberis undefined. The skip condition!supplierevaluates tofalsefor"", allowing the query to execute with an empty supplier parameter.🛡️ Proposed fix
const { data: delegatedResources } = useGetMaskinportenSupplierResourcesQuery( { party: fromParty?.partyUuid, supplier, }, { - skip: !fromParty?.partyUuid || !supplier, + skip: !fromParty?.partyUuid || !toParty?.orgNumber, refetchOnMountOrArgChange: true, }, );🤖 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/features/amUI/maskinporten/ScopeSearch.tsx` around lines 43 - 52, The query is being triggered when supplier is an empty string because the skip test uses !supplier which is false for ""—update the logic so an empty supplier does not pass; either make the supplier variable undefined when missing (e.g., set supplier = toParty?.orgNumber ?? undefined) or change the skip condition for useGetMaskinportenSupplierResourcesQuery to explicitly check for a non-empty string (e.g., skip when !supplier || supplier.length === 0); reference: supplier and useGetMaskinportenSupplierResourcesQuery.src/features/amUI/maskinporten/ScopeInfo.tsx (1)
59-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty supplier string may bypass skip condition.
The
suppliervariable (line 36) defaults to an empty string whentoParty?.orgNumberis undefined. The skip condition checks!supplier, which evaluates tofalsefor an empty string, allowing the query to execute withsupplier="". Consider updating the skip condition to!fromParty?.partyUuid || !supplier || !resource.identifieror ensuringsupplieris explicitly undefined/null when unavailable.🛡️ Proposed fix
const supplier = toParty?.orgNumber ?? ''; ... const { data: delegatedResources, isFetching: isDelegatedResourcesLoading } = useGetMaskinportenSupplierResourcesQuery( { party: fromParty?.partyUuid, supplier, resource: resource.identifier, }, { - skip: !fromParty?.partyUuid || !supplier || !resource.identifier, + skip: !fromParty?.partyUuid || !toParty?.orgNumber || !resource.identifier, refetchOnMountOrArgChange: true, }, );🤖 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/features/amUI/maskinporten/ScopeInfo.tsx` around lines 59 - 70, The skip condition for useGetMaskinportenSupplierResourcesQuery allows execution when supplier is an empty string; update the logic so the query is skipped when supplier is missing or empty by either making supplier undefined/null when unavailable or changing the skip expression to explicitly check emptiness (e.g., !fromParty?.partyUuid || supplier === "" || !resource.identifier). Locate the supplier declaration and the useGetMaskinportenSupplierResourcesQuery call (including the skip option) and apply the change so the query does not run with supplier === "".backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Integration/Clients/MaskinportenClient.cs (1)
109-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
cancellationTokento delete calls.Both resource-delete methods accept a token but don’t pass it to the HTTP delete call, so request cancellation is not honored upstream.
Suggested fix
-HttpResponseMessage response = await _client.DeleteAsync(token, endpointUrl); +HttpResponseMessage response = await _client.DeleteAsync(token, endpointUrl, cancellationToken);Apply in both:
RemoveSupplierResource(...)RemoveConsumerResource(...)Also applies to: 205-211
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Integration/Clients/MaskinportenClient.cs` around lines 109 - 115, The DeleteAsync calls in RemoveSupplierResource and RemoveConsumerResource do not pass the provided CancellationToken, so cancellations are ignored; update the calls to _client.DeleteAsync to include the cancellationToken parameter (e.g., _client.DeleteAsync(token, endpointUrl, cancellationToken)) in both methods (also update the other occurrence around the 205–211 block) so the incoming cancellationToken is propagated to the HTTP request.backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI/Controllers/MaskinportenController.cs (1)
121-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
[Required]on non-nullableGuid partyis insufficient—add explicitGuid.Emptyvalidation.These actions accept
Guid.Emptywhenpartyis omitted because[Required]validates the bound value (which isGuid.Emptyby default), not whether the parameter was present in the request. Use[BindRequired]instead, or add a guard check.Suggested fix
- if (!ModelState.IsValid) + if (!ModelState.IsValid || party == Guid.Empty) { return BadRequest(ModelState); }Apply to:
GetSupplierResources,AddSupplierResource,RemoveSupplierResource,GetConsumerResources,RemoveConsumerResource.Alternatively, replace
[Required]with[BindRequired]on all affectedpartyparameters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI/Controllers/MaskinportenController.cs` around lines 121 - 125, The action parameters decorated with [Required] accept Guid.Empty (missing in request) so add explicit validation or use model-binding to require presence: for GetSupplierResources, AddSupplierResource, RemoveSupplierResource, GetConsumerResources, RemoveConsumerResource either replace [Required] with [BindRequired] on the Guid party parameters or keep [Required] and add a guard at the start of each method that throws BadRequest/returns 400 when party == Guid.Empty; ensure the same approach is applied consistently to all listed methods and include a clear error message indicating the missing or empty party GUID.
🧹 Nitpick comments (2)
backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Tests/Controllers/MaskinportenControllerTest.cs (2)
390-404: ⚡ Quick winAdd coverage for the new
consumerfilter onGET /maskinporten/consumers.This PR adds optional consumer filtering across contracts/client/controller, but there’s no test asserting filtered results when
consumeris supplied.Also applies to: 785-815
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Tests/Controllers/MaskinportenControllerTest.cs` around lines 390 - 404, The test GetConsumers_ReturnsConsumers lacks coverage for the new optional consumer query filter on GET /maskinporten/consumers; add a new test (or extend this one) that calls _client.GetAsync with both party and consumer query params (e.g., ?party={party}&consumer={consumerId}), set up expected filtered data from the same test JSON (or a new filtered fixture) and assert HttpStatusCode.OK and that the deserialized List<MaskinportenConnection> only contains connections matching the supplied consumer; use the existing helper Util.GetMockData and SetAuthHeader and mirror assertions from GetConsumers_ReturnsConsumers to validate the filtered behavior.
199-236: ⚡ Quick winAdd missing-party tests for the renamed/new resource endpoints.
Please add cases where
partyis omitted to lock in expected 400 behavior for supplier/consumer resource endpoints.Also applies to: 804-815, 865-904
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Tests/Controllers/MaskinportenControllerTest.cs` around lines 199 - 236, Add parallel test cases that assert a 400 when the party query parameter is omitted for the supplier and consumer resource endpoints; specifically, in MaskinportenControllerTest add tests similar to GetSupplierResources_InvalidParty_ReturnsBadRequest, AddSupplierResource_InvalidParty_ReturnsBadRequest, and RemoveSupplierResource_InvalidParty_ReturnsBadRequest but call the same endpoints without the "party=..." query string (e.g., GET "accessmanagement/api/v1/maskinporten/suppliers/resources", POST "accessmanagement/api/v1/maskinporten/suppliers/resources?supplier=...&resource=...", DELETE "accessmanagement/api/v1/maskinporten/suppliers/resources?supplier=...&resource=..."), and likewise add corresponding tests for the consumer endpoints referenced around lines 804-815 and 865-904 to ensure each returns HttpStatusCode.BadRequest when party is missing.
🤖 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.
Outside diff comments:
In
`@backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Integration/Clients/MaskinportenClient.cs`:
- Around line 109-115: The DeleteAsync calls in RemoveSupplierResource and
RemoveConsumerResource do not pass the provided CancellationToken, so
cancellations are ignored; update the calls to _client.DeleteAsync to include
the cancellationToken parameter (e.g., _client.DeleteAsync(token, endpointUrl,
cancellationToken)) in both methods (also update the other occurrence around the
205–211 block) so the incoming cancellationToken is propagated to the HTTP
request.
In
`@backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI/Controllers/MaskinportenController.cs`:
- Around line 121-125: The action parameters decorated with [Required] accept
Guid.Empty (missing in request) so add explicit validation or use model-binding
to require presence: for GetSupplierResources, AddSupplierResource,
RemoveSupplierResource, GetConsumerResources, RemoveConsumerResource either
replace [Required] with [BindRequired] on the Guid party parameters or keep
[Required] and add a guard at the start of each method that throws
BadRequest/returns 400 when party == Guid.Empty; ensure the same approach is
applied consistently to all listed methods and include a clear error message
indicating the missing or empty party GUID.
In `@src/features/amUI/maskinporten/ScopeInfo.tsx`:
- Around line 59-70: The skip condition for
useGetMaskinportenSupplierResourcesQuery allows execution when supplier is an
empty string; update the logic so the query is skipped when supplier is missing
or empty by either making supplier undefined/null when unavailable or changing
the skip expression to explicitly check emptiness (e.g., !fromParty?.partyUuid
|| supplier === "" || !resource.identifier). Locate the supplier declaration and
the useGetMaskinportenSupplierResourcesQuery call (including the skip option)
and apply the change so the query does not run with supplier === "".
In `@src/features/amUI/maskinporten/ScopeSearch.tsx`:
- Around line 43-52: The query is being triggered when supplier is an empty
string because the skip test uses !supplier which is false for ""—update the
logic so an empty supplier does not pass; either make the supplier variable
undefined when missing (e.g., set supplier = toParty?.orgNumber ?? undefined) or
change the skip condition for useGetMaskinportenSupplierResourcesQuery to
explicitly check for a non-empty string (e.g., skip when !supplier ||
supplier.length === 0); reference: supplier and
useGetMaskinportenSupplierResourcesQuery.
---
Nitpick comments:
In
`@backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Tests/Controllers/MaskinportenControllerTest.cs`:
- Around line 390-404: The test GetConsumers_ReturnsConsumers lacks coverage for
the new optional consumer query filter on GET /maskinporten/consumers; add a new
test (or extend this one) that calls _client.GetAsync with both party and
consumer query params (e.g., ?party={party}&consumer={consumerId}), set up
expected filtered data from the same test JSON (or a new filtered fixture) and
assert HttpStatusCode.OK and that the deserialized List<MaskinportenConnection>
only contains connections matching the supplied consumer; use the existing
helper Util.GetMockData and SetAuthHeader and mirror assertions from
GetConsumers_ReturnsConsumers to validate the filtered behavior.
- Around line 199-236: Add parallel test cases that assert a 400 when the party
query parameter is omitted for the supplier and consumer resource endpoints;
specifically, in MaskinportenControllerTest add tests similar to
GetSupplierResources_InvalidParty_ReturnsBadRequest,
AddSupplierResource_InvalidParty_ReturnsBadRequest, and
RemoveSupplierResource_InvalidParty_ReturnsBadRequest but call the same
endpoints without the "party=..." query string (e.g., GET
"accessmanagement/api/v1/maskinporten/suppliers/resources", POST
"accessmanagement/api/v1/maskinporten/suppliers/resources?supplier=...&resource=...",
DELETE
"accessmanagement/api/v1/maskinporten/suppliers/resources?supplier=...&resource=..."),
and likewise add corresponding tests for the consumer endpoints referenced
around lines 804-815 and 865-904 to ensure each returns
HttpStatusCode.BadRequest when party is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c72b68b2-8602-4f9b-a981-651bcecc46d6
📒 Files selected for processing (12)
backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Core/ClientInterfaces/IMaskinportenClient.csbackend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Core/Services/Interfaces/IMaskinportenService.csbackend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Core/Services/MaskinportenService.csbackend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Integration/Clients/MaskinportenClient.csbackend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Mocks/Mocks/MaskinportenClientMock.csbackend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Tests/Controllers/MaskinportenControllerTest.csbackend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI/Controllers/MaskinportenController.cssrc/features/amUI/maskinporten/ScopeInfo.tsxsrc/features/amUI/maskinporten/ScopeSearch.tsxsrc/features/amUI/maskinporten/SupplierPageContent.tsxsrc/features/amUI/maskinporten/hooks/useMaskinportenResourceActions.tssrc/rtk/features/maskinportenApi.ts
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes the Maskinporten “consumer” BFF surface while refactoring existing “supplier” resource delegation endpoints to use explicit supplier/consumer paths, improving clarity and enabling optional filtering.
Changes:
- Refactors supplier resource delegation endpoints to
suppliers/resources(frontend RTK + backend controller/service/client) and updates all call sites/tests accordingly. - Adds consumer-oriented endpoints for listing delegated resources and removing a delegated resource via
consumers/resources. - Extends consumers query support with optional
consumerfilter parameter across frontend + backend + mocks/tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rtk/features/maskinportenApi.ts | Splits resource tags/endpoints into supplier vs consumer, adds consumer resources endpoints and optional consumer filtering. |
| src/features/amUI/maskinporten/SupplierPageContent.tsx | Updates supplier page to use the renamed supplier resources query hook. |
| src/features/amUI/maskinporten/ScopeSearch.tsx | Updates delegated resources lookup to use supplier-specific resources endpoint. |
| src/features/amUI/maskinporten/ScopeInfo.tsx | Updates delegated resources lookup to use supplier-specific resources endpoint. |
| src/features/amUI/maskinporten/hooks/useMaskinportenResourceActions.ts | Updates resource delegation actions to use supplier-specific add/remove mutations. |
| backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI/Controllers/MaskinportenController.cs | Renames supplier resource routes and adds consumers/resources GET/DELETE endpoints. |
| backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Tests/Controllers/MaskinportenControllerTest.cs | Updates supplier resource route tests and adds coverage for new consumer resources endpoints. |
| backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Mocks/Mocks/MaskinportenClientMock.cs | Aligns mock client with renamed supplier methods and adds consumer resources mock methods + optional consumer filter. |
| backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Integration/Clients/MaskinportenClient.cs | Renames supplier resource client methods and adds consumer resources client methods + optional consumer query param. |
| backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Core/Services/MaskinportenService.cs | Renames supplier resource service methods and adds consumer resource mapping method. |
| backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Core/Services/Interfaces/IMaskinportenService.cs | Updates service interface to reflect renamed supplier methods and new consumer resources operations. |
| backend/src/Altinn.AccessManagement.UI/Altinn.AccessManagement.UI.Core/ClientInterfaces/IMaskinportenClient.cs | Updates client interface to reflect renamed supplier methods and new consumer resources operations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/amUI/maskinporten/ScopeInfo.tsx (1)
259-268:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
aria-disabledinstead ofdisabledonDsButton.Per codebase conventions,
DsButtonshould usearia-disabledto remain focusable for accessibility. The click handler should guard against the disabled state.♿ Proposed fix
<DsButton data-size='sm' data-color='danger' - disabled={isActionLoading} - onClick={handleRemoveResource} + aria-disabled={isActionLoading} + onClick={() => { + if (isActionLoading) return; + handleRemoveResource(); + }} variant='secondary' >Based on learnings: "when using the
DsButtoncomponent, it is intentional to usearia-disabled(not the nativedisabledattribute) so the button remains focusable for accessibility."🤖 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/features/amUI/maskinporten/ScopeInfo.tsx` around lines 259 - 268, Replace the native disabled prop on the DsButton with aria-disabled and ensure the click handler early-exits when the button should be disabled: change the DsButton prop from disabled={isActionLoading} to aria-disabled={isActionLoading}, and update handleRemoveResource (or add a small wrapper onClick) to check isActionLoading and return immediately if true so clicks are ignored while aria-disabled is set; keep the existing variant, icon, and label intact.
🧹 Nitpick comments (3)
src/features/amUI/maskinporten/hooks/useMaskinportenResourceActions.ts (1)
47-55: 💤 Low valueInconsistent return types from
delegateandremove.When the action is missing, you return
undefined(sync). When present, you returnrun(...)which is aPromise<void>. Callers awaiting these might get unexpected behavior.Consider making the return type explicit and consistent:
♻️ Proposed fix
- const delegate = (resource: ServiceResource, callbacks: ActionCallbacks = {}) => { - if (!delegateAction) return; + const delegate = async (resource: ServiceResource, callbacks: ActionCallbacks = {}): Promise<void> => { + if (!delegateAction) return; return run(resource, delegateAction, callbacks); }; - const remove = (resource: ServiceResource, callbacks: ActionCallbacks = {}) => { - if (!removeAction) return; + const remove = async (resource: ServiceResource, callbacks: ActionCallbacks = {}): Promise<void> => { + if (!removeAction) return; return run(resource, removeAction, callbacks); };🤖 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/features/amUI/maskinporten/hooks/useMaskinportenResourceActions.ts` around lines 47 - 55, The functions delegate and remove return inconsistent types: when the corresponding action (delegateAction/removeAction) is absent they return undefined synchronously, but when present they return the Promise from run(...). Make the return type consistent by always returning a Promise<void>; update delegate and remove to return Promise.resolve() when the action is missing (or explicitly return Promise<void>), ensuring callers can always await the result; reference the delegate and remove functions and the run(resource, delegateAction/removeAction, callbacks) call sites when making the change.src/features/amUI/maskinporten/SupplierPageContent.tsx (1)
64-71: 💤 Low valueNon-null assertions on
partyandsupplierin mutation callback.The
party!andsupplier!assertions assume these values are defined when the remove action executes. While the query skips when undefined (line 63), the mutation callback is defined unconditionally.If user triggers removal before data loads (edge case), this could throw. The risk is low since UI guards exist, but explicit guards would be safer.
🛡️ Defensive alternative
const { remove, isLoading } = useMaskinportenResourceActions({ remove: (resource) => + party && supplier + ? removeSupplierResource({ + party, + supplier, + resource: resource.identifier, + }).unwrap() + : Promise.reject(new Error('Missing party or supplier')), - removeSupplierResource({ - party: party!, - supplier: supplier!, - resource: resource.identifier, - }).unwrap(), });🤖 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/features/amUI/maskinporten/SupplierPageContent.tsx` around lines 64 - 71, The mutation callback passed to useMaskinportenResourceActions uses non-null assertions on party and supplier (party!, supplier!) which can throw if the user triggers remove before data loads; update the remove callback (the remove property in the useMaskinportenResourceActions call) to guard that party and supplier are defined before calling removeSupplierResource — if either is missing, reject/return an appropriate error or no-op (e.g., return Promise.reject/new Error or resolve) so the mutation never dereferences undefined; reference the remove property, useMaskinportenResourceActions hook, and removeSupplierResource function when making this change.src/features/amUI/maskinporten/ConsumersTab.tsx (1)
38-42: 💤 Low value
getUserLinkperforms lookup on each call.The
find()iterates throughconsumersfor each user. If the search component calls this frequently or with many users, consider memoizing a lookup map.Current implementation is acceptable for typical list sizes; flagging as optional.
♻️ Optional: Pre-compute lookup map
+ const consumerOrgNrById = React.useMemo(() => { + const map = new Map<string, string>(); + consumers?.forEach((c) => { + if (c.party.organizationIdentifier) { + map.set(c.party.id, c.party.organizationIdentifier); + } + }); + return map; + }, [consumers]); + <MaskinportenUserSearch connections={consumers} isLoading={isLoading} error={error} emptyText={t('maskinporten_page.no_consumers')} canDelegate={false} - getUserLink={(user) => { - const orgNr = consumers?.find((c) => c.party.id === user.id)?.party - .organizationIdentifier; - return orgNr ? `/maskinporten/consumer/${orgNr}` : ''; - }} + getUserLink={(user) => { + const orgNr = consumerOrgNrById.get(user.id); + return orgNr ? `/maskinporten/consumer/${orgNr}` : ''; + }} />🤖 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/features/amUI/maskinporten/ConsumersTab.tsx` around lines 38 - 42, getUserLink currently scans consumers on every call by using consumers.find(...) which is inefficient if invoked frequently; precompute a lookup map from user id to party.organizationIdentifier (e.g., build a Map or object from the consumers array) once in the ConsumersTab component (using useMemo if React hooks are available) and then have getUserLink read orgNr from that map (falling back to '' when missing), referencing the existing getUserLink function, consumers array, and party.organizationIdentifier field to locate where to replace the per-call find with a constant-time lookup.
🤖 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/features/amUI/maskinporten/hooks/useMaskinportenResourceActions.ts`:
- Line 35: The early return in useMaskinportenResourceActions that does "if
(!resource.identifier) return;" silently skips execution; instead, detect the
missing identifier and call the provided onError callback (e.g., onError(new
Error("Missing resource.identifier") or a descriptive Error/Message) so callers
are notified, then return or abort; update the block in
useMaskinportenResourceActions where resource.identifier is checked to invoke
onError with a clear message referencing the resource and identifier before
exiting.
---
Outside diff comments:
In `@src/features/amUI/maskinporten/ScopeInfo.tsx`:
- Around line 259-268: Replace the native disabled prop on the DsButton with
aria-disabled and ensure the click handler early-exits when the button should be
disabled: change the DsButton prop from disabled={isActionLoading} to
aria-disabled={isActionLoading}, and update handleRemoveResource (or add a small
wrapper onClick) to check isActionLoading and return immediately if true so
clicks are ignored while aria-disabled is set; keep the existing variant, icon,
and label intact.
---
Nitpick comments:
In `@src/features/amUI/maskinporten/ConsumersTab.tsx`:
- Around line 38-42: getUserLink currently scans consumers on every call by
using consumers.find(...) which is inefficient if invoked frequently; precompute
a lookup map from user id to party.organizationIdentifier (e.g., build a Map or
object from the consumers array) once in the ConsumersTab component (using
useMemo if React hooks are available) and then have getUserLink read orgNr from
that map (falling back to '' when missing), referencing the existing getUserLink
function, consumers array, and party.organizationIdentifier field to locate
where to replace the per-call find with a constant-time lookup.
In `@src/features/amUI/maskinporten/hooks/useMaskinportenResourceActions.ts`:
- Around line 47-55: The functions delegate and remove return inconsistent
types: when the corresponding action (delegateAction/removeAction) is absent
they return undefined synchronously, but when present they return the Promise
from run(...). Make the return type consistent by always returning a
Promise<void>; update delegate and remove to return Promise.resolve() when the
action is missing (or explicitly return Promise<void>), ensuring callers can
always await the result; reference the delegate and remove functions and the
run(resource, delegateAction/removeAction, callbacks) call sites when making the
change.
In `@src/features/amUI/maskinporten/SupplierPageContent.tsx`:
- Around line 64-71: The mutation callback passed to
useMaskinportenResourceActions uses non-null assertions on party and supplier
(party!, supplier!) which can throw if the user triggers remove before data
loads; update the remove callback (the remove property in the
useMaskinportenResourceActions call) to guard that party and supplier are
defined before calling removeSupplierResource — if either is missing,
reject/return an appropriate error or no-op (e.g., return Promise.reject/new
Error or resolve) so the mutation never dereferences undefined; reference the
remove property, useMaskinportenResourceActions hook, and removeSupplierResource
function when making this change.
🪄 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: e0a06212-e7d5-44a5-a380-0d4a1647fdca
📒 Files selected for processing (13)
src/features/amUI/common/DelegationModal/EditModal.tsxsrc/features/amUI/maskinporten/ConsumerPage.tsxsrc/features/amUI/maskinporten/ConsumerPageContent.tsxsrc/features/amUI/maskinporten/ConsumersTab.tsxsrc/features/amUI/maskinporten/ScopeInfo.tsxsrc/features/amUI/maskinporten/ScopeSearchControls.tsxsrc/features/amUI/maskinporten/SupplierPageContent.tsxsrc/features/amUI/maskinporten/hooks/useMaskinportenResourceActions.tssrc/localizations/en.jsonsrc/localizations/no_nb.jsonsrc/localizations/no_nn.jsonsrc/routes/Router/Router.tsxsrc/routes/paths/amUIPath.tsx
✅ Files skipped from review due to trivial changes (1)
- src/routes/paths/amUIPath.tsx
sonwit
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/amUI/maskinporten/SupplierPageContent.tsx (1)
85-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the supplier page back link on the suppliers tab.
Line 85 returns to
?tab=suppliers, but Line 96 still pointsPageContainerat the bare Maskinporten route. That makes the page header/back action land on a different tab than the delete flow.Suggested fix
- backUrl={`/${amUIPath.Maskinporten}`} + backUrl={`/${amUIPath.Maskinporten}?tab=suppliers`}🤖 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/features/amUI/maskinporten/SupplierPageContent.tsx` around lines 85 - 96, The PageContainer back link currently uses backUrl={`/${amUIPath.Maskinporten}`} which conflicts with the delete flow that navigates to `/${amUIPath.Maskinporten}?tab=suppliers`; update the PageContainer backUrl prop to include the suppliers tab query so it matches the navigation used in the remove handler (e.g., set backUrl to `/${amUIPath.Maskinporten}?tab=suppliers`), ensuring PageContainer, its backUrl prop, and amUIPath.Maskinporten use the same destination as the navigate call.
🤖 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.
Outside diff comments:
In `@src/features/amUI/maskinporten/SupplierPageContent.tsx`:
- Around line 85-96: The PageContainer back link currently uses
backUrl={`/${amUIPath.Maskinporten}`} which conflicts with the delete flow that
navigates to `/${amUIPath.Maskinporten}?tab=suppliers`; update the PageContainer
backUrl prop to include the suppliers tab query so it matches the navigation
used in the remove handler (e.g., set backUrl to
`/${amUIPath.Maskinporten}?tab=suppliers`), ensuring PageContainer, its backUrl
prop, and amUIPath.Maskinporten use the same destination as the navigate call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3520412-48b7-4d6f-9778-50a6cc1f4055
📒 Files selected for processing (3)
src/features/amUI/maskinporten/ConsumerPageContent.tsxsrc/features/amUI/maskinporten/DelegatedResourcesSection.tsxsrc/features/amUI/maskinporten/SupplierPageContent.tsx
✅ Files skipped from review due to trivial changes (1)
- src/features/amUI/maskinporten/DelegatedResourcesSection.tsx
…error state management; add DelegatedResourcesSection styles.
…ctions; refactor DelegatedResourcesSection to accept editModal as a prop
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/features/amUI/maskinporten/ScopeInfo.tsx`:
- Around line 54-67: supplierOrgNumber is set to '' when toParty.orgNumber is
missing, but the delegate path (delegate in useMaskinportenResourceActions) and
addSupplierResource calls are not guarded, allowing delegation attempts with
supplier: ''. Fix by early-checking supplierOrgNumber wherever delegation or
addSupplierResource is wired (the delegate callback passed into
useMaskinportenResourceActions, the handleAddResource flow, and the button
enablement logic referenced around the other occurrences), and prevent or
short-circuit the action if supplierOrgNumber is falsy: disable the "give POA"
button when supplierOrgNumber is empty, and have delegate/handleAddResource
return/fail fast before calling addSupplierResource({ supplier:
supplierOrgNumber, ... }) when supplierOrgNumber === ''.
In `@src/features/amUI/maskinporten/SupplierPage.tsx`:
- Around line 62-63: The current notFound logic (const notFound = !!error ||
(!isLoading && !data?.length)) treats transport/server/auth errors as "not
found"; change it so notFound is true only when the request completed
successfully with an empty result or when the error is explicitly a 404.
Concretely, update the notFound calculation in SupplierPage (and the rendering
branches around lines where notFound is used) to check: notFound = (!isLoading
&& !error && Array.isArray(data) && data.length === 0) || (error?.status === 404
|| error?.response?.status === 404); and ensure all other error cases render a
generic error state (use the existing error handling path rather than the
notFound UI).
🪄 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: a0e95575-3c9c-4d45-8855-f23c448b708b
📒 Files selected for processing (9)
src/features/amUI/maskinporten/ConsumerPage.tsxsrc/features/amUI/maskinporten/ConsumerPageContent.tsxsrc/features/amUI/maskinporten/DelegatedResourcesSection.module.csssrc/features/amUI/maskinporten/DelegatedResourcesSection.tsxsrc/features/amUI/maskinporten/MaskinportenPage.module.csssrc/features/amUI/maskinporten/ScopeInfo.tsxsrc/features/amUI/maskinporten/SupplierPage.tsxsrc/features/amUI/maskinporten/SupplierPageContent.tsxsrc/features/amUI/maskinporten/hooks/useMaskinportenResourceActions.ts
💤 Files with no reviewable changes (1)
- src/features/amUI/maskinporten/MaskinportenPage.module.css
✅ Files skipped from review due to trivial changes (1)
- src/features/amUI/maskinporten/DelegatedResourcesSection.module.css
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/amUI/maskinporten/ConsumerPageContent.tsx
- src/features/amUI/maskinporten/ConsumerPage.tsx
allinox
approved these changes
May 19, 2026
allinox
left a comment
allinox
left a comment
Contributor
There was a problem hiding this comment.
I've tested it and looked through the code, and I only have a couple of small comments.
Great work bringing this feature to completion! 😄🙌
|
5 tasks
This was referenced Jun 2, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Legger til en Consumer-side for Maskinporten og rydder opp i BFF-API-et slik at consumer/supplier-kontekst er tydelig adskilt.
Ny side for "Consumer" - viser delegerte ressurser per consumer med mulighet til å fjerne.
Felles DelegatedResourcesSection brukes av både Supplier og Consumer.
Endret BFF edepunkter til suppliers/... og consumers/...
Nye consumer-endepunkter: hent delegerte ressurser, fjern ressurs.
Description
Related Issue(s)
Altinn/altinn-authorization-tmp#3117
Verification
Documentation
Summary by CodeRabbit
New Features
Refactor
Tests
Localization