Make signee list form work in Summary2 in different task. #3871
Conversation
bjorntore
added
kind/bug
📝 WalkthroughWalkthroughThe changes introduce taskId resolution logic allowing override-driven behavior via context. TaskId is now retrieved from TaskOverrides context with a fallback to query parameters, then passed to the fetchSigneeList function. The API function signature is expanded to accept an optional taskId parameter, conditionally appending it to the URL, and results are sorted alphabetically by signee name. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
bjorntore
added
kind/product-feature
|
Feel free to adjust the code yourselves if you want. I'm way out of my comfort zone. |
|
/publish |
PR release:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/layout/SigneeList/api.ts (1)
34-43: Based on my verification, I can now provide the rewritten review comment:taskId requirement in query gate is unnecessarily strict and breaks existing tests.
The
signingQueries.signeeListrequires all three values to be truthy before executing:queryFn: partyId && instanceGuid && taskId ? () => fetchSigneeList(...) : skipToken,However,
fetchSigneeListacceptstaskIdas optional (line 45), and the backend endpoint supports calls without it (using a default task). The test suite callsfetchSigneeList(partyId, instanceGuid)withouttaskIdand expects success (lines 40, 75, 94, 101, 130 in api.test.ts).This creates a regression: when rendering
SigneeListSummarywithout a task override, the query won't execute even though the backend can handle it.Relax the query gate to match the function signature:
- queryFn: partyId && instanceGuid && taskId ? () => fetchSigneeList(partyId, instanceGuid, taskId) : skipToken, + queryFn: + partyId && instanceGuid + ? () => fetchSigneeList(partyId, instanceGuid, taskId) + : skipToken,
🧹 Nitpick comments (3)
src/layout/SigneeList/SigneeListSummary.tsx (1)
12-32: Safer task override handling and interaction with taskId‑gated queryThe override/fallback chain
taskIdFromTaskOverrides ?? taskIdFromQueryis clear and matches the PR goal, but there are two things to double‑check:
- Context safety
IfuseTaskOverrides()can ever returnnull/undefined(e.g., component rendered outside the provider or a nullable default increateContext), the destructuring on Line 27 will throw. A more defensive pattern avoids that:- const { taskId: taskIdFromTaskOverrides } = useTaskOverrides(); + const taskOverrides = useTaskOverrides(); + const taskIdFromTaskOverrides = taskOverrides?.taskId;
- Behavior when no taskId exists
With the updated query logic insigningQueries.signeeList, the query will only run whentaskIdis truthy. If there are any flows whereSigneeListSummarypreviously worked without ataskId(backend using a default task), those will now silently stop fetching data. Please confirm thatSigneeListSummaryis only used in contexts where either the route param ortaskOverrides.taskIdis always defined; otherwise you may want to let the query run without a taskId and let the backend pick the default.src/layout/SigneeList/api.ts (2)
45-50: URL‑encode taskId when constructing the signing endpoint URLWhen appending the
taskIdas a query parameter, it’s safer to URL‑encode it in case the ID ever contains characters that need escaping:-export async function fetchSigneeList(partyId: string, instanceGuid: string, taskId?: string): Promise<SigneeState[]> { - let url = `${appPath}/instances/${partyId}/${instanceGuid}/signing`; - - if (taskId) { - url = url.concat(`?taskId=${taskId}`); - } +export async function fetchSigneeList(partyId: string, instanceGuid: string, taskId?: string): Promise<SigneeState[]> { + let url = `${appPath}/instances/${partyId}/${instanceGuid}/signing`; + + if (taskId) { + url = `${url}?taskId=${encodeURIComponent(taskId)}`; + }This keeps behavior identical for simple IDs while avoiding subtle bugs if the task naming scheme ever changes.
55-55: Consider avoidingtoSortedfor broader runtime compatibilitySorting the signee list is a good addition, but
Array.prototype.toSortedis a relatively new JS feature and may not be available in all supported runtimes without proper target/lib or polyfills:return parsed.signeeStates.toSorted((a, b) => (a.name ?? '').localeCompare(b.name ?? ''));To be safe across older browsers/test environments, you can achieve the same effect with widely‑supported APIs:
- return parsed.signeeStates.toSorted((a, b) => (a.name ?? '').localeCompare(b.name ?? '')); + return parsed.signeeStates + .slice() + .sort((a, b) => (a.name ?? '').localeCompare(b.name ?? ''));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/layout/SigneeList/SigneeListSummary.tsx(2 hunks)src/layout/SigneeList/api.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganytype or type casting (as type) in TypeScript code; improve typing by avoiding casts andanys when refactoring
Use objects for managing query keys and functions, andqueryOptionsfor sharing TanStack Query patterns across the system for central management
Files:
src/layout/SigneeList/SigneeListSummary.tsxsrc/layout/SigneeList/api.ts
{**/*.module.css,**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and leverage Digdir Design System components when possible
Files:
src/layout/SigneeList/SigneeListSummary.tsxsrc/layout/SigneeList/api.ts
🧠 Learnings (1)
📚 Learning: 2025-11-25T12:53:54.399Z
Learnt from: CR
Repo: Altinn/app-frontend-react PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:53:54.399Z
Learning: Applies to **/*.test.{ts,tsx} : Use `renderWithProviders` from `src/test/renderWithProviders.tsx` when testing components that require form layout context
Applied to files:
src/layout/SigneeList/SigneeListSummary.tsx
🧬 Code graph analysis (2)
src/layout/SigneeList/SigneeListSummary.tsx (2)
src/core/contexts/TaskOverrides.tsx (1)
useTaskOverrides(32-32)src/layout/SigneeList/api.ts (1)
useSigneeList(58-64)
src/layout/SigneeList/api.ts (1)
src/utils/urls/appUrlHelper.ts (1)
appPath(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
olemartinorg
left a comment
olemartinorg
left a comment
There was a problem hiding this comment.
Looks good! 🥳
Approving, assuming that:
- It's backwards compatible (backend won't complain if the taskId is passed to older versions not supporting it)
- You test it manually
Of course, automated tests are preferable, but at the same time this change is pretty low-risk of getting reverted
|
2 participants
Description
In this app lib PR I'm adding support for a taskId query param to the SingingController endpoints, so that we can use it to create a summary2 of the SigneeList component in another task than the original signing task.
This PR starts passing the taskOverrides taskId to the backend.
If tests are needed, I'd love some help.
Test/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.