Use new update tags endpoint and update validations from result #3641
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a version-gated attachment Set Tags API with legacy fallback, backend-backed attachment removal, widened AxiosResponse (.data) consumption across network and consumers, validation-type renames and new mapping utilities, widespread identifier renames from dataModelGuid → dataElementId, URL/versioning helpers, and corresponding test/mock updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests
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 |
cammiida
added
kind/product-feature
cammiida
force-pushed
the
feature/validate-tags
branch
from
aa3632b to
0ecc7b5
Compare
src/utils/versioning/versions.ts
Outdated
| @@ -8,6 +8,7 @@ export const FEATURE_VERSION_MAP = { | |||
| NEW_ATTACHMENTS_API: '8.5.0.153', | |||
| PDF_PREVIEW_BUTTON: '8.5.0.157', | |||
| APP_LANGUAGES_IN_ANONYMOUS: '8.5.6.180', | |||
| SET_TAGS_ENDPOINT: '8.8.0.0', | |||
There was a problem hiding this comment.
Needs to wait until this is verified (that is, the new endpoint is released and the release version corresponds with the version set here)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/network/networking.ts (1)
31-37: Unify return typing and config generic for httpPost to preserve T and align with other wrappersReturn type is widened to AxiosResponse due to missing generic on AxiosResponse, and config isn’t generic over D. Keep the existing param order to avoid churn, but tighten the types.
Apply:
-export async function httpPost<T, D = unknown>( - url: string, - options?: AxiosRequestConfig, - data?: D, -): Promise<AxiosResponse> { - return axios.post<T, AxiosResponse<T>, D>(url, data, options); -} +export async function httpPost<T, D = unknown>( + url: string, + options?: AxiosRequestConfig<D>, + data?: D, +): Promise<AxiosResponse<T>> { + return axios.post<T, AxiosResponse<T>, D>(url, data, options); +}src/features/validation/backendValidation/backendValidationUtils.ts (1)
164-168: Remove unreachable legacy fallbackThis condition is always false (
legacyText === issue.code). Drop it to avoid confusion.- // Fallback to old behavior if source not set. - const legacyText = issue.code; - if (legacyText !== issue.code) { - return { key: legacyText }; - }
🧹 Nitpick comments (16)
src/utils/versioning/versions.ts (1)
11-12: Add unit test for SET_TAGS_ENDPOINT
Extend existing isFeatureSupported tests to assert the newSET_TAGS_ENDPOINTthreshold (8.8.0.0) via appSupportsSetTagsEndpoint.src/features/validation/index.ts (1)
106-115: Type rename is fine; offer a compatibility alias to reduce churnIf downstreams still import the old name, provide a temporary alias.
export type BackendValidationIssuesWithSource = { source: string; issues: BackendValidationIssue[]; }; + +// Backwards-compat alias (deprecated; remove after callers migrate) +export type BackendValidationIssueGroupListItem = BackendValidationIssuesWithSource;src/features/validation/backendValidation/backendValidationUtils.ts (1)
107-109: Type the shared empties to avoid implicit anyGive these precise types to satisfy our TS guideline and reduce accidental misuse.
-const emptyObject = {}; -const emptyArray = []; +const emptyObject: BackendFieldValidatorGroups = {}; +const emptyArray: BaseValidation[] = [];src/features/validation/backendValidation/BackendValidation.tsx (2)
25-28: Consider memoizing initial validator groupsRecomputing this object on each render may retrigger the effect due to identity changes. Memoize to stabilize deps.
-import type { BackendFieldValidatorGroups } from 'src/features/validation'; +import type { BackendFieldValidatorGroups } from 'src/features/validation'; +import { useMemo } from 'react'; @@ -const initialValidatorGroups: BackendFieldValidatorGroups = mapBackendValidationsToValidatorGroups( - initialValidations, - defaultDataElementId, -); +const initialValidatorGroups: BackendFieldValidatorGroups = useMemo( + () => mapBackendValidationsToValidatorGroups(initialValidations, defaultDataElementId), + [initialValidations, defaultDataElementId], +);
31-31: Likewise, memoize task validationsSmall stability win for the effect deps.
-const initialTaskValidations = mapBackendIssuesToTaskValidations(initialValidations); +const initialTaskValidations = useMemo( + () => mapBackendIssuesToTaskValidations(initialValidations), + [initialValidations], +);src/features/instance/useProcessNext.tsx (1)
45-47: Deduplicate hook usage
useOnFormSubmitValidation()is called twice; keep one instance to avoid redundancy.- const onFormSubmitValidation = useOnFormSubmitValidation(); + const onFormSubmitValidation = useOnFormSubmitValidation(); @@ - const onSubmitFormValidation = useOnFormSubmitValidation();src/features/formData/FormData.test.tsx (1)
683-683: Fix typo in test description."Invaid data" → "Invalid data".
-describe('Invaid data', () => { +describe('Invalid data', () => {src/utils/urls/appUrlHelper.ts (2)
52-56: Confirm server expects comma-separated ignoredValidators; consider safer encoding.Using join(',') may not match API expectations if the backend expects repeated params. Prefer appending multiple ignoredValidators entries for robustness.
-export const getUpdateFileTagsUrl = (instanceId: string, dataGuid: string) => { - const searchParams = new URLSearchParams({ ignoredValidators: IgnoredValidators.join(',') }); - return `${appPath}/instances/${instanceId}/data/${dataGuid}/tags?${searchParams}`; -}; +export const getUpdateFileTagsUrl = (instanceId: string, dataGuid: string) => { + const params = new URLSearchParams(); + for (const v of IgnoredValidators) { + params.append('ignoredValidators', String(v)); + } + return `${appPath}/instances/${instanceId}/data/${dataGuid}/tags?${params.toString()}`; +};
1-1: Optional: add language support and reuse url helper for consistency.If this endpoint returns validations/messages, consider supporting language like other URLs.
-import { IgnoredValidators } from 'src/features/validation'; +import { IgnoredValidators } from 'src/features/validation'; +import { getQueryStringFromObject } from 'src/utils/urls/urlHelper'; @@ -export const getUpdateFileTagsUrl = (instanceId: string, dataGuid: string) => { - const searchParams = new URLSearchParams({ ignoredValidators: IgnoredValidators.join(',') }); - return `${appPath}/instances/${instanceId}/data/${dataGuid}/tags?${searchParams}`; -}; +export const getUpdateFileTagsUrl = (instanceId: string, dataGuid: string, language?: string) => { + const queryString = getQueryStringFromObject({ + language, + // If backend supports repeated params, switch to multiple keys instead of join + ignoredValidators: IgnoredValidators.join(','), + }); + return `${appPath}/instances/${instanceId}/data/${dataGuid}/tags${queryString}`; +};Also applies to: 52-56
src/features/formData/FormDataWrite.tsx (2)
285-287: Harden against undefined validationIssues from backend.Defensively handle missing validationIssues to avoid runtime errors.
- const validationIssueGroups: BackendValidationIssueGroups = Object.fromEntries( - validationIssues.map(({ source, issues }) => [source, issues]), - ); + const validationIssueGroups: BackendValidationIssueGroups = Object.fromEntries( + (validationIssues ?? []).map(({ source, issues }) => [source, issues]), + );
310-317: Align single-patch call with multi-patch by passing language.Multi-patch includes language via getUrlWithLanguage, but single-patch does not. This can cause inconsistent localization of validation messages.
- const { newDataModel, validationIssues, instance } = ( - await doPatchFormData(url, { + const { newDataModel, validationIssues, instance } = ( + await doPatchFormData(getUrlWithLanguage(url, currentLanguage.current), { patch, // Ignore validations that require layout parsing in the backend which will slow down requests significantly ignoredValidators: IgnoredValidators, }) ).data;src/features/processEnd/confirm/containers/ConfirmPage.test.tsx (1)
6-6: Use type-only import for AxiosResponse.Slight perf/readability win in TS contexts.
-import { AxiosResponse } from 'axios'; +import type { AxiosResponse } from 'axios';src/setupTests.ts (1)
14-14: Mock AxiosResponse without casting and include minimal required fields.Avoid
as AxiosResponsecasts; return a response object that satisfies the type.- doProcessNext: jest.fn<typeof doProcessNext>(async () => ({ data: getProcessDataMock() }) as AxiosResponse<IProcess>), + doProcessNext: jest.fn<typeof doProcessNext>(async () => + ({ + data: getProcessDataMock(), + status: 200, + statusText: 'OK', + headers: {}, + config: {}, + } satisfies AxiosResponse<IProcess>), + ),Also applies to: 29-29, 122-122
src/test/renderWithProviders.tsx (1)
423-437: Return AxiosResponse-shaped object for doPatchFormData mock.Keeps tests aligned with production shape and removes reliance on structural subtyping.
- .mockImplementation( - async (url: string, req: IDataModelPatchRequest): Promise<{ data: IDataModelPatchResponse }> => { + .mockImplementation( + async (url: string, req: IDataModelPatchRequest): Promise<AxiosResponse<IDataModelPatchResponse>> => { const model = structuredClone(models[url] ?? {}); applyPatch(model, req.patch); const afterProcessing = typeof mockBackend === 'function' ? mockBackend(model, url) : model; models[url] = afterProcessing; - return { - data: { - newDataModel: afterProcessing as object, - validationIssues: {}, - }, - }; + return { + data: { + newDataModel: afterProcessing as object, + validationIssues: {}, + }, + status: 200, + statusText: 'OK', + headers: {}, + config: {}, + }; }, );src/queries/queries.ts (1)
163-185: Add explicit return type and keep Axios semantics clear.Annotate return type for the new API to make the public surface unambiguous.
-export const doUpdateAttachmentTags = async ({ +export const doUpdateAttachmentTags = async ({ instanceId, dataGuid, setTagsRequest, }: { instanceId: string; dataGuid: string; setTagsRequest: SetTagsRequest; -}) => { +}): Promise<UpdateTagsResponse> => { const url = getUpdateFileTagsUrl(instanceId, dataGuid); const response = await httpPut<UpdateTagsResponse, SetTagsRequest>(url, setTagsRequest, { headers: { 'Content-Type': 'application/json', }, });src/features/attachments/AttachmentsStorePlugin.tsx (1)
29-36: Consider DI via AppMutations for doUpdateAttachmentTags.Directly importing the query makes test injection harder; exposing it via AppMutations keeps consistency with other mutations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
src/features/attachments/AttachmentsStorePlugin.tsx(9 hunks)src/features/attachments/index.ts(2 hunks)src/features/formData/FormData.test.tsx(2 hunks)src/features/formData/FormDataWrite.tsx(2 hunks)src/features/formData/types.ts(2 hunks)src/features/formData/useDataModelBindings.test.tsx(2 hunks)src/features/instance/useProcessNext.tsx(1 hunks)src/features/processEnd/confirm/containers/ConfirmPage.test.tsx(2 hunks)src/features/validation/backendValidation/BackendValidation.tsx(1 hunks)src/features/validation/backendValidation/backendValidationUtils.ts(2 hunks)src/features/validation/index.ts(1 hunks)src/layout/RepeatingGroup/Providers/OpenByDefaultProvider.test.tsx(1 hunks)src/queries/queries.ts(3 hunks)src/queries/types.ts(1 hunks)src/setupTests.ts(3 hunks)src/test/renderWithProviders.tsx(1 hunks)src/utils/network/networking.ts(1 hunks)src/utils/network/sharedNetworking.ts(1 hunks)src/utils/urls/appUrlHelper.ts(2 hunks)src/utils/versioning/versions.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/features/instance/useProcessNext.tsxsrc/features/formData/FormDataWrite.tsxsrc/layout/RepeatingGroup/Providers/OpenByDefaultProvider.test.tsxsrc/features/formData/useDataModelBindings.test.tsxsrc/features/validation/index.tssrc/utils/urls/appUrlHelper.tssrc/features/formData/types.tssrc/utils/network/networking.tssrc/features/attachments/index.tssrc/features/validation/backendValidation/BackendValidation.tsxsrc/queries/types.tssrc/features/processEnd/confirm/containers/ConfirmPage.test.tsxsrc/features/formData/FormData.test.tsxsrc/utils/versioning/versions.tssrc/utils/network/sharedNetworking.tssrc/features/validation/backendValidation/backendValidationUtils.tssrc/test/renderWithProviders.tsxsrc/features/attachments/AttachmentsStorePlugin.tsxsrc/queries/queries.tssrc/setupTests.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/layout/RepeatingGroup/Providers/OpenByDefaultProvider.test.tsxsrc/features/formData/useDataModelBindings.test.tsxsrc/features/processEnd/confirm/containers/ConfirmPage.test.tsxsrc/features/formData/FormData.test.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-22T13:53:28.252Z
Learnt from: CR
PR: Altinn/app-frontend-react#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.252Z
Learning: Applies to **/*.test.{ts,tsx} : In tests, use `renderWithProviders` from `src/test/renderWithProviders.tsx` to supply required form layout context
Applied to files:
src/test/renderWithProviders.tsx
🧬 Code graph analysis (11)
src/features/formData/FormDataWrite.tsx (3)
src/queries/queries.ts (2)
doPatchMultipleFormData(240-241)doPatchFormData(236-237)src/utils/urls/urlHelper.ts (1)
getUrlWithLanguage(137-151)src/features/validation/index.ts (1)
IgnoredValidators(23-27)
src/utils/urls/appUrlHelper.ts (1)
src/features/validation/index.ts (1)
IgnoredValidators(23-27)
src/features/formData/types.ts (1)
src/features/validation/index.ts (1)
BackendValidationIssuesWithSource(106-109)
src/features/attachments/index.ts (1)
src/features/validation/index.ts (1)
BackendValidationIssuesWithSource(106-109)
src/features/validation/backendValidation/BackendValidation.tsx (5)
src/features/validation/validationContext.tsx (1)
Validation(319-374)src/features/datamodel/DataModelsProvider.tsx (1)
DataModels(394-435)src/features/validation/index.ts (1)
BackendFieldValidatorGroups(117-119)src/features/validation/backendValidation/backendValidationUtils.ts (3)
useShouldValidateInitial(30-36)mapBackendValidationsToValidatorGroups(129-147)mapBackendIssuesToTaskValidations(110-127)src/features/validation/backendValidation/backendValidationQuery.ts (1)
useBackendValidationQuery(129-172)
src/features/processEnd/confirm/containers/ConfirmPage.test.tsx (2)
src/types/shared.ts (1)
IProcess(194-201)src/queries/queries.ts (1)
doProcessNext(98-99)
src/features/validation/backendValidation/backendValidationUtils.ts (1)
src/features/validation/index.ts (3)
BackendValidationIssue(205-217)BaseValidation(121-127)BackendFieldValidatorGroups(117-119)
src/test/renderWithProviders.tsx (1)
src/features/formData/types.ts (2)
IDataModelPatchRequest(41-44)IDataModelPatchResponse(46-50)
src/features/attachments/AttachmentsStorePlugin.tsx (7)
src/features/instance/InstanceContext.tsx (3)
useOptimisticallyUpdateDataElement(212-220)useLaxInstanceId(69-73)useOptimisticallyRemoveDataElement(221-229)src/features/applicationMetadata/ApplicationMetadataProvider.tsx (1)
useApplicationMetadata(67-67)src/utils/versioning/versions.ts (1)
appSupportsSetTagsEndpoint(67-69)src/queries/queries.ts (2)
SetTagsRequest(154-156)doUpdateAttachmentTags(163-184)src/features/datamodel/DataModelsProvider.tsx (1)
DataModels(394-435)src/features/validation/validationContext.tsx (1)
Validation(319-374)src/features/validation/backendValidation/backendValidationUtils.ts (3)
mapBackendIssuesToTaskValidations(110-127)mapBackendValidationsToValidatorGroups(129-147)mapValidatorGroupsToDataModelValidations(177-205)
src/queries/queries.ts (3)
src/features/validation/index.ts (1)
BackendValidationIssuesWithSource(106-109)src/utils/urls/appUrlHelper.ts (1)
getUpdateFileTagsUrl(52-56)src/utils/network/sharedNetworking.ts (1)
httpPut(15-17)
src/setupTests.ts (3)
src/queries/queries.ts (1)
doProcessNext(98-99)src/__mocks__/getProcessDataMock.ts (1)
getProcessDataMock(3-34)src/types/shared.ts (1)
IProcess(194-201)
⏰ 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). (2)
- GitHub Check: Install
- GitHub Check: Install
🔇 Additional comments (19)
src/utils/network/networking.ts (1)
43-45: MakehttpDeletegeneric (expose T) and type its return
Expose T onAxiosResponseto propagate payload types downstream—call-site scan found only uses of.dataand.status, so the generic default is safe.-export async function httpDelete(url: string, options?: AxiosRequestConfig): Promise<AxiosResponse> { - return axios.delete(url, options); -} +export async function httpDelete<T = unknown>( + url: string, + options?: AxiosRequestConfig, +): Promise<AxiosResponse<T>> { + return axios.delete<T, AxiosResponse<T>>(url, options); +}src/queries/types.ts (1)
3-12: doUpdateAttachmentTags is internal-only – exclusion from AppMutations/AppQueries is correct
Verified that doUpdateAttachmentTags is only called in AttachmentsStorePlugin and never exposed via AppQueriesContext or referenced in tests; keeping it in IgnoredQueriesAndMutations is intentional.src/utils/network/sharedNetworking.ts (1)
15-17: Make httpPut return type explicit and config generic over DAvoid silent breaking changes and align with the other wrappers.
-export async function httpPut<T, D = unknown>(url: string, data: D, config?: AxiosRequestConfig) { - return axios.put<T, AxiosResponse<T>, D>(url, data, config); -} +export async function httpPut<T, D = unknown>( + url: string, + data: D, + config?: AxiosRequestConfig<D>, +): Promise<AxiosResponse<T>> { + return axios.put<T, AxiosResponse<T>, D>(url, data, config); +}No
httpPutcall sites were found during search—please manually verify there are no existing usages relying on the implicit return type before merging.src/utils/versioning/versions.ts (1)
67-69: The script above will list all mock files and verify whetherFEATURE_VERSION_MAPandapplicationMetadataappear in them. Once you’ve reviewed its output, you can update any mocks to include the newSET_TAGS_ENDPOINTentry under your version map fixture.src/features/validation/index.ts (1)
112-115: Function signature updated; LGTMThe updated parameter type matches the renamed alias; logic unchanged.
src/features/validation/backendValidation/backendValidationUtils.ts (2)
110-114: LGTM: undefined-safe task validationsAccepting undefined and returning a typed empty array is correct and simplifies call sites.
129-147: LGTM: grouping field validations by sourceThe grouping logic is straightforward and correctly ignores task-level validations for initial load.
src/features/formData/useDataModelBindings.test.tsx (1)
162-163: LGTM: mocks now match Axios-like shapeWrapping patch responses in
{ data: ... }aligns tests with the new response contracts.Also applies to: 206-206
src/features/instance/useProcessNext.tsx (2)
75-75: LGTM: consume Axios payload via .dataExtracting
processfrom{ data }matches the updated query/mocking strategy.
109-112: No change required
useOnFormSubmitValidationreturnstruewhen validation errors exist (as seen in its call sites usinghasErrors) so the negationif (!hasValidationErrors)correctly firessetShowAllBackendErrors()only when there are no client-side errors.src/layout/RepeatingGroup/Providers/OpenByDefaultProvider.test.tsx (1)
145-149: LGTM: response wrapped in dataThis aligns with the global shift to AxiosResponse-shaped mocks.
src/features/formData/types.ts (1)
4-6: Approve changes – old type alias fully removed
No occurrences ofBackendValidationIssueGroupListItemfound insrc/**after verification.src/features/formData/FormData.test.tsx (1)
516-517: Axios-like mock shape is aligned with production code.Wrapping resolve payloads in { data } matches the new httpPatch return type usage. LGTM.
Also applies to: 578-579
src/features/formData/FormDataWrite.tsx (1)
267-274: Switch to AxiosResponse.data is correct.Destructuring from .data matches the updated httpPatch wrappers. LGTM.
Also applies to: 310-317
src/features/attachments/index.ts (1)
4-4: Type alignment with validation layer looks good.Import and DataPostResponse.validationIssues updated to BackendValidationIssuesWithSource[]. LGTM.
Also applies to: 36-37
src/features/processEnd/confirm/containers/ConfirmPage.test.tsx (1)
87-93: Non-resolving promise pattern is fine; watch for open handle warnings.Good adaptation to AxiosResponse. If Jest warns about open handles, reset the mock at the end of the test.
jest.mocked(doProcessNext).mockImplementation(async () => processNextPromise); + // After assertions (or in afterEach) + jest.mocked(doProcessNext).mockReset();src/queries/queries.ts (1)
154-162: Types for request/response look good.Exporting SetTagsRequest is appropriate; keep UpdateTagsResponse internal.
src/features/attachments/AttachmentsStorePlugin.tsx (2)
736-757: LGTM: remove mutation uses optimistic instance update.Good UX; errors are logged and the store action handles node data.
713-734: LGTM: upload mutation aligned with new API.Clear separation from legacy flow and solid error handling.
cammiida
force-pushed
the
feature/validate-tags
branch
from
3f0f01d to
653abf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/queries/queries.ts (4)
152-159: Types for request/response are clear; consider exporting the response typeIf callers/tests need to type the mutation result explicitly, exporting
UpdateTagsResponsecould help. If not needed externally, keeping it internal is fine.-type UpdateTagsResponse = { +export type UpdateTagsResponse = { tags: string[]; validationIssues: BackendValidationIssuesWithSource[]; };
161-179: New update-tags mutation: add explicit return type and tighten status handlingSmall refinements for a stronger API contract and clearer failures.
-export const doUpdateAttachmentTags = async ({ +export const doUpdateAttachmentTags = async ({ instanceId, dataGuid, setTagsRequest, }: { instanceId: string; dataGuid: string; setTagsRequest: SetTagsRequest; -}) => { +}): Promise<UpdateTagsResponse> => { const url = getUpdateFileTagsUrl(instanceId, dataGuid); - const response = await httpPut<UpdateTagsResponse, SetTagsRequest>(url, setTagsRequest, { - headers: { - 'Content-Type': 'application/json', - }, - }); - - if (response.status !== 200) { - throw new Error('Failed to update tags on attachment'); - } + const response = await httpPut<UpdateTagsResponse, SetTagsRequest>(url, setTagsRequest, { + headers: { 'Content-Type': 'application/json' }, + // Fail fast unless exactly 200, since we rely on response payload + validateStatus: (s) => s === 200, + }); return response.data; };
199-207: Include action and status in error message for easier debugging (optional)Improves observability without changing behavior.
- if (response.status !== 200) { - throw new Error('Failed to perform action'); - } + if (response.status !== 200) { + throw new Error(`Failed to perform action "${actionRequest.action ?? 'unknown'}" (HTTP ${response.status})`); + }
246-251: Make stateless form-data POST generic to avoidobjectThis tightens typing for callers without behavior changes.
-export const doPostStatelessFormData = async ( - url: string, - data: object, - options?: AxiosRequestConfig, -): Promise<object> => (await httpPost<object>(url, options, data)).data; +export const doPostStatelessFormData = async <T extends object>( + url: string, + data: T, + options?: AxiosRequestConfig, +): Promise<T> => (await httpPost<T>(url, options, data)).data;src/features/validation/backendValidation/backendValidationUtils.ts (4)
124-124: Avoid magic number for category; use a named mask/constant
category: 0is unclear. Prefer a named enum/member (e.g., ValidationMask.None/Task) or a local constant to document intent.If no suitable mask exists, add:
// near severityMap const TASK_VALIDATION_CATEGORY = 0;Then:
- taskValidations.push({ severity, message, category: 0, source, noIncrementalUpdates }); + taskValidations.push({ severity, message, category: TASK_VALIDATION_CATEGORY, source, noIncrementalUpdates });Please confirm downstream consumers expect 0 here (always-visible task validation).
139-145: Harden dictionary creation against prototype keys from backend-controlled strings
validatorGroupsuses a plain object and keys come fromvalidation.source(backend-controlled). Defensive: create a null-prototype map to avoid surprises with keys like__proto__/constructor.- const validatorGroups: BackendFieldValidatorGroups = {}; + const validatorGroups: BackendFieldValidatorGroups = Object.create(null);
183-185: Docs nit: data “elements”, not “types”The param was renamed; update the comment to match.
- // We need to clear all data types regardless if there are any validations or not - // Otherwise it would not update if there are no validations for a data type any more + // We need to clear all data elements regardless of whether there are any validations + // Otherwise it would not update if there are no validations for a data element anymore
164-169: Unreachable branch in legacy text fallback
legacyTextis assigned fromissue.code, solegacyText !== issue.codecan never be true. Remove the dead branch.- const legacyText = issue.code; - if (legacyText !== issue.code) { - return { key: legacyText }; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/features/attachments/AttachmentsStorePlugin.tsx(9 hunks)src/features/validation/backendValidation/backendValidationUtils.ts(3 hunks)src/queries/queries.ts(9 hunks)src/utils/network/networking.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/attachments/AttachmentsStorePlugin.tsx
- src/utils/network/networking.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/queries/queries.tssrc/features/validation/backendValidation/backendValidationUtils.ts
🧬 Code graph analysis (2)
src/queries/queries.ts (7)
src/features/instance/instanceUtils.ts (1)
removeProcessFromInstance(6-9)src/utils/network/networking.ts (1)
httpPost(31-37)src/types/shared.ts (2)
IInstance(70-89)IData(34-54)src/utils/urls/appUrlHelper.ts (3)
getUpdateFileTagsUrl(52-56)getActionsUrl(82-85)getDataModelTypeUrl(75-76)src/features/attachments/index.ts (1)
DataPostResponse(33-38)src/features/validation/index.ts (1)
BackendValidationIssuesWithSource(106-109)src/utils/network/sharedNetworking.ts (1)
httpPut(15-17)
src/features/validation/backendValidation/backendValidationUtils.ts (1)
src/features/validation/index.ts (4)
BackendValidationIssue(205-217)BaseValidation(121-127)BackendFieldValidatorGroups(117-119)DataModelValidations(88-90)
⏰ 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). (7)
- GitHub Check: Cypress Run Internal (1)
- GitHub Check: Cypress Run Internal (3)
- GitHub Check: Cypress Run Internal (6)
- GitHub Check: Cypress Run Internal (5)
- GitHub Check: Cypress Run Internal (4)
- GitHub Check: Cypress Run Internal (2)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (6)
src/queries/queries.ts (5)
43-43: Import of update-tags URL helper looks correctThe new helper is used below and the import path matches existing URL helpers.
68-72: Validation types import aligns with new response shapeBrings in the exact types needed for the update-tags response and custom validation config.
92-97: Consistent switch to returning.datafrom httpPostGood cleanup and consistent with the typed HTTP helpers.
112-112: Attachment upload functions: return.datadirectlyLooks good and consistent with axios defaults (non-2xx will reject).
Also applies to: 131-131
223-228: Typed post for subform entry: LGTMGeneric usage is correct and retains existing status check semantics.
src/features/validation/backendValidation/backendValidationUtils.ts (1)
136-147: mapBackendValidationsToValidatorGroups intentionally omits task-level validations; those are handled via mapBackendIssuesToTaskValidations and passed separately to updateBackendValidations. No change required.Likely an incorrect or invalid review comment.
src/features/validation/backendValidation/backendValidationUtils.ts
Outdated
Show resolved
Hide resolved
|
/publish |
PR release:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/utils/versioning/versions.ts (1)
11-12: Confirm the exact backend release version for the Update Tags endpointPlease verify that 8.7.0.0 is the first app-lib version that exposes the new update-tags endpoint. Also confirm that the runtime provides a four-part version (e.g., 8.7.0.0) to avoid compare mismatches if some environments still report 8.7.0.
Run to cross-check usage and version boundaries:
#!/bin/bash # Find all call sites toggled by this feature rg -nP --type=ts -C3 'appSupports(Set|Update)TagsEndpoint\(' # Sanity-check isAtLeastVersion boundary tests exist referencing "8.7.0.0" rg -nP --type=ts -C2 "8\.7\.0\.0" test || echo "No tests found around 8.7.0.0 boundary"
🧹 Nitpick comments (2)
src/utils/versioning/versions.ts (2)
11-12: Rename for terminology consistency: use UPDATE_TAGS_ENDPOINTThe rest of the PR and URL builder use “update tags”. Rename the feature key to avoid mixed terminology.
- SET_TAGS_ENDPOINT: '8.7.0.0', + UPDATE_TAGS_ENDPOINT: '8.7.0.0',Note: Update the corresponding guard function name and all call sites.
66-69: Align guard name with endpoint namingRename the guard to reflect “update tags” and the renamed feature key.
-export function appSupportsSetTagsEndpoint(currentNugetVersion: string | undefined) { - return isFeatureSupported({ feature: 'SET_TAGS_ENDPOINT', currentNugetVersion }); -} +export function appSupportsUpdateTagsEndpoint(currentNugetVersion: string | undefined) { + return isFeatureSupported({ feature: 'UPDATE_TAGS_ENDPOINT', currentNugetVersion }); +}Follow up: rename imports/usages (e.g., AttachmentsStorePlugin.tsx) accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/utils/versioning/versions.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/utils/versioning/versions.ts
🔇 Additional comments (1)
src/utils/versioning/versions.ts (1)
66-69: Add boundary tests for version compareAdd tests to lock behavior around 8.7.0.0 (just below, equal, and above).
Apply a new Jest test (example):
+// tests/utils/versioning/versions.update-tags.test.ts +import { appSupportsUpdateTagsEndpoint } from 'src/utils/versioning/versions'; + +describe('appSupportsUpdateTagsEndpoint', () => { + it('is false before 8.7.0.0', () => { + expect(appSupportsUpdateTagsEndpoint('8.6.999.999')).toBe(false); + }); + it('is true at 8.7.0.0', () => { + expect(appSupportsUpdateTagsEndpoint('8.7.0.0')).toBe(true); + }); + it('is true after 8.7.0.0', () => { + expect(appSupportsUpdateTagsEndpoint('8.7.0.1')).toBe(true); + }); + it('handles three-part inputs if emitted by runtime', () => { + expect(appSupportsUpdateTagsEndpoint('8.7.0')).toBe(true); + }); +});If three-part inputs aren’t supported by isAtLeastVersion, either normalize inputs or adjust the expectation and document it.
cammiida
force-pushed
the
feature/validate-tags
branch
from
1f7aee5 to
6fa68f9
Compare
cammiida
changed the base branch from
main
to
chore/update-data-model-validations
cammiida
force-pushed
the
feature/validate-tags
branch
from
6fa68f9 to
9e700ac
Compare
|
Description
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores