chore: Update NGINX worker_processes to match CPU cores
#14872
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a variety of changes across multiple files, primarily focusing on enhancing the functionality related to organization code lists and text resources. Key updates include the addition of new hooks for managing code lists and text resources, modifications to existing components to handle blur events, and the introduction of new test files to validate these functionalities. Additionally, several dependencies have been updated, and new constants and enums have been introduced to support these changes. No modifications were made to exported or public entity declarations. Changes
Possibly related PRs
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditor.test.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditor.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. frontend/libs/studio-components/src/components/StudioCodelistEditor/StudioCodeListEditorRow/StudioCodeListEditorRow.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
skip-releasenotes
mlqn
changed the title
worker_processes to match CPU cores
mlqn
changed the title
worker_processes to match CPU coresworker_processes to match CPU cores
![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
🧹 Nitpick comments (2)
development/load-balancer/templates/nginx.conf.conf (1)
1-1: Good optimization for multicore environments!Changing from a fixed value of
1toautoallows NGINX to automatically determine the optimal number of worker processes based on the available CPU cores. This aligns perfectly with the PR objective to address the connection exhaustion issue by utilizing more CPU cores.This change will significantly improve the load balancer's ability to handle concurrent connections, as the total connection capacity is determined by
worker_processes * worker_connections. With the current worker_connections value of 1024, each CPU core can now handle up to 1024 connections, multiplying your overall capacity.charts/altinn-loadbalancer/templates/configmap.yaml (1)
7-7: Good optimization for Kubernetes deployments!Changing from a fixed value of
1toautoallows NGINX to automatically determine the optimal number of worker processes based on the available CPU cores in your Kubernetes pods. This change is consistent with the modification in the development environment configuration.This change will significantly improve the load balancer's ability to handle concurrent connections in production, addressing the issue described in the PR objectives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/altinn-loadbalancer/templates/configmap.yaml(1 hunks)development/load-balancer/templates/nginx.conf.conf(1 hunks)
🔇 Additional comments (2)
development/load-balancer/templates/nginx.conf.conf (1)
3-5: Consider increasing worker_connections if neededThe PR objectives mentioned NGINX rejecting requests with logs stating "1024 worker_connections are not enough." While increasing worker processes will help, you might still hit the connection limit if your traffic is very high.
If after implementing this change you continue to see connection exhaustion, consider also increasing the
worker_connectionsvalue. The optimal value depends on your system resources and traffic patterns, but common values range from 2048 to 4096 for high-traffic environments.charts/altinn-loadbalancer/templates/configmap.yaml (1)
9-9: Consider increasing worker_connections if neededSimilar to the development configuration, you might want to consider increasing the
worker_connectionsvalue if you continue to see connection exhaustion after this change.The optimal value depends on your production resources and traffic patterns, but common values range from 2048 to 4096 for high-traffic environments. Make sure to test any changes to this value under load to find the optimal configuration.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14872 +/- ##
=======================================
Coverage 95.83% 95.83%
=======================================
Files 1959 1959
Lines 25525 25525
Branches 2887 2887
=======================================
Hits 24462 24462
Misses 801 801
Partials 262 262 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (62)
frontend/packages/ux-editor/src/hooks/useLayoutSetIcon.test.tsx (1)
31-34: Use more specific assertions for expected iconsInstead of just asserting that the icon is not the default icon, consider checking for the specific expected icon type for more robust tests.
frontend/packages/ux-editor/src/components/Settings/SettingsTabs.tsx (1)
33-40: Consider extracting repeated alert to a variable or componentThe same alert component is duplicated in both tab contents. Consider extracting it to a variable or a separate component to adhere to the DRY principle.
+ const wipAlert = ( + <StudioAlert severity='info' className={classes.wipMessage}> + {t('ux_editor.settings.wip_message')} + </StudioAlert> + ); <StudioTabs.Content value={Tabs.Navigation}> - <StudioAlert severity='info' className={classes.wipMessage}> - {t('ux_editor.settings.wip_message')} - </StudioAlert> + {wipAlert} </StudioTabs.Content> <StudioTabs.Content value={Tabs.Database}> - <StudioAlert severity='info' className={classes.wipMessage}> - {t('ux_editor.settings.wip_message')} - </StudioAlert> + {wipAlert} </StudioTabs.Content>frontend/packages/ux-editor/src/hooks/useLayoutSetIcon.tsx (4)
13-25: Hook implementation is clean but could be more concise.The hook logic is easy to follow with sequential condition checks for different layout set types. However, consider using a switch statement or object mapping for better maintainability.
export const useLayoutSetIcon = ( layoutSetModel: LayoutSetModel, ): { icon: ReactElement; iconColor: StudioIconCardIconColors } => { - if (layoutSetModel.type == 'subform') return { icon: <ClipboardIcon />, iconColor: 'blue' }; - - if (layoutSetModel.task?.id == 'CustomReceipt') - return { icon: <ReceiptIcon />, iconColor: 'green' }; - if (layoutSetModel.task?.type == 'data') return { icon: <DataTaskIcon />, iconColor: 'blue' }; - if (layoutSetModel.task?.type == 'signing') return { icon: <SignTaskIcon />, iconColor: 'red' }; - if (layoutSetModel.task?.type == 'payment') return { icon: <CardIcon />, iconColor: 'yellow' }; - - return { icon: <QuestionmarkIcon />, iconColor: 'grey' }; + // Check type first + if (layoutSetModel.type === 'subform') return { icon: <ClipboardIcon />, iconColor: 'blue' }; + + // Then check task properties + const taskId = layoutSetModel.task?.id; + const taskType = layoutSetModel.task?.type; + + if (taskId === 'CustomReceipt') return { icon: <ReceiptIcon />, iconColor: 'green' }; + + // Map task types to icons and colors + const taskTypeMap = { + data: { icon: <DataTaskIcon />, iconColor: 'blue' as StudioIconCardIconColors }, + signing: { icon: <SignTaskIcon />, iconColor: 'red' as StudioIconCardIconColors }, + payment: { icon: <CardIcon />, iconColor: 'yellow' as StudioIconCardIconColors }, + }; + + return taskType && taskTypeMap[taskType] + ? taskTypeMap[taskType] + : { icon: <QuestionmarkIcon />, iconColor: 'grey' }; };
16-16: Use strict equality comparison.Using
==instead of===for equality checks is generally discouraged in TypeScript/JavaScript.- if (layoutSetModel.type == 'subform') return { icon: <ClipboardIcon />, iconColor: 'blue' }; + if (layoutSetModel.type === 'subform') return { icon: <ClipboardIcon />, iconColor: 'blue' };
18-19: Use strict equality comparison for task ID check.- if (layoutSetModel.task?.id == 'CustomReceipt') + if (layoutSetModel.task?.id === 'CustomReceipt')
20-22: Use strict equality comparison for task type checks.- if (layoutSetModel.task?.type == 'data') return { icon: <DataTaskIcon />, iconColor: 'blue' }; - if (layoutSetModel.task?.type == 'signing') return { icon: <SignTaskIcon />, iconColor: 'red' }; - if (layoutSetModel.task?.type == 'payment') return { icon: <CardIcon />, iconColor: 'yellow' }; + if (layoutSetModel.task?.type === 'data') return { icon: <DataTaskIcon />, iconColor: 'blue' }; + if (layoutSetModel.task?.type === 'signing') return { icon: <SignTaskIcon />, iconColor: 'red' }; + if (layoutSetModel.task?.type === 'payment') return { icon: <CardIcon />, iconColor: 'yellow' };frontend/packages/ux-editor/src/components/TaskNavigation/TaskCard.tsx (2)
23-25: Event handler has empty implementation.The onClick handler has a TODO comment but doesn't prevent default behavior. Consider adding
event.preventDefault()to ensure the event doesn't propagate unexpectedly.onClick={(event: MouseEvent<HTMLButtonElement>) => { /* TODO: Implement editing mode */ + event.preventDefault(); }
40-56: Consider adding accessibility attributes.The StudioIconCard component should include proper accessibility attributes, especially for interactive elements.
Consider adding an
aria-labelor similar accessibility attributes to the StudioIconCard to improve screen reader support.frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.ts (1)
1-7: Consider using a simple function instead of a static-only classThe static analysis tool correctly identifies that classes with only static members should be avoided. This approach adds unnecessary complexity without providing any benefits of object-oriented design.
-export class FileUtils { - static convertToFormData = (file: File): FormData => { - const formData = new FormData(); - formData.append('file', file); - return formData; - }; -} +export const convertToFormData = (file: File): FormData => { + const formData = new FormData(); + formData.append('file', file); + return formData; +};This refactoring maintains the exact same functionality while simplifying the code and following functional programming principles, which are generally preferred in modern JavaScript/TypeScript development.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-7: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.test.ts (1)
1-17: The test looks good, but consider improving the assertion method.The test correctly verifies that a File object is appended to FormData under the key "file". However, using
toBe()checks for reference equality, which assumes the implementation doesn't create a copy of the file. For more robust testing, consider checking file properties instead.- expect(retrievedFile).toBe(file); + expect(retrievedFile).toBeInstanceOf(File); + expect((retrievedFile as File).name).toBe(fileName); + expect((retrievedFile as File).type).toBe(fileType); + expect((retrievedFile as File).size).toBe(fileContent.length);frontend/dashboard/pages/OrgContentLibrary/utils.ts (1)
1-10: Function looks good, but consider a small optimization.The implementation correctly identifies organization context types. For a slight performance improvement, consider declaring the
notOrgContextsarray outside the function to avoid recreating it on each call.+const notOrgContexts: string[] = [ + SelectedContextType.Self, + SelectedContextType.All, + SelectedContextType.None, +]; + export function isOrg(contextType: string): boolean { - const notOrgContexts: string[] = [ - SelectedContextType.Self, - SelectedContextType.All, - SelectedContextType.None, - ]; return !notOrgContexts.includes(contextType); }frontend/packages/shared/src/contexts/ServicesContext.tsx (1)
11-11: Enhanced error handling.The added check for non-Axios errors improves error logging by ensuring that all error types are properly handled. This helps with debugging issues that might not come from HTTP requests.
However, consider using a more descriptive message with the error, such as:
- console.error(error); + console.error('Non-Axios error occurred in ServicesContext:', error);This would make the logs more informative when troubleshooting.
Also applies to: 40-43
frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.test.ts (1)
1-23: Well-structured test implementationThe test correctly verifies that the hook calls the expected function with the right parameters.
Consider adding tests for error handling scenarios to increase test coverage.
+ it('handles errors correctly', async () => { + queriesMock.getTextResourcesForOrg.mockRejectedValueOnce(new Error('Test error')); + const { result } = renderHookWithProviders(() => useTextResourcesForOrgQuery(org, languageMock)); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(result.current.error).toBeDefined(); + });frontend/libs/studio-components/src/components/StudioCodelistEditor/utils.ts (1)
77-80: Good implementation of the boolean code limit checkThe function correctly implements a limit for boolean code lists, which makes sense as boolean values typically have only two options.
Consider extracting the
booleanCodeLimitconstant outside the function to improve maintainability:+const BOOLEAN_CODE_LIMIT = 2; + export function isCodeLimitReached(codeList: CodeList, codeType: CodeListItemType): boolean { - const booleanCodeLimit = 2; - return codeType === CodeListItemType.Boolean && codeList.length >= booleanCodeLimit; + return codeType === CodeListItemType.Boolean && codeList.length >= BOOLEAN_CODE_LIMIT; }frontend/testing/playwright/components/DashboardHeader.ts (1)
1-17: Well-structured Playwright component class, but with an unnecessary constructor.The
DashboardHeaderclass is well designed, but the constructor doesn't add any functionality beyond what's provided by the parent class.Consider simplifying by removing the unnecessary constructor:
- constructor(page: Page, environment?: Environment) { - super(page, environment); - }The class will inherit the constructor from
BasePageautomatically.🧰 Tools
🪛 Biome (1.9.4)
[error] 8-10: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.ts (1)
1-15: Good implementation of the custom hook for fetching text resourcesThis custom hook is well-structured and follows React Query best practices:
- Proper type definitions for parameters and return values
- Effective use of context to access the service function
- Clean implementation of the query configuration
One minor observation: the query key only includes
QueryKey.TextResourcesForOrgbut doesn't include the organization ID or language parameters as part of the key. This could potentially cause issues with caching if you need to fetch resources for multiple organizations or languages.Consider including the org and language in the query key for proper cache management:
return useQuery<ITextResourcesWithLanguage>({ - queryKey: [QueryKey.TextResourcesForOrg], + queryKey: [QueryKey.TextResourcesForOrg, org, language], queryFn: () => getTextResourcesForOrg(org, language), });frontend/packages/shared/src/api/queries.ts (1)
190-190: Temporary mock with clear TODO commentThe function uses a mock response with a well-documented TODO comment that includes a GitHub issue reference, making it clear this is a temporary solution pending API implementation.
Consider separating the TODO comment to its own line for better readability:
-export const getTextResourcesForOrg = async (org: string, language: string): Promise<ITextResourcesWithLanguage> => Promise.resolve(textResourcesMock); // Todo: Replace with real API call when endpoint is ready. https://github.com/Altinn/altinn-studio/issues/14503 +// Todo: Replace with real API call when endpoint is ready. https://github.com/Altinn/altinn-studio/issues/14503 +export const getTextResourcesForOrg = async (org: string, language: string): Promise<ITextResourcesWithLanguage> => Promise.resolve(textResourcesMock);frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.test.ts (1)
43-57: Comprehensive cache update verificationGood test that validates both the API call and the subsequent cache update. Consider adding assertion messages to make test failures more descriptive.
You could make test failures more descriptive by adding messages to your expect statements:
- expect(updatedData).toEqual(expectedUpdatedData); + expect(updatedData).toEqual(expectedUpdatedData, 'Cache should be updated with API response');backend/tests/Designer.Tests/Controllers/ApiTests/DesignerEndpointsTestsBase.cs (1)
136-139: Consider checking directory existence before deletionThe deletion logic could throw an exception if the directory doesn't exist. Consider checking first with
Directory.Exists().if (!string.IsNullOrWhiteSpace(TestOrgPath)) { + if (Directory.Exists(TestOrgPath)) + { Directory.Delete(TestOrgPath, true); + } }backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs (1)
31-40: Minor typo in UpdateTextsForKeys documentationThere's a small typo in the documentation comment - "resouce" should be "resource".
- /// Updates values for specified keys in the text resouce. + /// Updates values for specified keys in the text resource.Additionally, based on the retrieved learning, this method assumes the text resource already exists. Consider adding this precondition to the documentation.
backend/tests/Designer.Tests/Controllers/OrgTextController/CreateResourceTests.cs (1)
1-44: Consider adding more test cases with invalid inputsWhile the current test covers valid inputs, consider adding tests for invalid inputs such as invalid language codes, malformed JSON payloads, or missing fields to ensure proper error handling.
frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.tsx (1)
79-82: Consider edge cases in route parameter extractionThe
extractSecondLastRouterParamfunction is simple but could fail with edge cases like URLs with only one segment. Consider adding safeguards.-function extractSecondLastRouterParam(pathname: string): string { - const pathnameArray = pathname.split('/'); - return pathnameArray[pathnameArray.length - 2]; -} +function extractSecondLastRouterParam(pathname: string): string { + const pathnameArray = pathname.split('/').filter(Boolean); + return pathnameArray.length > 1 ? pathnameArray[pathnameArray.length - 2] : ''; +}backend/tests/Designer.Tests/Infrastructure/GitRepository/AltinnOrgGitRepositoryTests.cs (1)
119-136: Fix method name typoThere's a typo in the method name: "CreatCodeList" should be "CreateCodeList" to match the naming pattern used in other tests.
-public async Task CreatCodeList_WithEmptyRepo_ShouldCreateCodeList(string repository, string codeListId) +public async Task CreateCodeList_WithEmptyRepo_ShouldCreateCodeList(string repository, string codeListId)backend/tests/Designer.Tests/Services/OrgTextsServiceTests.cs (1)
117-117: Fix typo in test method name.The method name contains a typo: "DoesNotExit" should be "DoesNotExist".
- public async Task UpdateTextsForKeys_ShouldCreateNewIdWhenTextResourceDoesNotExit(string repo, string lang) + public async Task UpdateTextsForKeys_ShouldCreateNewIdWhenTextResourceDoesNotExist(string repo, string lang)backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs (1)
65-65: Clarify comment about handling non-existent keys.The comment "handle if file not already exist" is inaccurate as the code below is actually handling the case where a specific key doesn't exist in the existing text resource.
- // handle if file not already exist + // Handle keys that don't exist in the current text resourcebackend/tests/Designer.Tests/Controllers/OrgCodeListController/CreateCodeListTests.cs (2)
52-57: Consider adding a null check for the deserialized response.
After deserializing the response intoresponseList, you immediately useresponseList.Counton lines 56–57. IfresponseListis null (due to a deserialization issue or unexpected response), this will throw an exception. Adding a quick null assertion before accessingCountcan make the tests more robust.+ Assert.NotNull(responseList); Assert.Equal(expectedResponse.Count, responseList.Count);
60-66: Use higher-level collection equality checks.
Within theforloop, you compare individual elements ofexpectedDataandactualData. Consider using a higher-level collection comparison approach (likeAssert.Equalon two lists orAssert.Collection) for better readability and to get more detailed error messages on failure.backend/tests/Designer.Tests/Controllers/OrgCodeListController/UploadCodeListTests.cs (2)
35-50: Validate the response object to avoid null references.
Deserializing toList<OptionListData>could result in a null if JSON parsing fails or the API response differs. Consider checking for null before further assertions to prevent test crashes and improve debugging.
101-111: Consider verifying content beyond the status code.
When testing null values, you only assertBadRequest. If you’d like stronger guarantees, you could verify the error message or structure in the response to confirm the code list was indeed invalid.frontend/dashboard/hooks/guards/useContextRedirectionGuard.ts (4)
23-24: Avoid immediate session storage writes within a conditional.
The current check and write to session storage occur outside auseEffect. This can lead to repeated writes during render cycles if the component re-renders andselectedContextTypeis notNone. To ensure consistency, consider moving this logic inside a stable effect.- if (selectedContextType !== SelectedContextType.None) { - typedSessionStorage.setItem('dashboard::selectedContext', selectedContextType); - } + useEffect(() => { + if (selectedContextType !== SelectedContextType.None) { + typedSessionStorage.setItem('dashboard::selectedContext', selectedContextType); + } + }, [selectedContextType]);
75-81: Check for potential naming confusion.
navigateToSelfcreates aDashboardRouteforcibly usingSelectedContextType.Selfwhile also reusingsubroute. IfselectedContextTypeis something else, this might cause confusion about the final context. Consider clarifying or adding logs for better debugging.
84-87: Consider handling cases wherelocationis undefined.
The calllocation.searchpresumes a global object is available. In certain test environments (or older React versions),locationmight not be defined. If so, you could fallback to an empty string.
101-103: Refine the check for context differences.
isTargetContextDifferentcomparesgetTargetContext(selectedContextType)toselectedContextType. You may reduce overhead by storing the result ofgetTargetContextto avoid calling it repeatedly in other parts of the code.backend/tests/Designer.Tests/Controllers/OrgTextController/UpdateResourceTests.cs (3)
24-48: Consider adding negative test scenarios.
TheUpdateResource_Returns200OK_WithValidInputtest verifies the expected behavior with valid input. Expanding coverage to include invalid or missing fields inupdateDictionarywould strengthen confidence in error handling.
50-77: Preserve broader variable integrity in the second test.
Currently, the test checks only the number of variables inTextUsingVariables. Consider asserting that other text elements with variables remain unchanged, ensuring no accidental modifications to unrelated text IDs.
91-105: Ensure consistent insertion order for new text elements.
When inserting new keys at index 0 insidePrepareExpectedResourceWithoutVariables, existing entries are pushed back. If maintaining original order is desired, consider appending them instead. Otherwise, document the intended new-element-first logic.frontend/packages/shared/src/mocks/queriesMock.ts (8)
87-87: Return realistic mock data for test reliability.
Currently returning an empty array may limit test coverage. Consider returning at least one mock code list to simulate real workflows.
201-205: Provide variant scenarios for text resources.
Returning the same mock resource each time ensures consistency, but adding slight variations in language or structure could improve test depth.
225-225: Consider returning a newly created code list.
createCodeListForOrgmight return a mock code list object or ID to help consumers verify that creation succeeded.
233-233: Return details for created text resources.
createTextResourcesForOrgcould return the newly created resource keys to confirm success in tests.
235-235: Validate input in mock deletion.
deleteCodeListForOrgalways resolves without verifying an ID. Optional checks or logging could surface potential mismatches in test usage.
263-263: Return updated code list object for better testing.
updateCodeListForOrgmight provide a modified code list or status message, letting tests confirm the update was applied.
265-265: Enhance updating text resources with a verifiable result.
Similar to code lists, returning updated text resource data strengthens test validation.
266-266: Provide success feedback for code list upload.
uploadCodeListForOrgmight return metadata (e.g., file name, status) to simulate real upload results in tests.frontend/dashboard/app/App.tsx (3)
25-39: Clear switch-based status handling.
UsingmergeQueryStatusesand a switch statement provides concise clarity. A fallback default case could handle unexpected states or logging as an extra safeguard.
50-72: Differentiated error messages are good but strictly bound to user error.
The logic distinguishes between user data errors and organization data errors. Consider a scenario where both fail simultaneously, ensuring it's still user-friendly.
92-103: Subroute guard covers known segments.
A default fallback could help surface or handle unknown routes gracefully. Otherwise, good handling for recognized subroutes.backend/src/Designer/Services/Interfaces/Organisation/IOrgCodeListService.cs (2)
43-51: Clarify documentation for UploadCodeList methodThe XML comments for
UploadCodeListstate that it "Creates a new code list in the org repository" which is identical to the comments forCreateCodeList. It would be more accurate to describe this method's specific functionality of uploading from a file./// <summary> -/// Creates a new code list in the org repository. +/// Uploads and creates a new code list from a file in the org repository. /// If the file already exists, it will be overwritten. /// </summary>
53-60: Fix typo in documentation for DeleteCodeList methodThere's a small grammatical error in the XML comment.
/// <summary> -/// Deletes an code list from the org repository. +/// Deletes a code list from the org repository. /// </summary>frontend/packages/shared/src/api/mutations.ts (1)
190-192: Address TODO comment with a concrete planThe TODO comments indicate these functions need to be updated once the endpoints are ready. Consider adding a GitHub issue number or a more specific timeline to track this work.
While there is a GitHub issue reference, it would be helpful to include an estimated timeline or milestone for when these placeholder implementations will be replaced with real API calls.
backend/tests/Designer.Tests/Services/OrgCodeListServiceTests.cs (2)
148-151: Fix trailing comma in JSON test dataThe JSON string has a trailing comma after the last element which may cause issues with some JSON parsers.
const string jsonCodeList = @"[ {""label"": ""someLabel"",""value"": ""someValue"" }, -]"; +]";
161-171: Useusingstatement for proper resource disposalThe memory stream is manually closed after the service call. It would be better to use a
usingstatement to ensure proper disposal even if an exception occurs.-var stream = new MemoryStream(codeListBytes); -IFormFile file = new FormFile(stream, 0, codeListBytes.Length, fileName, fileName); - -TargetOrg = TestDataHelper.GenerateTestOrgName(); -string targetRepository = TestDataHelper.GetOrgContentRepoName(TargetOrg); -await TestDataHelper.CopyOrgForTest(Developer, Org, Repo, TargetOrg, targetRepository); -var service = GetOrgCodeListService(); - -// Act -var codeListData = await service.UploadCodeList(TargetOrg, Developer, file); -stream.Close(); +using (var stream = new MemoryStream(codeListBytes)) +{ + IFormFile file = new FormFile(stream, 0, codeListBytes.Length, fileName, fileName); + + TargetOrg = TestDataHelper.GenerateTestOrgName(); + string targetRepository = TestDataHelper.GetOrgContentRepoName(TargetOrg); + await TestDataHelper.CopyOrgForTest(Developer, Org, Repo, TargetOrg, targetRepository); + var service = GetOrgCodeListService(); + + // Act + var codeListData = await service.UploadCodeList(TargetOrg, Developer, file); +}backend/tests/Designer.Tests/Controllers/OrgCodeListController/UpdateCodeListTests.cs (1)
42-43: Consider clarifying duplicate labels in test dataThere are duplicate entries with the same label "NumberValue" in the test data. While this might be intentional to test how the system handles duplicates, it could be confusing. Consider adding a comment explaining this choice or using distinct labels.
Also applies to: 93-94
backend/src/Designer/Controllers/Organisation/OrgCodeListController.cs (1)
132-155: Reassess returning all code lists after delete.Currently, the
DeleteCodeListmethod returns the entire set of code lists after removal. This could be inefficient and might not scale when organizations have many code lists. Consider returning only a confirmation or the metadata of the deleted item for better performance and clarity.backend/src/Designer/Services/Implementation/Organisation/OrgCodeListService.cs (3)
38-38: Use explicit list instantiation for clarity.The syntax
List<OptionListData> codeLists = [];may not be supported in earlier C# versions and could cause confusion. An explicit instantiation such asnew List<OptionListData>()is more universally recognized and clear.- List<OptionListData> codeLists = []; + List<OptionListData> codeLists = new List<OptionListData>();
103-107: Ensure robust handling of invalid attributes.Currently, an exception is thrown for the first invalid item encountered (
ValueorLabelis null). If partial acceptance of valid items is desired, or if more detailed error reporting is needed, consider collecting all invalid items to provide richer feedback or partial success responses.
68-78: Avoid returning all code lists for create/update operations.Returning the entire list of code lists after creating or updating one might be inefficient in organizations with many code lists. Consider returning only the newly created/updated code list and letting clients fetch the entire list separately if needed.
Also applies to: 80-91
frontend/testing/playwright/tests/org-library/codelists.spec.ts (1)
20-20: Serial mode usage notice.Enabling serial mode (
test.describe.configure({ mode: 'serial' })) ensures these tests run in sequence, preventing cross-test interference. This is beneficial if the environment is not stateless. Retraining parallel test execution or implementing thorough cleanup steps could make the tests more scalable.frontend/testing/playwright/pages/OrgLibraryPage/CodeLists.ts (4)
28-35: Consider making timeout configurableThe hardcoded timeout of 8000ms might be problematic in environments with varying performance characteristics.
- await expect(modalTitle).toBeVisible({ timeout: 8000 }); + const modalTimeout = process.env.MODAL_TIMEOUT || 8000; + await expect(modalTitle).toBeVisible({ timeout: parseInt(modalTimeout) });
45-51: Potential code duplicationThe methods
clickOnAddAlternativeButtonandclickOnAddItemButtonappear to perform identical actions. Consider consolidating them if they're intended to do the same thing.- public async clickOnAddAlternativeButton(): Promise<void> { - await this.page - .getByRole('button', { - name: this.textMock('code_list_editor.add_option'), - }) - .click(); - } // Later in the file... // Keep this method and use it in place of clickOnAddAlternativeButton public async clickOnAddItemButton(): Promise<void> { await this.page .getByRole('button', { name: this.textMock('code_list_editor.add_option'), }) .click(); }Also applies to: 107-113
161-168: Consider more robust file path handlingThe current approach to file path management might be brittle if file locations change. Consider using a more centralized approach for test assets.
- await fileChooser.setFiles(path.join(__dirname, fileName)); + // Assuming you have a constant or config for test assets directory + const TEST_ASSETS_DIR = process.env.TEST_ASSETS_DIR || path.join(__dirname, '../../../assets'); + await fileChooser.setFiles(path.join(TEST_ASSETS_DIR, fileName));
190-203: Improve selector robustnessThe XPath selector
xpath=..used to navigate to the parent element is fragile. Consider using more robust selectors.- const accordion = accordionTitle.locator('xpath=..'); + // Using data attributes or more specific selectors would be more robust + const accordionId = await accordionTitle.getAttribute('id'); + const accordion = this.page.locator(`[aria-labelledby="${accordionId}"]`);
🛑 Comments failed to post (5)
frontend/packages/ux-editor/src/components/TaskNavigation/TaskCardBar.tsx (2)
16-22: 🛠️ Refactor suggestion
Ensure layoutSetsModel.sets exists before mapping
There's a potential runtime error if
layoutSetsModel.setsis undefined. Add a null check or provide a fallback empty array.return ( <div className={classes.container}> - {layoutSetsModel.sets.map((layoutSetModel) => ( + {layoutSetsModel?.sets?.map((layoutSetModel) => ( <TaskCard key={layoutSetModel.id} layoutSetModel={layoutSetModel} /> ))} </div> );📝 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 ( <div className={classes.container}> {layoutSetsModel?.sets?.map((layoutSetModel) => ( <TaskCard key={layoutSetModel.id} layoutSetModel={layoutSetModel} /> ))} </div> );
14-14: 🛠️ Refactor suggestion
Add error handling for failed queries
The component only handles the loading state but lacks error handling. If the query fails, the component will render with potentially undefined data.
- if (layoutSetsPending) return null; + if (layoutSetsPending || !layoutSetsModel) return null;📝 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.if (layoutSetsPending || !layoutSetsModel) return null;frontend/packages/ux-editor/src/components/TaskNavigation/TaskCard.tsx (2)
52-54:
⚠️ Potential issueButton needs an onClick handler.
The primary button doesn't have an onClick handler defined, which makes it non-functional.
-<StudioButton color='second' variant='primary'> +<StudioButton + color='second' + variant='primary' + onClick={() => { + /* TODO: Implement navigation to UX editor */ + }} +> {t('ux_editor.task_card.ux_editor')} </StudioButton>📝 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.<StudioButton color='second' variant='primary' onClick={() => { /* TODO: Implement navigation to UX editor */ }} > {t('ux_editor.task_card.ux_editor')} </StudioButton>
14-38: 💡 Verification agent
🧩 Analysis chain
Implement TODOs for edit and delete functionality.
The component has TODOs for the edit and delete functionality. These need to be implemented before the component is fully functional.
🏁 Script executed:
#!/bin/bash # Check if there are other TODO comments in the codebase related to TaskCard functionality rg "TODO.*TaskCard" --type ts --type tsx rg "TODO.*task.*edit" --type ts --type tsx rg "TODO.*task.*delete" --type ts --type tsxLength of output: 218
Below is a revised approach to help verify and address the concerns raised. It appears that the initial search failed due to an unrecognized file type ("tsx"). To gather the necessary context, please run the following updated shell script that registers the TSX file type and searches for the exact TODO comments in the TaskCard component:
Once you verify that these TODOs still exist in the file (
frontend/packages/ux-editor/src/components/TaskNavigation/TaskCard.tsx), please address them by implementing the actual editing and deletion logic rather than leaving placeholder comments.
Action Item: Implement Functional Edit and Delete Handlers in TaskCard
- Replace the editing mode placeholder (
/* TODO: Implement editing mode */) with code that toggles the edit view or opens an editor modal.- Replace the delete callback placeholder (
/* TODO: Call delete on task */) with logic to call the proper delete function or API, ensuring that it updates the state accordingly.Please ensure that these functionalities are fully implemented before merging the component changes.
🏁 Script executed:
#!/bin/bash # Register TSX file type for ripgrep and search for TaskCard-related TODOs in TSX files # Search for the TODO comment for implementing the editing mode rg --type-add 'tsx:*.tsx' "TODO: Implement editing mode" --type tsx || echo "No 'editing mode' TODO found in TaskCard" # Search for the TODO comment for calling delete on task rg --type-add 'tsx:*.tsx' "TODO: Call delete on task" --type tsx || echo "No 'delete' TODO found in TaskCard"Length of output: 487
Action Required: Implement Functional Edit and Delete Handlers
The
TaskCardcomponent infrontend/packages/ux-editor/src/components/TaskNavigation/TaskCard.tsxstill contains the following placeholder TODO comments:
Editing Functionality:
Replace/* TODO: Implement editing mode */with logic to toggle an edit mode or open an editor modal.Deletion Functionality:
Replace/* TODO: Call delete on task */with handling that properly deletes the task (updating state and/or making necessary API calls).Please implement these functionalities to ensure that the component is fully operational before merging this PR.
backend/tests/Designer.Tests/_TestData/Repositories/testUser/ttd/org-content/Codelists/codeListTrailingComma.json (1)
1-3:
⚠️ Potential issueFix invalid JSON trailing comma
The JSON file contains a trailing comma after the last item in the array, which is not valid according to the JSON specification. Although some parsers might accept this, it's better to follow the standard for broader compatibility.
[ - { "label": "someLabel", "value": "someValue"}, + { "label": "someLabel", "value": "someValue"} ]📝 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.[ { "label": "someLabel", "value": "someValue"} ]🧰 Tools
🪛 Biome (1.9.4)
[error] 2-3: Expected an array, an object, or a literal but instead found ']'.
Expected an array, an object, or a literal here.
(parse)
Description
We recently experienced an issue in production where NGINX was rejecting all requests because all available connections were in use. Here is a sample of logs that were produced at that time:
I have investigated a bit and didn’t notice any problems with connections remaining needlessly open, but I noticed that
worker_processeswas set to1, which means that it only uses a 1 process on 1 core, instead of using all available cores we have in dev/staging/production. I changed it toautowhich will allow NGINX to run 1 process for each available core. This will significantly increase the number of possible connections (number of available CPUs * worker_connections) and should also improve performances.Related Issue(s)
Verification
Summary by CodeRabbit