feat: Add errors, health and app metrics to the Admin application #17298
Conversation
📝 WalkthroughWalkthroughAdds backend alerts and metrics APIs, services, a SignalR hub, runtime-gateway client methods and DI registrations. Frontend adds metric UI, hooks, chart utilities, WebSocket sync wrapper and invalidator, shared useWebSocket migration, types, styles, tests and Chart.js dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Frontend as Frontend
participant API as Designer API
participant Service as Designer Service
participant Gateway as RuntimeGatewayClient
participant Hub as SignalR Hub
participant Runtime as Runtime Gateway
Frontend->>API: GET /designer/api/admin/metrics/{org}/{env}/app?range=...
API->>Service: GetAppMetricsAsync(org, env, app, range, ct)
Service->>Gateway: GetAppMetricsAsync(...)
Gateway->>Runtime: GET /runtime/gateway/api/v1/metrics/app?app=...&range=...
Runtime-->>Gateway: 200 AppMetric[]
Gateway-->>Service: AppMetric[]
Service-->>API: AppMetric[]
API-->>Frontend: 200 OK + AppMetric[]
Frontend->>API: POST /designer/api/admin/alerts/{org}/{env}
API->>Service: NotifyAlertsUpdatedAsync(org, env, ct)
Service->>Hub: Clients.Group(org).AlertsUpdated({ environment: env })
Hub-->>Frontend: Broadcast AlertsUpdated{ environment: env }
Note right of Frontend: AlertsUpdatedQueriesInvalidator.invalidateQueries(environment)
Frontend->>API: React Query refetches invalidated metric endpoints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
skip-releasenotes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (17)
src/Designer/frontend/packages/shared/src/hooks/useWebSocket/useWebSocket.ts (1)
18-21: Consider the implications ofonWSMessageReceivedin the dependency array.Including
onWSMessageReceivedin theuseEffectdependency array means the effect will re-run whenever the callback reference changes. If the callback isn't memoised withuseCallbackat the call site, this could cause the WebSocket connection to re-initialise on every render, leading to performance issues or connection instability.Consider either:
- Documenting that consumers must memorise the
onWSMessageReceivedcallback- Using a ref to store the callback and updating it without re-running the effect
- Removing it from dependencies if the callback is expected to be stable
🔎 Alternative approach using ref for callback
export const useWebSocket = <T>({ webSocketUrls, clientsName, webSocketConnector, onWSMessageReceived, }: UseWebsocket<T>): void => { const wsConnectionRef = useRef<WSConnector | null>(null); + const callbackRef = useRef(onWSMessageReceived); + + useEffect(() => { + callbackRef.current = onWSMessageReceived; + }, [onWSMessageReceived]); + useEffect(() => { wsConnectionRef.current = webSocketConnector.getInstance(webSocketUrls, clientsName); - wsConnectionRef.current?.onMessageReceived(onWSMessageReceived); - }, [webSocketConnector, webSocketUrls, clientsName, onWSMessageReceived]); + wsConnectionRef.current?.onMessageReceived((msg) => callbackRef.current(msg)); + }, [webSocketConnector, webSocketUrls, clientsName]); };src/Designer/backend/src/Designer/Hubs/AlertsUpdate/AlertsUpdatedHub.cs (1)
10-19: Consider addingOnDisconnectedAsyncto remove connections from groups.The existing
EntityUpdatedHub(see relevant snippets) implementsOnDisconnectedAsyncto remove connections from groups. Without this, group membership may accumulate stale connection IDs, though SignalR does handle cleanup on disconnect. If explicit cleanup is desired for consistency with the existing pattern, consider adding the override.🔎 Optional: Add OnDisconnectedAsync for explicit cleanup:
+ public override async Task OnDisconnectedAsync(Exception exception) + { + string connectionId = Context.ConnectionId; + List<Organization> organizations = await giteaService.GetUserOrganizations(); + foreach (var org in organizations) + { + await Groups.RemoveFromGroupAsync(connectionId, org.Username); + } + await base.OnDisconnectedAsync(exception); + }src/Designer/frontend/admin/features/appDetails/AppDetails.tsx (1)
27-27: Consider the breadcrumb link pattern.The breadcrumb creates a self-link to the current page with conditional range query parameter. Whilst this may be intentional to preserve the range state, breadcrumbs typically don't link to the current page. Consider whether this breadcrumb item should be non-interactive (not a link) since it represents the current location.
src/Designer/frontend/admin/features/appDetails/components/AppHealthMetric.tsx (1)
27-27: Consider extracting hard-coded colour values.The hard-coded colour values could be extracted to constants or CSS variables for better maintainability and consistency with the design system.
src/Designer/frontend/admin/shared/Alert/Alert.tsx (2)
9-15: Consider using a more specific type for the colour prop.The
colorprop is typed asstring, which allows any string value. IfStudioAlertexpects specific colour values (e.g.,'danger' | 'success' | 'info'), consider using that specific type instead for better type safety.
26-30: Consider a more generic translation key.The translation key
'admin.metrics.errors.link'is specific to errors, but this Alert component is used for all metric types (health, app, error). Consider using a more generic key like'admin.metrics.link'unless the link text intentionally differs by metric type.src/Designer/frontend/admin/utils/charts.ts (2)
4-34: Consider documenting the time unit threshold.Line 21 uses
1140as a threshold to switch between minute and hour units. Consider adding a comment or extracting this as a named constant to explain that this represents 19 hours, making the logic more self-documenting.🔎 Suggested improvement:
+const HOUR_UNIT_THRESHOLD_MINUTES = 1140; // 19 hours + export const getChartOptions = (range: number): ChartOptions<'line'> => ({ responsive: true, maintainAspectRatio: false, plugins: { legend: { display: false, }, }, scales: { x: { ticks: { font: { size: 10, }, }, type: 'time', time: { - unit: range >= 1140 ? 'hour' : 'minute', + unit: range >= HOUR_UNIT_THRESHOLD_MINUTES ? 'hour' : 'minute', }, }, y: { beginAtZero: true, ticks: { stepSize: 1, font: { size: 10, }, }, }, }, });
36-53: Consider removing optional chaining if dataPoints is guaranteed.Lines 41 and 45 use optional chaining (
?.) ondataPoints, but based on theAppMetricDataPoint[]type signature,dataPointsshould always be an array. If it can be undefined, update the type signature toAppMetricDataPoint[] | undefined. Otherwise, the optional chaining is unnecessary and could mask type issues.src/Designer/backend/src/Designer/Models/Metrics/AppMetricDataPoint.cs (1)
5-9: Consider using required keyword for properties.The properties
DateTimeOffsetandCountappear to be essential for metric data points. Consider marking them with therequiredkeyword to ensure they are always initialised, similar to how properties are marked inAppMetric.cs(as shown in relevant code snippets). This would prevent accidental creation of incomplete data points.🔎 Suggested improvement:
public class AppMetricDataPoint { - public DateTimeOffset DateTimeOffset { get; set; } - public double Count { get; set; } + public required DateTimeOffset DateTimeOffset { get; set; } + public required double Count { get; set; } }src/Designer/frontend/admin/utils/apiPaths.ts (1)
3-18: Consider URL-encoding query parameter values.The query parameters (
app,metric,range) are interpolated directly into the URL without encoding. If these values contain special characters (e.g., spaces,&,=), the URL could become malformed. Consider usingencodeURIComponentor the existinggetQueryStringFromObjecthelper (used ininstancesListPath) for consistency and safety.🔎 Example fix for one function:
-export const appMetricsPath = (org: string, env: string, app: string, range: number) => - `${adminApiBasePath}/metrics/${org}/${env}/app?app=${app}&range=${range}`; // Get +export const appMetricsPath = (org: string, env: string, app: string, range: number) => + `${adminApiBasePath}/metrics/${org}/${env}/app?app=${encodeURIComponent(app)}&range=${range}`; // Getsrc/Designer/frontend/admin/hooks/queries/useAppHealthMetricsQuery.ts (1)
8-20: LGTM!The hook follows established patterns with proper query key structure including all relevant parameters, and correctly passes the abort signal for request cancellation.
Consider adding an
enabledoption to guard against fetching with empty/invalid parameters if callers might pass them:enabled: Boolean(org && env && app),This is optional if calling components always provide valid values.
src/Designer/frontend/admin/features/appDetails/components/AppMetrics.tsx (1)
63-73: Remove unnecessary non-null assertions.The
rangeprop is typed asnumber(notnumber | undefined), so the non-null assertions (range!) on lines 63, 71, and 131 are unnecessary and could mask type issues if the prop type changes.🔎 Suggested fix:
} = useAppErrorMetricsQuery(org, env, app, range!, { + } = useAppErrorMetricsQuery(org, env, app, range, { hideDefaultError: true, }); const { data: appMetrics, isPending: appMetricsIsPending, isError: appMetricsIsError, - } = useAppMetricsQuery(org, env, app, range!, { + } = useAppMetricsQuery(org, env, app, range, { hideDefaultError: true, });And on line 131:
- value={range!} + value={range}src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.tsx (1)
6-6: Consider removing unused type imports.
SyncErrorandSyncSuccessare imported but the message handler only processesAlertsUpdatedmessages. If these types are only used for the union type in the callback, consider simplifying to justAlertsUpdatedor documenting why the broader union is needed.src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (1)
92-100: Consider avoiding non-null assertions by refining the type or using a fallback.The
range!assertions on lines 98, 114, and 173 work becausedefaultRangeensures a value. However, if the hook signature returnsT | undefined, consider usingrange ?? defaultRangeat usage sites for type safety without assertions.src/Designer/backend/src/Designer/Services/Implementation/MetricsService.cs (1)
11-13: Typo in constructor parameter name.The parameter
runetimeGatewayClientshould beruntimeGatewayClient(missing 'i').🔎 Apply this diff to fix the typo:
-internal sealed class MetricsService( - IRuntimeGatewayClient runetimeGatewayClient +internal sealed class MetricsService( + IRuntimeGatewayClient runtimeGatewayClient ) : IMetricsServiceThen update all references from
runetimeGatewayClienttoruntimeGatewayClientthroughout the class.src/Designer/backend/src/Designer/Controllers/MetricsController.cs (2)
20-31: Consider adding input validation forrangeparameter.The
rangeparameter is bound from the query string without validation. Negative or excessively large values could cause issues downstream. Consider adding a[Range]attribute or explicit validation.🔎 Example validation approach:
+using System.ComponentModel.DataAnnotations; + [HttpGet("errors")] public async Task<ActionResult<IEnumerable<ErrorMetric>>> GetErrorMetrics( string org, string env, - int range, + [Range(1, 10080)] int range, CancellationToken cancellationToken )Apply similar validation to other endpoints that accept
range.
61-74: Validate redirected URL from the metrics service.The
Redirect(url)uses a URL from the Runtime Gateway service without validation. While the base URL comes from trusted platform configuration, adding validation to ensure the returned URL targets an expected domain would strengthen defensive programming practices.Consider using
Uri.TryCreate()to validate the URL or checking that it belongs to an expected domain before redirecting.
src/Designer/backend/src/Designer/Services/Implementation/AlertsService.cs
Show resolved
Hide resolved
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
Show resolved
Hide resolved
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
Show resolved
Hide resolved
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.test.tsx
Show resolved
Hide resolved
src/Designer/frontend/app-development/components/WebSocketSyncWrapper/WebSocketSyncWrapper.tsx
Show resolved
Hide resolved
src/Designer/frontend/packages/shared/src/queryInvalidator/AlertsUpdatedQueriesInvalidator.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/Designer/frontend/admin/features/appDetails/components/AppErrorMetric.tsx (1)
33-33: Remove unnecessary non-null assertion.The non-null assertion operator (
range!) is unnecessary becauserangeis typed asnumberin the component props (line 12), so it cannot be null or undefined.🔎 Apply this diff
- url={appErrorMetricsLogsPath(org, env, app, metric.name, range!)} + url={appErrorMetricsLogsPath(org, env, app, metric.name, range)}src/Designer/frontend/admin/features/appDetails/components/AppHealthMetric.tsx (2)
22-32: Fix backgroundColor array mismatch.The
dataarray contains two segments[metric.count, 100 - metric.count], but thebackgroundColorarray provides only one colour. Chart.js requires thebackgroundColorarray length to match thedataarray length, so the second segment will render with an undefined or default colour, breaking the visual representation.🔎 Proposed fix to provide both segment colours
<Doughnut data={{ datasets: [ { data: [metric.count, 100 - metric.count], - backgroundColor: [isDown ? '#e8adad' : isPartiallyDown ? '#eeb04c' : '#8fc997'], + backgroundColor: [ + isDown ? '#e8adad' : isPartiallyDown ? '#eeb04c' : '#8fc997', + '#e0e0e0', + ], borderWidth: 0, }, ], }} />
11-15: Add bounds validation for metric.count.The component assumes
metric.countis within 0–100, but there's no validation to enforce this. Ifcountis negative or exceeds 100, the chart calculation100 - metric.countproduces invalid data segments, and the status flags (isDown,isPartiallyDown) won't correctly represent out-of-range values.🔎 Proposed fix to clamp metric.count to valid range
export const AppHealthMetric = ({ metric }: AppHealthMetricProps) => { const { t } = useTranslation(); - const isDown = metric.count === 0; - const isPartiallyDown = metric.count > 0 && metric.count < 100; + const count = Math.max(0, Math.min(100, metric.count)); + const isDown = count === 0; + const isPartiallyDown = count > 0 && count < 100;Then update references to use
countinstead ofmetric.countin lines 20, 26:- count={metric.count + '%'} + count={count + '%'} > <Doughnut data={{ datasets: [ { - data: [metric.count, 100 - metric.count], + data: [count, 100 - count], backgroundColor: [isDown ? '#e8adad' : isPartiallyDown ? '#eeb04c' : '#8fc997'],
🧹 Nitpick comments (5)
src/Designer/frontend/admin/features/appDetails/components/AppErrorMetric.tsx (1)
24-26: Consider extracting hard-coded colour values to constants.The border colours
'#590d0d'and'#023409'are hard-coded. Extracting these to named constants or a theme configuration would improve maintainability and make it easier to ensure colour consistency across the application.🔎 Example refactor
+const CHART_COLOURS = { + error: '#590d0d', + success: '#023409', +}; + export const AppErrorMetric = ({ metric, range, org, env, app }: AppErrorMetricProps) => { const { t } = useTranslation(); const options = getChartOptions(range); const count = metric.dataPoints.reduce((sum, item) => sum + item.count, 0); const isError = count > 0; const metricsChartData = getChartData(metric.dataPoints, { - borderColor: isError ? '#590d0d' : '#023409', + borderColor: isError ? CHART_COLOURS.error : CHART_COLOURS.success, });src/Designer/backend/src/Designer/Hubs/AlertsUpdate/AlertsUpdatedHub.cs (1)
13-22: Consider adding OnDisconnectedAsync for symmetry.While SignalR automatically removes connections from groups upon disconnection, the similar
EntityUpdatedHub(seesrc/Designer/backend/src/Designer/Hubs/EntityUpdate/EntityUpdatedHub.cs) implementsOnDisconnectedAsyncto explicitly remove from groups. Adding this override would provide symmetry and make the cleanup behaviour more explicit.🔎 Proposed implementation
public override async Task OnConnectedAsync() { string connectionId = Context.ConnectionId; List<Organization> organizations = await giteaService.GetUserOrganizations(); foreach (Organization org in organizations) { await Groups.AddToGroupAsync(connectionId, org.Username); } await base.OnConnectedAsync(); } + + public override async Task OnDisconnectedAsync(Exception exception) + { + string connectionId = Context.ConnectionId; + List<Organization> organizations = await giteaService.GetUserOrganizations(); + foreach (Organization org in organizations) + { + await Groups.RemoveFromGroupAsync(connectionId, org.Username); + } + await base.OnDisconnectedAsync(exception); + }src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (2)
92-100: Consider avoiding non-null assertions onrange.The
range!assertions at lines 98, 114, and 173 are safe at runtime becausedefaultRangeguarantees a value, but they could mask issues if the hook's return type changes. A fallback pattern would be more defensive.🔎 Suggested fallback pattern
- const { - data: errorMetrics, - isPending: errorMetricsIsPending, - isError: errorMetricsIsError, - } = useErrorMetricsQuery(org, env, range!, { + const effectiveRange = range ?? defaultRange; + const { + data: errorMetrics, + isPending: errorMetricsIsPending, + isError: errorMetricsIsError, + } = useErrorMetricsQuery(org, env, effectiveRange, { hideDefaultError: true, });Then use
effectiveRangethroughout the component instead ofrange!.
102-106: Toast may fire repeatedly on re-renders.If
errorMetricsIsErrorstaystrueacross re-renders (e.g., whenrangechanges but the previous error state hasn't cleared), thisuseEffectcould show duplicate toasts. Consider tracking whether the toast has already been shown for the current error state.🔎 Option: reset on range change
useEffect(() => { if (errorMetricsIsError) { toast.error(t('admin.metrics.errors.error')); } - }, [errorMetricsIsError, t]); + }, [errorMetricsIsError, t, range]);Alternatively, the query hook may automatically reset
isErroron a new fetch, in which case this is fine. Verify that changingrangetriggers a fresh query and clears the error state.src/Designer/frontend/admin/features/appDetails/components/AppHealthMetric.tsx (1)
27-27: Consider extracting hardcoded colour values.The hex colour codes
'#e8adad','#eeb04c', and'#8fc997'are hardcoded, which could complicate maintenance if the design system evolves. Consider using CSS custom properties or a centralised theme constant to ensure consistency across components.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Designer/backend/src/Designer/Hubs/AlertsUpdate/AlertsUpdatedHub.cs(1 hunks)src/Designer/backend/src/Designer/Services/Implementation/AlertsService.cs(1 hunks)src/Designer/backend/src/Designer/Services/Implementation/MetricsService.cs(1 hunks)src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs(2 hunks)src/Designer/frontend/admin/features/appDetails/AppDetails.tsx(2 hunks)src/Designer/frontend/admin/features/appDetails/components/AppErrorMetric.tsx(1 hunks)src/Designer/frontend/admin/features/appDetails/components/AppHealthMetric.tsx(1 hunks)src/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx(1 hunks)src/Designer/frontend/admin/features/apps/components/AppsTable.tsx(4 hunks)src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.test.tsx(1 hunks)src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx(1 hunks)src/Designer/frontend/packages/shared/src/queryInvalidator/AlertsUpdatedQueriesInvalidator.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Designer/frontend/admin/features/appDetails/AppDetails.tsx
- src/Designer/backend/src/Designer/Services/Implementation/AlertsService.cs
- src/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx
🧰 Additional context used
🧠 Learnings (24)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
Learnt from: framitdavid
Repo: Altinn/altinn-studio PR: 16372
File: src/Designer/frontend/libs/studio-components/src/components/index.ts:34-34
Timestamp: 2025-09-23T08:53:19.508Z
Learning: In the Altinn Studio codebase, when moving components from studio-components-legacy to studio-components, the team prefers to handle the component migration and remaining import updates in separate PRs to maintain focused, atomic changes.
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 16095
File: src/App/app-template-dotnet/src/App/models/model.cs:10-29
Timestamp: 2025-08-15T05:53:32.529Z
Learning: When moving app templates in Altinn Studio, mirkoSekulic prefers to keep the PR scope strictly limited to the move operation itself, with all content changes (including code quality improvements, naming convention fixes, and attribute cleanup) deferred to separate PRs for better maintainability and atomic changes.
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Applies to src/App/frontend/**/*.test.{ts,tsx} : Use `renderWithProviders` utility from `src/test/renderWithProviders.tsx` when testing components that require form layout context
Applied to files:
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.test.tsx
📚 Learning: 2025-07-15T06:14:47.860Z
Learnt from: wrt95
Repo: Altinn/altinn-studio PR: 15850
File: frontend/app-development/features/appSettings/components/TabsContent/Tabs/AboutTab/AppConfigForm/StatusRadioGroup/StatusRadioGroup.test.tsx:47-56
Timestamp: 2025-07-15T06:14:47.860Z
Learning: In StatusRadioGroup tests for Altinn Studio, the onChangeStatus callback is called with two parameters: the selected value and the previous value. This is why test assertions like `expect(onChangeStatus).toHaveBeenCalledWith('Completed', '');` use both parameters, even though the TypeScript interface only shows one parameter.
Applied to files:
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.test.tsx
📚 Learning: 2025-08-08T13:24:30.117Z
Learnt from: nkylstad
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-08-08T13:24:30.117Z
Learning: In Altinn/altinn-studio PR reviews, when unit tests need React Query/TanStack Query data, prefer seeding with QueryClient.setQueryData over rendering/mocking use…Query hooks and waiting for isSuccess. Flag patterns like renderHookWithMockStore(() => use…Query(...)) and waitFor(() => expect(result.current.isSuccess).toBe(true)) and recommend the simpler setQueryData approach. Cite the internal Confluence guideline on component tests and the best-practice example in useAddItemToLayoutMutation.test.ts (lines updated 08.08.2025).
Applied to files:
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.test.tsx
📚 Learning: 2025-10-28T20:54:18.494Z
Learnt from: JamalAlabdullah
Repo: Altinn/altinn-studio PR: 16787
File: src/Designer/frontend/packages/ux-editor-v3/src/components/toolbar/RuleModal.test.tsx:70-70
Timestamp: 2025-10-28T20:54:18.494Z
Learning: Test inconsistencies in the ux-editor-v3 package do not need to be fixed, as indicated by the maintainer for the file `src/Designer/frontend/packages/ux-editor-v3/src/components/toolbar/RuleModal.test.tsx`.
Applied to files:
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.test.tsx
📚 Learning: 2025-09-26T12:03:51.850Z
Learnt from: TomasEng
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-09-26T12:03:51.850Z
Learning: In PR #16443, TomasEng clarified that the ExpressionTexts type in studio/components does not require additional keys like dataModel, resource, or missingDataModelLabel that were suggested in a review comment. TypeScript compilation would catch any missing required properties, confirming the migration is correctly implemented.
Applied to files:
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx
📚 Learning: 2025-07-03T14:54:51.389Z
Learnt from: mgunnerud
Repo: Altinn/altinn-studio PR: 15826
File: frontend/resourceadm/components/ResourcePageInputs/ResourceLanguageTextField.tsx:95-95
Timestamp: 2025-07-03T14:54:51.389Z
Learning: The StudioTextfield component at frontend/libs/studio-components/src/components/StudioTextfield/StudioTextfield.tsx has a typing limitation where it only accepts `Ref<HTMLInputElement>` even when rendering textarea elements (when multiline=true). This creates a type mismatch between the ref type and the actual element type, requiring workarounds like removing ref props when both input and textarea elements need to be supported.
Applied to files:
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx
📚 Learning: 2025-11-03T08:16:33.782Z
Learnt from: JamalAlabdullah
Repo: Altinn/altinn-studio PR: 16865
File: src/Designer/frontend/app-preview/src/components/PreviewControlHeader/PreviewControlHeader.tsx:44-50
Timestamp: 2025-11-03T08:16:33.782Z
Learning: In PreviewControlHeader.tsx (app-preview package), JamalAlabdullah confirmed that the StudioSelect component for layout set selection is acceptable with an empty label prop (label={''}), indicating this is an intentional design decision for that specific component.
Applied to files:
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx
📚 Learning: 2025-09-26T12:03:56.908Z
Learnt from: TomasEng
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-09-26T12:03:56.908Z
Learning: In the Altinn Studio codebase, when reviewing migration from studio/components-legacy to studio/components for the StudioExpression component, the ExpressionTexts type interface does not require additional keys like `dataModel`, `resource`, or `missingDataModelLabel`. TypeScript compilation will catch any missing required properties, so successful builds indicate proper type compliance.
Applied to files:
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx
📚 Learning: 2025-07-03T14:54:51.389Z
Learnt from: mgunnerud
Repo: Altinn/altinn-studio PR: 15826
File: frontend/resourceadm/components/ResourcePageInputs/ResourceLanguageTextField.tsx:95-95
Timestamp: 2025-07-03T14:54:51.389Z
Learning: The StudioTextfield component at frontend/libs/studio-components/src/components/StudioTextfield/StudioTextfield.tsx accepts only HTMLInputElement as ref type, even when the component renders an HTMLTextAreaElement (when multiline is true). This is a typing constraint of the component library.
Applied to files:
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx
📚 Learning: 2025-06-16T12:19:07.682Z
Learnt from: lassopicasso
Repo: Altinn/altinn-studio PR: 15495
File: frontend/app-development/layout/VersionDialog/VersionDialogTableRow.tsx:5-13
Timestamp: 2025-06-16T12:19:07.682Z
Learning: In the Altinn Studio codebase, the VersionDialog components work with major version numbers only (e.g., 4, 8) rather than full semantic versions. The latestVersion prop in VersionDialogTableRow should be typed as number since it represents maximum supported major version constants like MAXIMUM_SUPPORTED_FRONTEND_VERSION = 4.
Applied to files:
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx
📚 Learning: 2025-12-11T09:10:06.960Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 17174
File: src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs:1-8
Timestamp: 2025-12-11T09:10:06.960Z
Learning: In C# 12, you can declare a class, interface, or struct with an empty body using a trailing semicolon as a shorthand (e.g., internal sealed class X : Y;). When reviewing StudioGateway.Api code under src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs and other C# files, verify the language version is set to at least 12.0+ and that any empty-body types use the single semicolon shorthand only when semantically appropriate (i.e., the type has no members). If a type previously had members, continue using a normal body.
Applied to files:
src/Designer/backend/src/Designer/Hubs/AlertsUpdate/AlertsUpdatedHub.cssrc/Designer/backend/src/Designer/Services/Implementation/MetricsService.cssrc/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Applies to src/App/frontend/**/*.{ts,tsx} : Use objects for managing TanStack Query keys and functions; use `queryOptions` for sharing query configuration centrally
Applied to files:
src/Designer/frontend/packages/shared/src/queryInvalidator/AlertsUpdatedQueriesInvalidator.ts
📚 Learning: 2025-12-18T12:57:31.132Z
Learnt from: TomasEng
Repo: Altinn/altinn-studio PR: 17285
File: src/Designer/frontend/libs/studio-content-library/src/pages/Page.tsx:25-29
Timestamp: 2025-12-18T12:57:31.132Z
Learning: In src/Designer/frontend/libs/studio-content-library/src/pages/Page.tsx, the extractProps method requires a type assertion `as PagePropsMap<Name>` on the return because TypeScript cannot narrow the indexed access `config.pages[this.name].props` to the specific generic type PagePropsMap<Name> even after Guard.againstMissingProperty. Removing this cast results in TS2322 error with a union type instead of the narrowed type. This is a known TypeScript limitation with generics and indexed access types.
Applied to files:
src/Designer/frontend/admin/features/appDetails/components/AppErrorMetric.tsx
📚 Learning: 2025-01-28T22:18:03.328Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:43-48
Timestamp: 2025-01-28T22:18:03.328Z
Learning: Error handling improvements for the Get method in AzureDevOpsBuildClient were deemed out of scope for PR #14456, which focused on undeploy endpoints functionality.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-07-08T09:19:49.084Z
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-01-28T22:17:05.810Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/Services/Implementation/ReleaseService.cs:66-69
Timestamp: 2025-01-28T22:17:05.810Z
Learning: The GetDeveloperAppTokenAsync() method in Altinn.Studio.Designer is called within protected endpoints where authentication is already enforced, making additional null checks unnecessary.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-01-28T22:20:43.402Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.402Z
Learning: In the Altinn Studio project, HTTP status codes are handled globally through a delegating handler, eliminating the need for explicit EnsureSuccessStatusCode() calls in individual HTTP clients.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-02-11T13:27:27.159Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14627
File: backend/src/Designer/TypedHttpClients/AltinnStorage/AltinnStorageAppMetadataClient.cs:0-0
Timestamp: 2025-02-11T13:27:27.159Z
Learning: In Altinn.Studio.Designer's HTTP clients, success status codes are handled by delegating handlers in the request pipeline, making explicit status code checks in individual HTTP client methods unnecessary.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-02-11T13:27:27.159Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14627
File: backend/src/Designer/TypedHttpClients/AltinnStorage/AltinnStorageAppMetadataClient.cs:0-0
Timestamp: 2025-02-11T13:27:27.159Z
Learning: HTTP status codes are handled by delegating handlers in the Altinn.Studio.Designer codebase, making explicit status code checks in individual HTTP clients unnecessary.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-02-11T13:27:27.159Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14627
File: backend/src/Designer/TypedHttpClients/AltinnStorage/AltinnStorageAppMetadataClient.cs:0-0
Timestamp: 2025-02-11T13:27:27.159Z
Learning: In Altinn.Studio.Designer's HTTP clients, the `EnsureSuccessHandler` delegating handler is responsible for ensuring successful HTTP status codes, making explicit status code checks in individual HTTP client methods unnecessary.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-01-28T22:20:43.402Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.402Z
Learning: The QueueBuildRequest class in Altinn Studio uses System.Text.Json serialization annotations directly on the class properties, making explicit serializer options unnecessary.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-01-28T22:20:43.401Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.401Z
Learning: In Altinn Studio, HTTP status codes are handled globally through the EnsureSuccessHandler delegating handler, which throws HttpRequestWithStatusException for non-success status codes and provides detailed error logging.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-01-28T22:20:43.402Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.402Z
Learning: The QueueBuildRequest class in Altinn Studio uses JsonPropertyName attributes from System.Text.Json for serialization (e.g., [JsonPropertyName("definition")] for the DefinitionReference property).
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
🧬 Code graph analysis (7)
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.test.tsx (1)
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx (2)
TimeRangeSelectProps(5-9)TimeRangeSelect(11-31)
src/Designer/frontend/admin/features/appDetails/components/AppHealthMetric.tsx (2)
src/Designer/frontend/admin/types/metrics/AppHealthMetric.ts (1)
AppHealthMetric(1-4)src/Designer/frontend/admin/shared/Alert/Alert.tsx (1)
Alert(17-35)
src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx (1)
src/Designer/frontend/libs/studio-components/src/components/StudioSelect/StudioSelect.tsx (1)
StudioSelect(18-40)
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (4)
src/Designer/frontend/admin/hooks/queries/useErrorMetricsQuery.ts (1)
useErrorMetricsQuery(8-20)src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx (1)
TimeRangeSelect(11-31)src/Designer/frontend/admin/shared/Alert/Alert.tsx (1)
Alert(17-35)src/Designer/frontend/admin/utils/apiPaths.ts (1)
appErrorMetricsLogsPath(9-16)
src/Designer/backend/src/Designer/Hubs/AlertsUpdate/AlertsUpdatedHub.cs (3)
src/Designer/backend/src/Designer/Hubs/EntityUpdate/EntityUpdatedHub.cs (3)
Authorize(11-36)Task(21-27)Task(29-35)src/Designer/backend/src/Designer/Services/Implementation/AlertsService.cs (2)
Task(19-26)Task(29-36)src/Designer/backend/src/Designer/Hubs/AlertsUpdate/IAlertsUpdateClient.cs (1)
Task(7-7)
src/Designer/frontend/admin/features/appDetails/components/AppErrorMetric.tsx (3)
src/Designer/frontend/admin/utils/charts.ts (2)
getChartOptions(4-34)getChartData(36-53)src/Designer/frontend/admin/shared/Alert/Alert.tsx (1)
Alert(17-35)src/Designer/frontend/admin/utils/apiPaths.ts (1)
appErrorMetricsLogsPath(9-16)
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs (6)
src/Designer/backend/src/Designer/Services/Implementation/MetricsService.cs (5)
Task(16-24)Task(27-36)Task(39-48)Task(51-61)Task(64-72)src/Designer/backend/src/Designer/Services/Interfaces/IMetricsService.cs (4)
Task(11-16)Task(18-24)Task(26-32)Task(34-41)src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/IRuntimeGatewayClient.cs (7)
Task(12-12)Task(13-13)Task(14-14)Task(15-15)Task(16-16)Task(17-17)Task(18-18)src/Designer/backend/src/Designer/Models/Alerts/AlertRule.cs (1)
AlertRule(6-37)src/Designer/backend/src/Designer/Models/Metrics/ErrorMetric.cs (1)
ErrorMetric(3-8)src/Designer/backend/src/Designer/Models/Metrics/AppErrorMetric.cs (1)
AppErrorMetric(5-9)
⏰ 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). (9)
- GitHub Check: Analyze
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run integration tests against actual gitea and dbs
- GitHub Check: Typechecking and linting
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: CodeQL
- GitHub Check: Testing
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (20)
src/Designer/frontend/packages/shared/src/queryInvalidator/AlertsUpdatedQueriesInvalidator.ts (4)
1-3: LGTM!The imports are appropriate and minimal for the class implementation.
5-14: LGTM!The class structure and constructor implementation are clean. The default 500ms timeout is sensible for batching query invalidations.
16-34: Previous concern addressed; minor note on task loss.The stale
orgissue from the previous review has been properly resolved by checking if the organisation has changed and recreating the instance accordingly.One minor consideration: when the organisation changes, any tasks pending in the old instance's queue are discarded. In practice, this means some metric invalidations might not execute if they were queued just before the organisation switch. Given the real-time WebSocket flow will likely trigger new invalidations for the new organisation, the impact should be minimal.
36-51: LGTM!The query invalidation logic is clean and correctly batches invalidations via the queue. The use of specific query keys ensures only relevant metrics queries are invalidated when alert updates arrive.
src/Designer/backend/src/Designer/Hubs/AlertsUpdate/AlertsUpdatedHub.cs (2)
10-11: LGTM!The class correctly uses the
[Authorize]attribute for security and follows the SignalR typed hub pattern withHub<IAlertsUpdateClient>. The primary constructor syntax is clean and appropriate for dependency injection.
13-22: Async pattern correctly implemented.The previous fire-and-forget issue has been properly resolved by using a
foreachloop that awaits eachGroups.AddToGroupAsynccall. This ensures all group additions complete before the connection handshake finishes.src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.test.tsx (2)
12-15: Good fix: Mock reset now in place.The
beforeEachwithjest.clearAllMocks()properly isolates tests, addressing the previous concern about shared mock state.
23-34: Test implementation looks correct.The async test properly uses
userEvent.setup()andwaitForfor the assertion. The test verifies that selecting '60' from the combobox triggersonChangewith the numeric value60, which aligns with the component's behaviour of converting string option values to numbers.src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx (1)
11-31: Clean controlled component implementation.The type handling is correct:
String(value)for the select andNumber(e.target.value)on change. The predefined options provide sensible time ranges for metrics queries.src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (3)
143-148: Good fix: Operator precedence issue resolved.The parentheses around
(appErrorMetrics?.length ?? 0) > 0correctly ensure the nullish coalescing is evaluated before the comparison, producing a proper boolean forhasMetrics.
156-160: URL construction looks correct.The conditional query string
${range && range !== defaultRange ? '?range=' + range : ''}properly omits the range param when it equals the default, keeping URLs clean.
165-177: Alert rendering is well-structured.Each error metric is mapped to an
Alertcomponent with appropriate props. Theurlcorrectly usesappErrorMetricsLogsPathto link to Application Insights traces as described in the PR objectives.src/Designer/backend/src/Designer/Services/Implementation/MetricsService.cs (1)
11-73: LGTM! Clean delegation pattern.The MetricsService follows a clear facade pattern, delegating all operations to the injected
IRuntimeGatewayClient. This separation provides a good architectural boundary between the service layer and HTTP client concerns, allowing for future enhancements such as caching, logging, or validation without affecting consumers.src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs (7)
1-10: LGTM! Proper imports for new functionality.The addition of
Systemnamespace enablesUri.EscapeDataStringusage, and the new model namespaces (Alerts,Metrics) support the new methods.
41-52: LGTM! Clean implementation.The method follows the established pattern and correctly handles null responses with the
?? []fallback.
55-67: LGTM! Correct query parameter handling.The
rangeparameter is an integer and doesn't require URL encoding. The implementation is clean and consistent.
70-83: LGTM! URL encoding issue resolved.The
appparameter is now properly encoded usingUri.EscapeDataStringon line 80, addressing the previous review concern about malformed URLs with special characters.
86-99: LGTM! URL encoding issue resolved.The
appparameter is now properly encoded on line 96, resolving the previous concern.
102-117: LGTM! Critical issues resolved.Both previous concerns have been addressed:
- URL encoding: Both
appandmetricparameters are properly encoded on line 113.- Safe dictionary access: Line 116 now uses
TryGetValueinstead of direct indexing, preventingKeyNotFoundExceptionand gracefully returning an empty string when the "url" key is missing.
120-132: LGTM! URL encoding issue resolved.The
appparameter is now properly encoded on line 129, addressing the previous review concern.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx (2)
22-33: Consider resetting the singleton instance between tests.
AlertsUpdatedQueriesInvalidatoruses a singleton pattern, and the first test creates an instance with specific parameters (org, timeout=0). This instance persists across tests, potentially causing test pollution if tests run in different orders or with different parameters.Recommendation: Reset the singleton instance in
afterEachto ensure test isolation.🔎 Proposed fix
afterEach(() => { jest.clearAllMocks(); + // Reset singleton to prevent test pollution + (AlertsUpdatedQueriesInvalidator as any).instance = null; });
63-73: Consider consolidating or enhancing this test.This test only verifies that
useWebSocketis called with the expected parameters, which provides minimal value as a regression check. The first test already exercises theonWSMessageReceivedcallback behaviour indirectly.You might consider either:
- Consolidating both assertions into the first test, or
- Enhancing this test to verify additional edge cases (e.g., messages without an
environmentproperty).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Register services in DI container properly following existing patterns for options configuration
📚 Learning: 2025-08-08T13:24:30.117Z
Learnt from: nkylstad
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-08-08T13:24:30.117Z
Learning: In Altinn/altinn-studio PR reviews, when unit tests need React Query/TanStack Query data, prefer seeding with QueryClient.setQueryData over rendering/mocking use…Query hooks and waiting for isSuccess. Flag patterns like renderHookWithMockStore(() => use…Query(...)) and waitFor(() => expect(result.current.isSuccess).toBe(true)) and recommend the simpler setQueryData approach. Cite the internal Confluence guideline on component tests and the best-practice example in useAddItemToLayoutMutation.test.ts (lines updated 08.08.2025).
Applied to files:
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Applies to src/App/frontend/**/*.test.{ts,tsx} : Use `renderWithProviders` utility from `src/test/renderWithProviders.tsx` when testing components that require form layout context
Applied to files:
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx
🧬 Code graph analysis (1)
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx (5)
src/Designer/frontend/packages/shared/src/types/api/AlertsUpdated.ts (1)
AlertsUpdated(1-3)src/Designer/frontend/packages/shared/src/queryInvalidator/AlertsUpdatedQueriesInvalidator.ts (1)
AlertsUpdatedQueriesInvalidator(5-52)src/Designer/frontend/packages/shared/src/hooks/useWebSocket/useWebSocket.ts (1)
useWebSocket(11-22)src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.tsx (1)
WebSocketSyncWrapper(18-38)src/Designer/frontend/app-development/components/WebSocketSyncWrapper/WebSocketSyncWrapper.tsx (1)
WebSocketSyncWrapper(28-75)
⏰ 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). (9)
- GitHub Check: CodeQL
- GitHub Check: Testing
- GitHub Check: Typechecking and linting
- GitHub Check: Build environment and run e2e test
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run integration tests against actual gitea and dbs
- GitHub Check: Analyze
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #17298 +/- ##
==========================================
- Coverage 96.26% 96.09% -0.17%
==========================================
Files 2384 2417 +33
Lines 30282 30766 +484
Branches 3529 3597 +68
==========================================
+ Hits 29150 29566 +416
- Misses 820 884 +64
- Partials 312 316 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx (1)
35-61: Previous queryClient mismatch has been resolved.The earlier concern about the test creating a separate
QueryClientinstance in the render helper has been addressed. The helper now acceptsqueryClientas a parameter (line 78), and this test correctly passesqueryClientMockon line 52, ensuring the component and assertions operate on the same instance.The test logic is correct and validates the environment-scoped query invalidation behaviour as expected.
src/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx (1)
83-83: Replacevarwithconstfor variable declarations.Lines 83, 95, and 107 use
varto declaremockData. Modern TypeScript prefersconstorletfor block-scoped variables. UseconstsincemockDatais not reassigned.🔎 Apply these changes
- var mockData = [{ name: 'ready_pods', count: 100.0 }]; + const mockData = [{ name: 'ready_pods', count: 100.0 }];- var mockData = [{ name: 'ready_pods', count: 50.0 }]; + const mockData = [{ name: 'ready_pods', count: 50.0 }];- var mockData = [{ name: 'ready_pods', count: 0.0 }]; + const mockData = [{ name: 'ready_pods', count: 0.0 }];Also applies to: 95-95, 107-107
🧹 Nitpick comments (4)
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx (1)
64-64: Remove unnecessary mock return value.
useWebSocketreturnsvoid, so mocking a return value serves no purpose and adds confusion.🔎 Proposed fix
- (useWebSocket as jest.Mock).mockReturnValue({ onWSMessageReceived: jest.fn() }); + (useWebSocket as jest.Mock).mockImplementation(() => {});Or simply:
- (useWebSocket as jest.Mock).mockReturnValue({ onWSMessageReceived: jest.fn() });Since
useWebSocketis already mocked at the module level, you can rely on the default mock behaviour without explicitly setting a return value.src/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx (1)
31-31: Verify if the mock for useQueryParamState is necessary.The mock for
admin/hooks/useQueryParamStatemay be unused. Based on the component code inAppMetrics.tsx, this hook does not appear to be imported or used directly by theAppMetricscomponent.Run the following script to verify if this hook is used in the component:
#!/bin/bash # Description: Check if useQueryParamState is imported or used in AppMetrics.tsx # Search for useQueryParamState usage in the component file rg -n 'useQueryParamState' src/Designer/frontend/admin/features/appDetails/components/AppMetrics.tsxsrc/Designer/frontend/admin/shared/Alert/Alert.tsx (2)
9-15: Consider refining the type definitions forcolorandchildren.
The
colorprop is typed asstring, which accepts any value but is used as a data attribute (line 21). If only specific colour values are valid, consider using a union type (e.g.,'success' | 'warning' | 'danger') for better type safety.The
childrenprop is typed asReactElement, which restricts it to a single React element. If you want to support multiple children, fragments, or other React nodes, consider usingReactNodeinstead.🔎 Proposed type refinements
type AlertProps = { - color: string; + color: 'success' | 'warning' | 'danger' | 'info'; // Adjust to match valid values count: string; title: string; url?: string; - children?: ReactElement; + children?: ReactNode; } & StudioAlertProps;
31-35: Hardcoded translation key limits component reusability.The translation key
'admin.metrics.errors.link'is hardcoded on line 33, which ties the link text specifically to error metrics. Since this component resides in the shared directory (admin/shared/Alert/), it may be used in other contexts (e.g., health metrics as mentioned in the AI summary) where a different link label would be more appropriate.🔎 Proposed fix to make the link text configurable
Update the
AlertPropstype to accept an optionallinkTextprop:type AlertProps = { color: string; count: string; title: string; url?: string; + linkText?: string; children?: ReactElement; } & StudioAlertProps;Then update the component implementation:
-export const Alert = ({ color, count, title, url, children, className, ...rest }: AlertProps) => { +export const Alert = ({ color, count, title, url, linkText, children, className, ...rest }: AlertProps) => { const { t } = useTranslation(); return ( <StudioAlert data-color={color} role='alert' className={cn(classes.metric, className)} {...rest} > <div className={classes.heading}> <span className={classes.title}> <span className={classes.count}>{count}</span> {title} </span> {url && ( <StudioLink href={url} rel='noopener noreferrer' target='_blank' className={classes.link}> - {t('admin.metrics.errors.link')} + {linkText || t('admin.metrics.errors.link')} </StudioLink> )} </div> {children && <div className={classes.chart}>{children}</div>} </StudioAlert> ); };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx(1 hunks)src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx(1 hunks)src/Designer/frontend/admin/shared/Alert/Alert.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
📚 Learning: 2025-08-08T13:24:30.117Z
Learnt from: nkylstad
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-08-08T13:24:30.117Z
Learning: In Altinn/altinn-studio PR reviews, when unit tests need React Query/TanStack Query data, prefer seeding with QueryClient.setQueryData over rendering/mocking use…Query hooks and waiting for isSuccess. Flag patterns like renderHookWithMockStore(() => use…Query(...)) and waitFor(() => expect(result.current.isSuccess).toBe(true)) and recommend the simpler setQueryData approach. Cite the internal Confluence guideline on component tests and the best-practice example in useAddItemToLayoutMutation.test.ts (lines updated 08.08.2025).
Applied to files:
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsxsrc/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Applies to src/App/frontend/**/*.test.{ts,tsx} : Use `renderWithProviders` utility from `src/test/renderWithProviders.tsx` when testing components that require form layout context
Applied to files:
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsxsrc/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Use React Query for server state management instead of custom context wrappers
Applied to files:
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/test/**/*.cs : Use snapshot testing for OpenAPI docs and telemetry output
Applied to files:
src/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/test/**/*.cs : Use comprehensive unit tests with high coverage
Applied to files:
src/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Applies to src/App/frontend/src/__mocks__/** : Mock external dependencies in `src/__mocks__/` directory following the existing mock patterns
Applied to files:
src/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx
🧬 Code graph analysis (2)
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx (4)
src/Designer/frontend/packages/shared/src/types/api/AlertsUpdated.ts (1)
AlertsUpdated(1-3)src/Designer/frontend/packages/shared/src/queryInvalidator/AlertsUpdatedQueriesInvalidator.ts (1)
AlertsUpdatedQueriesInvalidator(5-52)src/Designer/frontend/packages/shared/src/hooks/useWebSocket/useWebSocket.ts (1)
useWebSocket(11-22)src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.tsx (1)
WebSocketSyncWrapper(18-38)
src/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx (1)
src/Designer/frontend/admin/features/appDetails/components/AppMetrics.tsx (2)
AppMetricsProps(42-45)AppMetrics(47-142)
⏰ 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). (9)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: CodeQL
- GitHub Check: Typechecking and linting
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Analyze
- GitHub Check: Run integration tests against actual gitea and dbs
🔇 Additional comments (5)
src/Designer/frontend/admin/layout/WebSocketSyncWrapper/WebSocketSyncWrapper.test.tsx (2)
67-73: LGTM!The assertions correctly verify that
useWebSocketis invoked with the expected configuration: the correct WebSocket URLs, client name, connector, and callback function.
78-84: LGTM!The render helper correctly accepts a
queryClientparameter with a sensible default, allowing tests to either provide their own mock instance or use the default. This resolves the previous concern about queryClient mismatch.src/Designer/frontend/admin/features/appDetails/components/AppMetrics.test.tsx (1)
271-282: Well-structured test helper.The
renderAppMetricshelper properly wraps the component with necessary providers (MemoryRouterandQueryClientProvider) and follows testing best practices. The use ofqueryClient.setQueryDatato seed test data aligns with the recommended approach.Based on learnings, this follows the preferred pattern of using
setQueryDataover mocking hooks and waiting forisSuccess.src/Designer/frontend/admin/shared/Alert/Alert.tsx (2)
17-40: Component structure looks good overall.The Alert component implementation is well-structured:
- Proper composition with
StudioAlertand className merging- Conditional rendering for optional
urlandchildrenprops- Good security practice with
rel='noopener noreferrer'for external linksThe main concerns have been noted in previous comments regarding the
roleattribute, hardcoded translation key, and type definitions.
20-25: Verify thatrole='alert'is not redundant.
StudioAlertis a wrapper component that passes props through toAlertfrom@digdir/designsystemet-react. Whether the explicitrole='alert'on line 22 is redundant depends on whether the underlying designsystemet Alert component already applies this ARIA role. Standard Alert component patterns typically includerole="alert"by default. If designsystemet-react's Alert already sets this role, remove the explicit assignment from line 22 to avoid duplication.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Designer/frontend/admin/features/instanceDetails/InstanceDetails.tsx (2)
20-20: Consider localizing hardcoded text.The breadcrumb label "Publiserte apper" is hardcoded in Norwegian. If the application supports multiple languages, this string should be externalized using the localisation system.
28-32: Consider removing the link for the current page.The last breadcrumb item (current page) uses a
Linkcomponent with an empty target. For better accessibility and semantics, consider rendering the current page without a link wrapper, or use aspanelement instead.🔎 Suggested alternative
<StudioBreadcrumbs.Item> - <StudioBreadcrumbs.Link asChild> - <Link to=''>{instanceId}</Link> - </StudioBreadcrumbs.Link> + <StudioBreadcrumbs.Link>{instanceId}</StudioBreadcrumbs.Link> </StudioBreadcrumbs.Item>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Designer/frontend/admin/features/instanceDetails/InstanceDetails.tsx(1 hunks)src/Designer/frontend/admin/features/instances/components/InstancesTable.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
🧬 Code graph analysis (1)
src/Designer/frontend/admin/features/instances/components/InstancesTable.tsx (1)
src/Designer/development/azure-devops-mock/src/routes/storage.js (1)
instance(174-174)
⏰ 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). (9)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Typechecking and linting
- GitHub Check: Run integration tests against actual gitea and dbs
- GitHub Check: CodeQL
- GitHub Check: Testing
- GitHub Check: Analyze
🔇 Additional comments (1)
src/Designer/frontend/admin/features/instances/components/InstancesTable.tsx (1)
112-112: Navigation path correctly resolves to the instance detail route.The relative path
instances/${instance.id}properly resolves to theapps/:env/:app/instances/:instanceIdroute, which is defined in the router configuration and mapped to the InstanceDetails component. The change integrates correctly with the new structure where Instances is now rendered within AppDetails rather than as a separate route page.
src/Designer/frontend/admin/features/instanceDetails/InstanceDetails.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (1)
180-180: Same non-null assertion issue.As with line 100, replace
range!withrange ?? defaultRangefor type safety.
🧹 Nitpick comments (2)
src/Designer/frontend/admin/features/apps/components/AppsTable.module.css (1)
15-17: Fixed height may cause layout issues.The hardcoded
height: 39pxcould cause content clipping or overflow if error metrics wrap or grow beyond this constraint. Consider usingmin-heightinstead, or remove the constraint to allow natural content flow.🔎 Proposed fix
.errorMetricsCell { - height: 39px; + min-height: 39px; }src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (1)
100-100: Replace non-null assertion with nullish coalescing.Since
useQueryParamStatecan returnundefined, the non-null assertionrange!bypasses TypeScript's safety. Userange ?? defaultRangeinstead to handle the case explicitly.🔎 Proposed fix
} = useErrorMetricsQuery(org, env, range!, { + } = useErrorMetricsQuery(org, env, range ?? defaultRange, { hideDefaultError: true, });Apply the same pattern on line 180.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Designer/frontend/admin/features/apps/components/AppsTable.module.csssrc/Designer/frontend/admin/features/apps/components/AppsTable.test.tsxsrc/Designer/frontend/admin/features/apps/components/AppsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Designer/frontend/admin/features/apps/components/AppsTable.test.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Register services in DI container properly following existing patterns for options configuration
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Applies to src/App/frontend/**/*.module.css : Use CSS Modules for component styling with `.module.css` file extension
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.module.css
📚 Learning: 2025-10-26T21:09:38.402Z
Learnt from: JamalAlabdullah
Repo: Altinn/altinn-studio PR: 16742
File: src/Designer/frontend/packages/schema-editor/src/components/SchemaInspector/ItemDataComponent.module.css:23-25
Timestamp: 2025-10-26T21:09:38.402Z
Learning: In the Altinn Studio codebase, the design system is migrating from `--fds-*` prefixed CSS variables to `--ds-*` prefixed variables. The `--fds-*` prefix will be removed in the future, so usage of `--ds-*` variables (like `--ds-size-4`) alongside existing `--fds-*` variables (like `--fds-spacing-4`) in the same file is intentional during this transition period.
<!--
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.module.css
📚 Learning: 2025-09-26T12:03:56.908Z
Learnt from: TomasEng
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-09-26T12:03:56.908Z
Learning: In the Altinn Studio codebase, when reviewing migration from studio/components-legacy to studio/components for the StudioExpression component, the ExpressionTexts type interface does not require additional keys like `dataModel`, `resource`, or `missingDataModelLabel`. TypeScript compilation will catch any missing required properties, so successful builds indicate proper type compliance.
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx
🧬 Code graph analysis (1)
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (5)
src/Designer/frontend/admin/hooks/useQueryParamState.ts (1)
useQueryParamState(4-44)src/Designer/frontend/admin/hooks/queries/useErrorMetricsQuery.ts (1)
useErrorMetricsQuery(8-20)src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx (1)
TimeRangeSelect(11-31)src/Designer/frontend/admin/shared/Alert/Alert.tsx (1)
Alert(17-40)src/Designer/frontend/admin/utils/apiPaths.ts (1)
appErrorMetricsLogsPath(9-16)
⏰ 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). (9)
- GitHub Check: Testing
- GitHub Check: CodeQL
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Analyze
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run integration tests against actual gitea and dbs
- GitHub Check: Build environment and run e2e test
- GitHub Check: Format check
🔇 Additional comments (4)
src/Designer/frontend/admin/features/apps/components/AppsTable.module.css (1)
27-29: Verify asymmetric padding intention.The left padding (
--ds-size-11) is significantly larger than the other sides (--ds-size-2). Please confirm this asymmetry is intentional—perhaps to accommodate an icon or badge alignment within the Alert component.src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (3)
22-24: LGTM: Type signatures properly updated.The addition of
orgto the component props is correctly threaded through the component hierarchy to support environment-scoped error metrics fetching.Also applies to: 40-43, 53-53
120-129: LGTM: Error handling properly distinguishes permission issues.The 403 status check provides better UX by showing a specific warning for permission issues versus generic errors. The use of
isAxiosErrortype guard is appropriate.
147-189: LGTM: Sorting and rendering logic is correct.The filtering, sorting (apps with metrics first, then alphabetical), and metrics rendering flow is well-implemented. The conditional range parameter in links (line 164) provides good UX by only including it when non-default.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mlqn
force-pushed
the
feat/admin-alerts-designer
branch
from
5584dcb to
4d0d935
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (2)
94-94: Consider removing non-null assertions onrangefor improved type safety.The
rangevariable has multiple non-null assertions (range!) throughout the component (lines 100, 111, 180), even though it's initialised with a default value on line 94. IfuseQueryParamStatecan returnundefined, consider handling that case explicitly rather than using assertions. Alternatively, if the default value guaranteesrangeis always defined, the type signature ofuseQueryParamStatemight need adjustment.💡 Potential approaches
Option 1: Add explicit fallback where used:
const rangeValue = range ?? defaultRange; // Then use rangeValue instead of range!Option 2: Update the query param hook usage to ensure non-nullable:
const [range, setRange] = useQueryParamState<number>('range', defaultRange); // Ensure the hook's type definition guarantees non-null when default is providedAlso applies to: 100-100, 111-111, 180-180
147-189: Optional: Consider consolidating array operations.The current implementation applies multiple array operations in sequence (filter → map → sort → map). Whilst this is clear and works well for typical app lists, you could optionally combine some operations to reduce iterations:
💡 Example consolidation
{runningApps[env] .reduce((acc, app) => { if (search && !app.app.toLowerCase().includes(search.toLowerCase())) { return acc; } const appErrorMetrics = errorMetrics?.filter((metric) => metric.appName === app.app); acc.push({ ...app, metrics: appErrorMetrics, hasMetrics: (appErrorMetrics?.length ?? 0) > 0, }); return acc; }, []) .sort((a, b) => Number(b.hasMetrics) - Number(a.hasMetrics) || a.app.localeCompare(b.app)) .map((app) => ( // ... render row )) }This combines filter and the first map into a single reduce operation, though the current implementation is perfectly acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Designer/frontend/admin/features/apps/components/AppsTable.module.csssrc/Designer/frontend/admin/features/apps/components/AppsTable.test.tsxsrc/Designer/frontend/admin/features/apps/components/AppsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Designer/frontend/admin/features/apps/components/AppsTable.module.css
- src/Designer/frontend/admin/features/apps/components/AppsTable.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/*.cs : Register services in DI container properly following existing patterns for options configuration
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Applies to src/App/frontend/src/layout/**/*.{ts,tsx} : Layout components must have standardized structure with `config.ts`, `Component.tsx`, `index.tsx`, and generated `config.generated.ts` files
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx
📚 Learning: 2025-09-26T12:03:56.908Z
Learnt from: TomasEng
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-09-26T12:03:56.908Z
Learning: In the Altinn Studio codebase, when reviewing migration from studio/components-legacy to studio/components for the StudioExpression component, the ExpressionTexts type interface does not require additional keys like `dataModel`, `resource`, or `missingDataModelLabel`. TypeScript compilation will catch any missing required properties, so successful builds indicate proper type compliance.
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx
⏰ 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). (9)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Format check
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Testing
- GitHub Check: Try to generate script and to add a new migrations
- GitHub Check: CodeQL
- GitHub Check: Analyze
- GitHub Check: Typechecking and linting
- GitHub Check: Run integration tests against actual gitea and dbs
🔇 Additional comments (2)
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (2)
120-129: Good error handling differentiation.The error handling properly distinguishes between 403 (forbidden) errors and other failure cases, providing appropriate context to users based on the error type.
150-155: Operator precedence issue correctly resolved.The parentheses around
(appErrorMetrics?.length ?? 0) > 0on line 154 correctly fix the operator precedence issue identified in the previous review. ThehasMetricsboolean now evaluates properly for the sorting logic.
mlqn
force-pushed
the
feat/admin-alerts-designer
branch
from
4d0d935 to
68637ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (1)
100-100: Replace non-null assertions with nullish coalescing for defensive coding.The
rangevariable fromuseQueryParamStateis typed asnumber | undefined, requiring non-null assertions (range!) at lines 100, 112, and 180. Whilst the default value ensuresrangeshould never be undefined in practice, usingrange ?? defaultRangeis more defensive and eliminates potential runtime errors if the hook behaviour changes.🔎 Suggested refactor
const { data: errorMetrics, isPending: errorMetricsIsPending, error, isError: errorMetricsIsError, - } = useErrorMetricsQuery(org, env, range!, { + } = useErrorMetricsQuery(org, env, range ?? defaultRange, { hideDefaultError: true, });Apply similar changes at lines 112 and 180:
<TimeRangeSelect label={t('admin.metrics.errors')} - value={range!} + value={range ?? defaultRange} onChange={(e) => setRange(e)} />url={appErrorMetricsLogsPath(org, env, app.app, metric.name, range!)} + url={appErrorMetricsLogsPath(org, env, app.app, metric.name, range ?? defaultRange)}Also applies to: 112-112, 180-180
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Designer/frontend/admin/features/apps/components/AppsTable.module.csssrc/Designer/frontend/admin/features/apps/components/AppsTable.test.tsxsrc/Designer/frontend/admin/features/apps/components/AppsTable.tsxsrc/Designer/frontend/language/src/nb.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Designer/frontend/language/src/nb.json
- src/Designer/frontend/admin/features/apps/components/AppsTable.module.css
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Applies to src/App/frontend/**/*.test.{ts,tsx} : Use `renderWithProviders` utility from `src/test/renderWithProviders.tsx` when testing components that require form layout context
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.test.tsx
📚 Learning: 2025-08-08T13:24:30.117Z
Learnt from: nkylstad
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-08-08T13:24:30.117Z
Learning: In Altinn/altinn-studio PR reviews, when unit tests need React Query/TanStack Query data, prefer seeding with QueryClient.setQueryData over rendering/mocking use…Query hooks and waiting for isSuccess. Flag patterns like renderHookWithMockStore(() => use…Query(...)) and waitFor(() => expect(result.current.isSuccess).toBe(true)) and recommend the simpler setQueryData approach. Cite the internal Confluence guideline on component tests and the best-practice example in useAddItemToLayoutMutation.test.ts (lines updated 08.08.2025).
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.test.tsx
📚 Learning: 2025-11-25T08:20:59.697Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/backend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:20:59.697Z
Learning: Applies to src/App/backend/**/test/**/Fixtures/AppFixture.*.cs : Follow existing AppFixture.{Feature}.cs pattern for new API operations in integration tests
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.test.tsx
📚 Learning: 2025-01-27T14:02:18.762Z
Learnt from: standeren
Repo: Altinn/altinn-studio PR: 14520
File: frontend/app-development/layout/PageHeader/SubHeader/SettingsModalButton/SettingsModal/hooks/useUpdateAppTitle.ts:20-29
Timestamp: 2025-01-27T14:02:18.762Z
Learning: When working with mutations in Altinn Studio, prefer sync implementations with `onError` callbacks over async/await patterns to allow for future custom error handling without promise complexity.
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.test.tsx
📚 Learning: 2025-08-12T05:59:20.146Z
Learnt from: TomasEng
Repo: Altinn/altinn-studio PR: 16064
File: frontend/libs/studio-components/src/components/StudioCheckboxGroup/StudioGetCheckboxProps.ts:1-1
Timestamp: 2025-08-12T05:59:20.146Z
Learning: In the Altinn Studio codebase, deep exports from digdir/designsystemet-react should be avoided. The team prefers using shared wrappers for design system imports to prevent breakage from upstream internal path changes, but such refactoring should be done in separate PRs to maintain focused scope.
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.test.tsx
📚 Learning: 2025-08-12T06:09:55.275Z
Learnt from: TomasEng
Repo: Altinn/altinn-studio PR: 16064
File: frontend/libs/studio-components/src/components/StudioCheckboxGroup/StudioGetCheckboxProps.ts:1-1
Timestamp: 2025-08-12T06:09:55.275Z
Learning: In the Altinn Studio codebase, the preferred approach for eliminating dependencies on deep imports from digdir/designsystemet-react is to make the code completely independent of deep imports rather than using shared wrappers. This approach would eliminate the need for import path adjustments during future Design System package updates.
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.test.tsx
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Applies to src/App/frontend/src/layout/**/*.{ts,tsx} : Layout components must have standardized structure with `config.ts`, `Component.tsx`, `index.tsx`, and generated `config.generated.ts` files
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx
📚 Learning: 2025-09-26T12:03:56.908Z
Learnt from: TomasEng
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-09-26T12:03:56.908Z
Learning: In the Altinn Studio codebase, when reviewing migration from studio/components-legacy to studio/components for the StudioExpression component, the ExpressionTexts type interface does not require additional keys like `dataModel`, `resource`, or `missingDataModelLabel`. TypeScript compilation will catch any missing required properties, so successful builds indicate proper type compliance.
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx
🧬 Code graph analysis (2)
src/Designer/frontend/admin/features/apps/components/AppsTable.test.tsx (2)
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (2)
AppsTableProps(22-24)AppsTable(26-38)src/Designer/frontend/admin/hooks/useQueryParamState.ts (1)
useQueryParamState(4-44)
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (6)
src/Designer/frontend/admin/hooks/useQueryParamState.ts (1)
useQueryParamState(4-44)src/Designer/frontend/admin/hooks/queries/useErrorMetricsQuery.ts (1)
useErrorMetricsQuery(8-20)src/Designer/frontend/libs/studio-components-legacy/src/components/StudioSpinner/StudioSpinner.tsx (1)
StudioSpinner(24-52)src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx (1)
TimeRangeSelect(11-31)src/Designer/frontend/admin/shared/Alert/Alert.tsx (1)
Alert(17-40)src/Designer/frontend/admin/utils/apiPaths.ts (1)
appErrorMetricsLogsPath(9-16)
⏰ 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). (10)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run integration tests against actual gitea and dbs
- GitHub Check: Format check
- GitHub Check: CodeQL
- GitHub Check: Build environment and run e2e test
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
- GitHub Check: Analyze
🔇 Additional comments (3)
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (2)
120-129: LGTM! Well-structured error handling.The conditional rendering distinguishes between 403 Forbidden errors (showing a warning) and other errors (showing a danger alert), providing clear feedback to users about permission issues versus general failures.
147-189: LGTM! Correct sorting and filtering logic.Apps are correctly filtered by search term, enriched with metrics data, and sorted to prioritise apps with metrics before alphabetically ordering by name. The implementation is clear and efficient.
src/Designer/frontend/admin/features/apps/components/AppsTable.test.tsx (1)
1-211: LGTM! Comprehensive test coverage following best practices.The test suite effectively covers:
- Loading and error states (including differentiated 403 vs other errors)
- Metrics rendering with seeded data
- User interactions (range selection)
The tests follow the recommended pattern of using
setQueryDatato seed React Query data rather than mocking hooks and waiting forisSuccess, which aligns with internal best practices.Based on learnings, the test approach using
QueryClient.setQueryDatais the preferred pattern in this codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/Designer/frontend/admin/shared/Alert/Alert.module.css (2)
8-10: Replace hardcoded pixel values with design system tokens.The
.metric:beforeselector uses hardcoded pixel values (3px,-20px) which reduce maintainability and may not scale consistently with the design system.🔎 Consider using design system spacing variables
.metric:before { - margin-top: 3px; - margin-left: -20px; + margin-top: var(--ds-size-1); + margin-left: calc(-1 * var(--ds-size-5)); }
17-17: Replace hardcoded pixel value with design system token.Line 17 uses
padding-left: 10pxinstead of a design system variable.🔎 Proposed change
.heading { display: flex; gap: var(--ds-size-4); justify-content: space-between; align-items: center; - padding-left: 10px; + padding-left: var(--ds-size-3); }src/Designer/frontend/admin/features/apps/components/AppsTable.module.css (1)
15-17: Hardcoded height may cause layout inconsistencies.The
.errorMetricsCellclass uses a fixed height of39px. This may not scale properly with different font sizes or design system updates.Consider using a flexible height or a design system token if available. If the height is critical for alignment, document the reason.
src/Designer/frontend/admin/shared/Alert/Alert.tsx (1)
9-15: Strengthen prop types for better type safety.The
colorprop accepts any string, but the CSS module only supports specific values ('danger','success','info'). Thecountprop is typed as string when it semantically represents a number.🔎 Proposed type improvements
type AlertProps = { - color: string; - count?: string; + color: 'danger' | 'success' | 'info'; + count?: number; title: ReactElement | string; url?: string; children?: ReactElement; } & Omit<StudioAlertProps, 'children' | 'title'>;Then update line 28 to convert the number to string:
- {count && <span className={classes.count}>{count}</span>} + {count !== undefined && <span className={classes.count}>{count.toString()}</span>}src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (2)
100-102: Non-null assertions bypass type safety.Lines 100, 112, and 184 use non-null assertions (
range!) assumingrangeis always defined due to the default value. However,useQueryParamStatereturnsnumber | undefined, which doesn't guarantee the default is applied in the type system.🔎 Consider asserting the type or updating the hook
Option 1: Assert the value is defined after retrieval:
const [range, setRange] = useQueryParamState<number>('range', defaultRange); + const rangeValue = range ?? defaultRange; const { data: errorMetrics, isPending: errorMetricsIsPending, error, isError: errorMetricsIsError, - } = useErrorMetricsQuery(org, env, range!, { + } = useErrorMetricsQuery(org, env, rangeValue, { hideDefaultError: true, });Then update lines 112 and 184 to use
rangeValue.Option 2: Improve
useQueryParamStatetyping to reflect that a non-undefined default guarantees a non-undefined return value.
163-167: Range conditional logic may fail for falsy values.Line 164 uses
range && range !== defaultRangeto determine whether to include the range query parameter. Ifrangeis0, this evaluates tofalseeven though0is a valid number.🔎 More explicit check
<Link - to={`${env}/${app.app}${range && range !== defaultRange ? '?range=' + range : ''}`} + to={`${env}/${app.app}${range !== undefined && range !== defaultRange ? '?range=' + range : ''}`} > {app.app} </Link>Note: Given the available range options in
TimeRangeSelect(5, 15, 30, ...),0is not a valid selection, so this is more of a defensive programming improvement than a critical fix.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Designer/frontend/admin/features/apps/components/AppsTable.module.csssrc/Designer/frontend/admin/features/apps/components/AppsTable.tsxsrc/Designer/frontend/admin/shared/Alert/Alert.module.csssrc/Designer/frontend/admin/shared/Alert/Alert.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
Learnt from: framitdavid
Repo: Altinn/altinn-studio PR: 16372
File: src/Designer/frontend/libs/studio-components/src/components/index.ts:34-34
Timestamp: 2025-09-23T08:53:19.508Z
Learning: In the Altinn Studio codebase, when moving components from studio-components-legacy to studio-components, the team prefers to handle the component migration and remaining import updates in separate PRs to maintain focused, atomic changes.
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 16095
File: src/App/app-template-dotnet/src/App/models/model.cs:10-29
Timestamp: 2025-08-15T05:53:32.529Z
Learning: When moving app templates in Altinn Studio, mirkoSekulic prefers to keep the PR scope strictly limited to the move operation itself, with all content changes (including code quality improvements, naming convention fixes, and attribute cleanup) deferred to separate PRs for better maintainability and atomic changes.
Learnt from: ErlingHauan
Repo: Altinn/altinn-studio PR: 15977
File: frontend/libs/studio-components-legacy/src/components/StudioCard/index.tsx:2-7
Timestamp: 2025-08-06T12:36:57.397Z
Learning: When a developer copies a component from `studio-components-legacy` to `studio-components` in the Altinn Studio repository and has not added a deprecation comment for the component in `studio-components-legacy`, suggest adding the deprecation comment in the PR review.
Learnt from: ErlingHauan
Repo: Altinn/altinn-studio PR: 15977
File: frontend/libs/studio-components-legacy/src/components/StudioCard/index.tsx:2-7
Timestamp: 2025-08-06T12:37:01.381Z
Learning: When reviewing PRs in the Altinn Studio repository where a developer copies a component from `studio-components-legacy` to `studio-components`, suggest adding a `deprecated` JSDoc comment to the component in `studio-components-legacy` if one hasn't been added. The deprecation comment should recommend using the component from `studio/components` instead.
📚 Learning: 2025-10-26T21:09:38.402Z
Learnt from: JamalAlabdullah
Repo: Altinn/altinn-studio PR: 16742
File: src/Designer/frontend/packages/schema-editor/src/components/SchemaInspector/ItemDataComponent.module.css:23-25
Timestamp: 2025-10-26T21:09:38.402Z
Learning: In the Altinn Studio codebase, the design system is migrating from `--fds-*` prefixed CSS variables to `--ds-*` prefixed variables. The `--fds-*` prefix will be removed in the future, so usage of `--ds-*` variables (like `--ds-size-4`) alongside existing `--fds-*` variables (like `--fds-spacing-4`) in the same file is intentional during this transition period.
<!--
Applied to files:
src/Designer/frontend/admin/shared/Alert/Alert.module.csssrc/Designer/frontend/admin/features/apps/components/AppsTable.module.css
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Applies to src/App/frontend/**/*.module.css : Use CSS Modules for component styling with `.module.css` file extension
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.module.css
📚 Learning: 2025-11-25T08:21:14.748Z
Learnt from: CR
Repo: Altinn/altinn-studio PR: 0
File: src/App/frontend/CLAUDE.md:0-0
Timestamp: 2025-11-25T08:21:14.748Z
Learning: Applies to src/App/frontend/src/layout/**/*.{ts,tsx} : Layout components must have standardized structure with `config.ts`, `Component.tsx`, `index.tsx`, and generated `config.generated.ts` files
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx
📚 Learning: 2025-09-26T12:03:56.908Z
Learnt from: TomasEng
Repo: Altinn/altinn-studio PR: 0
File: :0-0
Timestamp: 2025-09-26T12:03:56.908Z
Learning: In the Altinn Studio codebase, when reviewing migration from studio/components-legacy to studio/components for the StudioExpression component, the ExpressionTexts type interface does not require additional keys like `dataModel`, `resource`, or `missingDataModelLabel`. TypeScript compilation will catch any missing required properties, so successful builds indicate proper type compliance.
Applied to files:
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx
🧬 Code graph analysis (1)
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (5)
src/Designer/frontend/admin/hooks/useQueryParamState.ts (1)
useQueryParamState(4-44)src/Designer/frontend/admin/hooks/queries/useErrorMetricsQuery.ts (1)
useErrorMetricsQuery(8-20)src/Designer/frontend/admin/shared/TimeRangeSelect/TimeRangeSelect.tsx (1)
TimeRangeSelect(11-31)src/Designer/frontend/admin/shared/Alert/Alert.tsx (1)
Alert(17-40)src/Designer/frontend/admin/utils/apiPaths.ts (1)
appErrorMetricsLogsPath(9-16)
⏰ 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). (9)
- GitHub Check: Run integration tests against actual gitea and dbs
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Analyze
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
🔇 Additional comments (1)
src/Designer/frontend/admin/features/apps/components/AppsTable.tsx (1)
172-188: LGTM! Clean metric rendering implementation.The per-app error metrics are rendered clearly with proper Alert components, localized labels, and direct links to logs. The filtering, sorting, and display logic work together well.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs (1)
47-127: Consider removing unnecessary string interpolation for consistency.Several
CreateClientcalls use string interpolation ($"runtime-gateway") when a plain string literal would suffice. For consistency with lines 31 and 136, consider using plain string literals throughout.♻️ Suggested consistency fix
- using var client = _httpClientFactory.CreateClient($"runtime-gateway"); + using var client = _httpClientFactory.CreateClient("runtime-gateway");Apply this change to lines 47, 62, 78, 94, 111, and 127.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
src/Designer/backend/src/Designer/Infrastructure/ServiceRegistration.cssrc/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/IRuntimeGatewayClient.cssrc/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cssrc/Designer/frontend/admin/utils/apiPaths.tssrc/Designer/frontend/language/src/nb.jsonsrc/Designer/frontend/package.jsonsrc/Designer/frontend/packages/shared/src/types/QueryKey.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Designer/backend/src/Designer/Infrastructure/ServiceRegistration.cs
- src/Designer/frontend/package.json
- src/Designer/frontend/admin/utils/apiPaths.ts
- src/Designer/frontend/packages/shared/src/types/QueryKey.ts
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
📚 Learning: 2025-06-03T10:54:59.508Z
Learnt from: wrt95
Repo: Altinn/altinn-studio PR: 15542
File: frontend/app-development/features/appSettings/components/TabsContent/Tabs/AboutTab/utils/appResourceValidationUtils.ts:64-95
Timestamp: 2025-06-03T10:54:59.508Z
Learning: In the app settings validation logic for appResourceValidationUtils.ts, the Norwegian Bokmål ('nb') language uses a comprehensive error message via getMissingInputLanguageString that can describe multiple missing languages, while Norwegian Nynorsk ('nn') and English ('en') use simpler, direct translation keys for missing translations. This pattern should be preserved when refactoring validation logic.
Applied to files:
src/Designer/frontend/language/src/nb.json
📚 Learning: 2025-06-02T08:50:48.884Z
Learnt from: Jondyr
Repo: Altinn/altinn-studio PR: 15537
File: frontend/language/src/nb.json:1932-1932
Timestamp: 2025-06-02T08:50:48.884Z
Learning: In frontend/language/src/nb.json, the localization keys use hierarchical dot notation with multiple segments after the namespace (e.g., ux_editor.add_item.add_component, ux_editor.component_category.advanced). Keys with 2-3 segments after ux_editor are both common and accepted patterns.
Applied to files:
src/Designer/frontend/language/src/nb.json
📚 Learning: 2025-01-20T10:00:55.997Z
Learnt from: standeren
Repo: Altinn/altinn-studio PR: 14449
File: frontend/packages/process-editor/src/components/ConfigPanel/ConfigContent/EditTaskId/EditTaskId.tsx:47-55
Timestamp: 2025-01-20T10:00:55.997Z
Learning: The project only supports Norwegian translations in the solution. English translation files are not maintained.
Applied to files:
src/Designer/frontend/language/src/nb.json
📚 Learning: 2025-01-30T08:18:42.814Z
Learnt from: Jondyr
Repo: Altinn/altinn-studio PR: 14517
File: frontend/language/src/nb.json:1385-1385
Timestamp: 2025-01-30T08:18:42.814Z
Learning: English translations (en.json) are not currently used in the Altinn Studio project. Comments about missing English translations should be ignored.
Applied to files:
src/Designer/frontend/language/src/nb.json
📚 Learning: 2025-06-20T13:41:45.278Z
Learnt from: TomasEng
Repo: Altinn/altinn-studio PR: 15719
File: frontend/dashboard/pages/OrgContentLibraryPage/FeedbackForm/FeedbackForm.tsx:22-39
Timestamp: 2025-06-20T13:41:45.278Z
Learning: In the Altinn Studio project, temporary feedback form components may intentionally use hardcoded text instead of internationalization when they are designed to be removed before language support is implemented.
Applied to files:
src/Designer/frontend/language/src/nb.json
📚 Learning: 2025-01-28T22:18:03.328Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:43-48
Timestamp: 2025-01-28T22:18:03.328Z
Learning: Error handling improvements for the Get method in AzureDevOpsBuildClient were deemed out of scope for PR #14456, which focused on undeploy endpoints functionality.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-07-08T09:19:49.084Z
Learnt from: mlqn
Repo: Altinn/altinn-studio PR: 15838
File: src/KubernetesWrapper/src/Controllers/FailedRequestsController.cs:0-0
Timestamp: 2025-07-08T09:19:49.084Z
Learning: User mlqn prefers to handle error handling improvements for ASP.NET Core controllers in separate PRs rather than including them in feature PRs, as demonstrated when error handling for the FailedRequestsController was moved to PR #15831 while the main feature was in PR #15838.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-01-28T22:20:43.402Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.402Z
Learning: In the Altinn Studio project, HTTP status codes are handled globally through a delegating handler, eliminating the need for explicit EnsureSuccessStatusCode() calls in individual HTTP clients.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-02-11T13:27:27.159Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14627
File: backend/src/Designer/TypedHttpClients/AltinnStorage/AltinnStorageAppMetadataClient.cs:0-0
Timestamp: 2025-02-11T13:27:27.159Z
Learning: In Altinn.Studio.Designer's HTTP clients, success status codes are handled by delegating handlers in the request pipeline, making explicit status code checks in individual HTTP client methods unnecessary.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-02-11T13:27:27.159Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14627
File: backend/src/Designer/TypedHttpClients/AltinnStorage/AltinnStorageAppMetadataClient.cs:0-0
Timestamp: 2025-02-11T13:27:27.159Z
Learning: HTTP status codes are handled by delegating handlers in the Altinn.Studio.Designer codebase, making explicit status code checks in individual HTTP clients unnecessary.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-01-28T22:17:05.810Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/Services/Implementation/ReleaseService.cs:66-69
Timestamp: 2025-01-28T22:17:05.810Z
Learning: The GetDeveloperAppTokenAsync() method in Altinn.Studio.Designer is called within protected endpoints where authentication is already enforced, making additional null checks unnecessary.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-02-11T13:27:27.159Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14627
File: backend/src/Designer/TypedHttpClients/AltinnStorage/AltinnStorageAppMetadataClient.cs:0-0
Timestamp: 2025-02-11T13:27:27.159Z
Learning: In Altinn.Studio.Designer's HTTP clients, the `EnsureSuccessHandler` delegating handler is responsible for ensuring successful HTTP status codes, making explicit status code checks in individual HTTP client methods unnecessary.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-01-28T22:20:43.402Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.402Z
Learning: The QueueBuildRequest class in Altinn Studio uses System.Text.Json serialization annotations directly on the class properties, making explicit serializer options unnecessary.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-01-28T22:20:43.401Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.401Z
Learning: In Altinn Studio, HTTP status codes are handled globally through the EnsureSuccessHandler delegating handler, which throws HttpRequestWithStatusException for non-success status codes and provides detailed error logging.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-01-28T22:20:43.402Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 14456
File: backend/src/Designer/TypedHttpClients/AzureDevOps/AzureDevOpsBuildClient.cs:70-75
Timestamp: 2025-01-28T22:20:43.402Z
Learning: The QueueBuildRequest class in Altinn Studio uses JsonPropertyName attributes from System.Text.Json for serialization (e.g., [JsonPropertyName("definition")] for the DefinitionReference property).
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs
📚 Learning: 2025-12-11T09:10:06.960Z
Learnt from: mirkoSekulic
Repo: Altinn/altinn-studio PR: 17174
File: src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs:1-8
Timestamp: 2025-12-11T09:10:06.960Z
Learning: In C# 12, you can declare a class, interface, or struct with an empty body using a trailing semicolon as a shorthand (e.g., internal sealed class X : Y;). When reviewing StudioGateway.Api code under src/Runtime/StudioGateway/src/StudioGateway.Api/Clients/Designer/StudioEnvironmentConfig.cs and other C# files, verify the language version is set to at least 12.0+ and that any empty-body types use the single semicolon shorthand only when semantically appropriate (i.e., the type has no members). If a type previously had members, continue using a normal body.
Applied to files:
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cssrc/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/IRuntimeGatewayClient.cs
🧬 Code graph analysis (2)
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs (6)
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/IRuntimeGatewayClient.cs (8)
Task(12-12)Task(13-13)Task(14-14)Task(15-15)Task(16-16)Task(17-17)Task(18-18)Task(19-19)src/Designer/backend/src/Designer/Services/Implementation/MetricsService.cs (5)
Task(16-24)Task(27-36)Task(39-48)Task(51-61)Task(64-72)src/Designer/backend/src/Designer/Services/Interfaces/IMetricsService.cs (3)
Task(11-16)Task(18-24)Task(26-32)src/Designer/backend/src/Designer/Models/Alerts/AlertRule.cs (1)
AlertRule(6-37)src/Designer/backend/src/Designer/Models/Metrics/ErrorMetric.cs (1)
ErrorMetric(3-8)src/Designer/backend/src/Designer/Models/Metrics/AppErrorMetric.cs (1)
AppErrorMetric(5-9)
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/IRuntimeGatewayClient.cs (6)
src/Designer/backend/src/Designer/Models/Alerts/AlertRule.cs (1)
AlertRule(6-37)src/Designer/backend/src/Designer/Models/Metrics/ErrorMetric.cs (1)
ErrorMetric(3-8)src/Designer/frontend/admin/types/metrics/ErrorMetric.ts (1)
ErrorMetric(1-5)src/Designer/frontend/admin/features/appDetails/components/AppMetric.tsx (1)
AppMetric(14-28)src/Designer/backend/src/Designer/Models/Metrics/AppErrorMetric.cs (1)
AppErrorMetric(5-9)src/Designer/frontend/admin/features/appDetails/components/AppHealthMetric.tsx (1)
AppHealthMetric(11-35)
⏰ 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). (9)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run integration tests against actual gitea and dbs
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: Typechecking and linting
- GitHub Check: CodeQL
- GitHub Check: Analyze
🔇 Additional comments (3)
src/Designer/frontend/language/src/nb.json (1)
31-47: LGTM! The Norwegian translations are well-formed and consistent.The new admin.metrics translation keys are grammatically correct and follow Norwegian language conventions. The terminology is consistent throughout, and placeholder usage ({{env}}) is correct.
Note: Lines 39 and 46 contain intentionally incomplete phrases ("Feil fra siste", "Statistikk fra siste") which appear designed for dynamic composition with time ranges in the UI (e.g., "Feil fra siste 24 timer").
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/IRuntimeGatewayClient.cs (1)
1-18: LGTM!The interface additions are well-structured and follow established conventions. The six new methods provide comprehensive coverage for alerts and metrics retrieval, with appropriate async patterns, cancellation token support, and properly typed return values.
src/Designer/backend/src/Designer/TypedHttpClients/RuntimeGateway/RuntimeGatewayClient.cs (1)
40-132: LGTM! All previous issues have been addressed.The six new method implementations correctly follow the established pattern in this class:
- Proper URL encoding with
Uri.EscapeDataStringfor query parameters- Safe dictionary access using
TryGetValueinGetAppErrorMetricsLogsAsync- Consistent null-coalescing to empty collections or strings
- Appropriate use of cancellation tokens
All critical and major issues from previous reviews have been resolved.
2 participants
Description
useWebSockethookAppDetailsandInstancesVerification
Summary by CodeRabbit
New Features
Bug Fixes / Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.