From f789378cabb037c7a7f3563e6165f983a741fd0c Mon Sep 17 00:00:00 2001 From: Jaanus Sellin Date: Thu, 3 Jul 2025 15:32:56 +0300 Subject: [PATCH 1/8] feat: add query timer to event store (#10306) I noticed event search, as it is doing `ILIKE` search, is slow sometimes. Lets get some statistics about it. Meanwhile added timers for other interesting queries. --- src/lib/features/events/event-store.ts | 42 ++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/lib/features/events/event-store.ts b/src/lib/features/events/event-store.ts index 0d8a8f0ca6..43d86b43b4 100644 --- a/src/lib/features/events/event-store.ts +++ b/src/lib/features/events/event-store.ts @@ -28,6 +28,8 @@ import type { ProjectActivitySchema } from '../../openapi/index.js'; import type { IQueryParam } from '../feature-toggle/types/feature-toggle-strategies-store-type.js'; import { applyGenericQueryParams } from '../feature-search/search-utils.js'; import type { ITag } from '../../tags/index.js'; +import metricsHelper from '../../util/metrics-helper.js'; +import { DB_TIME } from '../../metric-events.js'; const EVENT_COLUMNS = [ 'id', @@ -113,26 +115,38 @@ export class EventStore implements IEventStore { private logger: Logger; + private metricTimer: Function; + // a new DB has to be injected per transaction constructor(db: Db, getLogger: LogProvider) { this.db = db; this.logger = getLogger('event-store'); + this.metricTimer = (action) => + metricsHelper.wrapTimer(this.eventEmitter, DB_TIME, { + store: 'event', + action, + }); } async store(event: IBaseEvent): Promise { + const stopTimer = this.metricTimer('store'); try { await this.db(TABLE) .insert(this.eventToDbRow(event)) .returning(EVENT_COLUMNS); } catch (error: unknown) { this.logger.warn(`Failed to store "${event.type}" event: ${error}`); + } finally { + stopTimer(); } } async count(): Promise { + const stopTimer = this.metricTimer('count'); const count = await this.db(TABLE) .count>() .first(); + stopTimer(); if (!count) { return 0; } @@ -147,8 +161,10 @@ export class EventStore implements IEventStore { queryParams: IQueryParam[], query?: IEventSearchParams['query'], ): Promise { + const stopTimer = this.metricTimer('searchEventsCount'); const searchQuery = this.buildSearchQuery(queryParams, query); const count = await searchQuery.count().first(); + stopTimer(); if (!count) { return 0; } @@ -160,6 +176,7 @@ export class EventStore implements IEventStore { } async batchStore(events: IBaseEvent[]): Promise { + const stopTimer = this.metricTimer('batchStore'); try { await this.db(TABLE).insert( events.map((event) => this.eventToDbRow(event)), @@ -169,10 +186,13 @@ export class EventStore implements IEventStore { `Failed to store events: ${JSON.stringify(events)}`, error, ); + } finally { + stopTimer(); } } async getMaxRevisionId(largerThan: number = 0): Promise { + const stopTimer = this.metricTimer('getMaxRevisionId'); const row = await this.db(TABLE) .max('id') .where((builder) => @@ -193,10 +213,12 @@ export class EventStore implements IEventStore { ) .andWhere('id', '>=', largerThan) .first(); + stopTimer(); return row?.max ?? 0; } async getRevisionRange(start: number, end: number): Promise { + const stopTimer = this.metricTimer('getRevisionRange'); const query = this.db .select(EVENT_COLUMNS) .from(TABLE) @@ -246,6 +268,7 @@ export class EventStore implements IEventStore { } async query(operations: IQueryOperations[]): Promise { + const stopTimer = this.metricTimer('query'); try { let query: Knex.QueryBuilder = this.select(); @@ -271,10 +294,13 @@ export class EventStore implements IEventStore { return rows.map(this.rowToEvent); } catch (e) { return []; + } finally { + stopTimer(); } } async queryCount(operations: IQueryOperations[]): Promise { + const stopTimer = this.metricTimer('queryCount'); try { let query: Knex.QueryBuilder = this.db.count().from(TABLE); @@ -300,6 +326,8 @@ export class EventStore implements IEventStore { return Number.parseInt(queryResult.count || 0); } catch (e) { return 0; + } finally { + stopTimer(); } } @@ -355,6 +383,7 @@ export class EventStore implements IEventStore { } async getEvents(query?: Object): Promise { + const stopTimer = this.metricTimer('getEvents'); try { let qB = this.db .select(EVENT_COLUMNS) @@ -371,6 +400,8 @@ export class EventStore implements IEventStore { return rows.map(this.rowToEvent); } catch (err) { return []; + } finally { + stopTimer(); } } @@ -379,6 +410,7 @@ export class EventStore implements IEventStore { queryParams: IQueryParam[], options?: { withIp?: boolean }, ): Promise { + const stopTimer = this.metricTimer('searchEvents'); const query = this.buildSearchQuery(queryParams, params.query) .select(options?.withIp ? [...EVENT_COLUMNS, 'ip'] : EVENT_COLUMNS) .orderBy([ @@ -396,6 +428,8 @@ export class EventStore implements IEventStore { ); } catch (err) { return []; + } finally { + stopTimer(); } } @@ -420,6 +454,7 @@ export class EventStore implements IEventStore { } async getEventCreators(): Promise> { + const stopTimer = this.metricTimer('getEventCreators'); const query = this.db('events') .distinctOn('events.created_by_user_id') .leftJoin('users', 'users.id', '=', 'events.created_by_user_id') @@ -437,6 +472,7 @@ export class EventStore implements IEventStore { ]); const result = await query; + stopTimer(); return result .filter((row: any) => row.name || row.username || row.email) .map((row: any) => ({ @@ -448,6 +484,7 @@ export class EventStore implements IEventStore { async getProjectRecentEventActivity( project: string, ): Promise { + const stopTimer = this.metricTimer('getProjectRecentEventActivity'); const result = await this.db('events') .select( this.db.raw("TO_CHAR(created_at::date, 'YYYY-MM-DD') AS date"), @@ -462,6 +499,7 @@ export class EventStore implements IEventStore { .groupBy(this.db.raw("TO_CHAR(created_at::date, 'YYYY-MM-DD')")) .orderBy('date', 'asc'); + stopTimer(); return result.map((row) => ({ date: row.date, count: Number(row.count), @@ -531,10 +569,12 @@ export class EventStore implements IEventStore { } async setUnannouncedToAnnounced(): Promise { + const stopTimer = this.metricTimer('setUnannouncedToAnnounced'); const rows = await this.db(TABLE) .update({ announced: true }) .where('announced', false) .returning(EVENT_COLUMNS); + stopTimer(); return rows.map(this.rowToEvent); } @@ -545,6 +585,7 @@ export class EventStore implements IEventStore { } async setCreatedByUserId(batchSize: number): Promise { + const stopTimer = this.metricTimer('setCreatedByUserId'); const API_TOKEN_TABLE = 'api_tokens'; const toUpdate = await this.db(`${TABLE} as e`) @@ -592,6 +633,7 @@ export class EventStore implements IEventStore { }); await Promise.all(updatePromises); + stopTimer(); return toUpdate.length; } } From 2b780e01207c4afab41451d2d2c7307e20dce81f Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 3 Jul 2025 14:49:36 +0200 Subject: [PATCH 2/8] Set a max width on the CR sidebar instead of a min width. (#10307) The max width is set to `max(40vw, 1000px)`, so that if you're on a very wide window, then it'll take up at most 40% of the horizontal space. Once your window is smaller than 2500px, however, the sidebar will stop shrinking and stay at 1000px (or as close to that as the window allows). It'll keep shrinking with the window size. This came up because in certain cases, such as if you have a release template with a long description, the modal would just keep growing until it took up 98% of the window width. I have not set a min width for now. I don't think there is any need for it, but if we find there is, we can add it back later. Before: ![image](https://github.com/user-attachments/assets/815b014b-765f-4670-8724-dc70a71b3c17) After: ![image](https://github.com/user-attachments/assets/d2b59410-0907-4606-81b3-8103dfbcb44c) --- .../changeRequest/ChangeRequestSidebar/ChangeRequestSidebar.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/component/changeRequest/ChangeRequestSidebar/ChangeRequestSidebar.tsx b/frontend/src/component/changeRequest/ChangeRequestSidebar/ChangeRequestSidebar.tsx index f855af363d..dadc379831 100644 --- a/frontend/src/component/changeRequest/ChangeRequestSidebar/ChangeRequestSidebar.tsx +++ b/frontend/src/component/changeRequest/ChangeRequestSidebar/ChangeRequestSidebar.tsx @@ -22,7 +22,7 @@ interface IChangeRequestSidebarProps { const StyledPageContent = styled(PageContent)(({ theme }) => ({ height: '100vh', overflow: 'auto', - minWidth: '50vw', + maxWidth: 'max(40vw, 1000px)', padding: theme.spacing(6), [theme.breakpoints.down('md')]: { padding: theme.spacing(4, 2), From 51a895b66095af3710a2169683a9418220fe79e8 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 3 Jul 2025 14:58:13 +0200 Subject: [PATCH 3/8] Chore(1-3894): update old/new name display + remove strategy type names (#10305) This PR updates how we show old/new strategy/segment names in change requests, and also removes the name of the strategy type from the change. For the old/new names: instead of showing them stacked vertically, we show them side by side (old name first, then new name). Compare before: image with after: image Only affects the new components (legacy CR view is untouched). If we get negative feedback while rolling it out because the strat type name is missing, we can always add it back. --- .../Changes/Change/ChangeSegmentName.tsx | 18 ----------- .../Changes/Change/ChangeStrategyName.tsx | 30 ------------------- .../NameWithChangeInfo/NameWithChangeInfo.tsx | 1 - .../Changes/Change/SegmentChangeDetails.tsx | 10 +++---- .../Changes/Change/StrategyChange.test.tsx | 2 -- .../Changes/Change/StrategyChange.tsx | 23 ++++++-------- 6 files changed, 14 insertions(+), 70 deletions(-) delete mode 100644 frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeSegmentName.tsx delete mode 100644 frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeStrategyName.tsx diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeSegmentName.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeSegmentName.tsx deleted file mode 100644 index dcb1ea979c..0000000000 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeSegmentName.tsx +++ /dev/null @@ -1,18 +0,0 @@ -import type { FC } from 'react'; -import { NameWithChangeInfo } from './NameWithChangeInfo/NameWithChangeInfo.tsx'; - -type ChangeSegmentNameProps = { - name: string; - previousName?: string; -}; - -export const ChangeSegmentName: FC = ({ - name, - previousName, -}) => { - return ( - - - - ); -}; diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeStrategyName.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeStrategyName.tsx deleted file mode 100644 index 20eb1fca20..0000000000 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeStrategyName.tsx +++ /dev/null @@ -1,30 +0,0 @@ -import type { FC } from 'react'; -import { formatStrategyName } from 'utils/strategyNames'; -import { styled } from '@mui/material'; -import { textTruncated } from 'themes/themeStyles'; -import { NameWithChangeInfo } from './NameWithChangeInfo/NameWithChangeInfo.tsx'; -import { Truncator } from 'component/common/Truncator/Truncator.tsx'; - -type ChangeStrategyNameProps = { - name: string; - title?: string; - previousTitle?: string; -}; - -const Truncated = styled('span')(() => ({ - ...textTruncated, - maxWidth: 500, -})); - -export const ChangeStrategyName: FC = ({ - name, - title, - previousTitle, -}) => { - return ( - - {formatStrategyName(name)} - - - ); -}; diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NameWithChangeInfo/NameWithChangeInfo.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NameWithChangeInfo/NameWithChangeInfo.tsx index d840ed899e..9fddf0a210 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NameWithChangeInfo/NameWithChangeInfo.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/NameWithChangeInfo/NameWithChangeInfo.tsx @@ -6,7 +6,6 @@ import { textTruncated } from 'themes/themeStyles'; const Truncated = styled('span')(() => ({ ...textTruncated, maxWidth: 500, - display: 'block', })); const NewName = styled(Typography)({ diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx index 08bd89552f..519de546dc 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx @@ -17,7 +17,7 @@ import { Deleted, } from './Change.styles.tsx'; import { SegmentDiff } from './SegmentDiff.tsx'; -import { ChangeSegmentName } from './ChangeSegmentName.tsx'; +import { NameWithChangeInfo } from './NameWithChangeInfo/NameWithChangeInfo.tsx'; const ActionsContainer = styled('div')(({ theme }) => ({ display: 'flex', @@ -72,8 +72,8 @@ export const SegmentChangeDetails: FC<{ Deleting segment - @@ -102,8 +102,8 @@ export const SegmentChangeDetails: FC<{ Editing segment - diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx index 01c18edbdf..8121ec7f28 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx @@ -243,7 +243,6 @@ test('Deleting strategy before change request is applied diffs against current s ); await screen.findByText('Deleting strategy'); - await screen.findByText('Gradual rollout'); await screen.findByText('current_title'); await screen.findByText('current_variant'); @@ -299,7 +298,6 @@ test('Deleting strategy after change request is applied diffs against the snapsh ); await screen.findByText('Deleting strategy'); - await screen.findByText('Gradual rollout'); await screen.findByText('snapshot_title'); expect(screen.queryByText('current_title')).not.toBeInTheDocument(); diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx index 9a51ddae1a..374c99ac13 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx @@ -18,7 +18,6 @@ import { EnvironmentVariantsTable } from 'component/feature/FeatureView/FeatureV import { ChangeOverwriteWarning } from './ChangeOverwriteWarning/ChangeOverwriteWarning.tsx'; import type { IFeatureStrategy } from 'interfaces/strategy'; import { Tab, TabList, TabPanel, Tabs } from './ChangeTabComponents.tsx'; -import { ChangeStrategyName } from './ChangeStrategyName.tsx'; import { StrategyDiff } from './StrategyDiff.tsx'; import { Action, @@ -27,6 +26,7 @@ import { ChangeItemWrapper, Deleted, } from './Change.styles.tsx'; +import { NameWithChangeInfo } from './NameWithChangeInfo/NameWithChangeInfo.tsx'; const StyledBox: FC<{ children?: React.ReactNode }> = styled(Box)( ({ theme }) => ({ @@ -103,10 +103,6 @@ const DeleteStrategy: FC<{ currentStrategy: IFeatureStrategy | undefined; actions?: ReactNode; }> = ({ change, changeRequestState, currentStrategy, actions }) => { - const name = - changeRequestState === 'Applied' - ? change.payload?.snapshot?.name - : currentStrategy?.name; const title = changeRequestState === 'Applied' ? change.payload?.snapshot?.title @@ -121,7 +117,10 @@ const DeleteStrategy: FC<{ Deleting strategy - + {actions} @@ -175,10 +174,9 @@ const UpdateStrategy: FC<{ wasDisabled={currentStrategy?.disabled} willBeDisabled={change.payload?.disabled} /> - {actions} @@ -248,10 +246,7 @@ const AddStrategy: FC<{ Adding {isDefaultChange && 'default'} strategy - + Date: Fri, 4 Jul 2025 10:47:11 +0200 Subject: [PATCH 4/8] fix: frontend API CORS (#10301) --- src/lib/app.ts | 5 +---- src/lib/features/frontend-api/frontend-api-controller.ts | 5 ----- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/lib/app.ts b/src/lib/app.ts index 4b09a14a70..0037c0d88b 100644 --- a/src/lib/app.ts +++ b/src/lib/app.ts @@ -103,10 +103,7 @@ export default async function getApp( if (config.enableOAS && services.openApiService) { services.openApiService.useDocs(app); } - // Support CORS preflight requests for the frontend endpoints. - // Preflight requests should not have Authorization headers, - // so this must be handled before the API token middleware. - app.options( + app.use( `${baseUriPath}/api/frontend*`, corsOriginMiddleware(services, config), ); diff --git a/src/lib/features/frontend-api/frontend-api-controller.ts b/src/lib/features/frontend-api/frontend-api-controller.ts index 85ce27fe2e..648868e04a 100644 --- a/src/lib/features/frontend-api/frontend-api-controller.ts +++ b/src/lib/features/frontend-api/frontend-api-controller.ts @@ -19,7 +19,6 @@ import { } from '../../openapi/index.js'; import type { Context } from 'unleash-client'; import { enrichContextWithIp } from './index.js'; -import { corsOriginMiddleware } from '../../middleware/index.js'; import NotImplementedError from '../../error/not-implemented-error.js'; import rateLimit from 'express-rate-limit'; import { minutesToMilliseconds } from 'date-fns'; @@ -65,10 +64,6 @@ export default class FrontendAPIController extends Controller { functionName, }); - // Support CORS requests for the frontend endpoints. - // Preflight requests are handled in `app.ts`. - this.app.use(corsOriginMiddleware(services, config)); - this.route({ method: 'get', path: '', From e516bbf14ca9e2d97e9051bdf37284ecc6f5669a Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 4 Jul 2025 13:32:46 +0200 Subject: [PATCH 5/8] chore: renames "Change" tab to "View change" to align with "View diff". (#10314) There was some confusion whether the options were related or not. This is a quick and easy solution that may solve the problem. If it doesn't, we can make further changes later. image --- .../EnvironmentStrategyExecutionOrder.tsx | 2 +- .../ChangeRequest/Changes/Change/ReleasePlanChange.tsx | 4 ++-- .../ChangeRequest/Changes/Change/SegmentChangeDetails.tsx | 2 +- .../ChangeRequest/Changes/Change/StrategyChange.tsx | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/EnvironmentStrategyExecutionOrder/EnvironmentStrategyExecutionOrder.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/EnvironmentStrategyExecutionOrder/EnvironmentStrategyExecutionOrder.tsx index 1733e428c9..ea1b6de57d 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/EnvironmentStrategyExecutionOrder/EnvironmentStrategyExecutionOrder.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/EnvironmentStrategyExecutionOrder/EnvironmentStrategyExecutionOrder.tsx @@ -109,7 +109,7 @@ export const EnvironmentStrategyExecutionOrder = ({
- Change + View change View diff {actions} diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ReleasePlanChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ReleasePlanChange.tsx index 965aaed6f7..49d4c34daf 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ReleasePlanChange.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ReleasePlanChange.tsx @@ -89,7 +89,7 @@ const StartMilestone: FC<{
- Change + View change View diff {actions} @@ -202,7 +202,7 @@ const AddReleasePlan: FC<{
- Changes + View change View diff {actions} diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx index 519de546dc..d87f17c24a 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx @@ -57,7 +57,7 @@ export const SegmentChangeDetails: FC<{ const actionsWithTabs = ( - Change + View change View diff {actions} diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx index 374c99ac13..47d128ec2f 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx @@ -316,7 +316,7 @@ export const StrategyChange: FC<{ const actionsWithTabs = ( - Change + View change View diff {actions} From 3bb74263922c681bfd4f3806764f56a57f16e697 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Fri, 4 Jul 2025 13:41:19 +0200 Subject: [PATCH 6/8] feat: decouple error impact metrics from logger (#10311) --- src/lib/middleware/request-logger.ts | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/lib/middleware/request-logger.ts b/src/lib/middleware/request-logger.ts index 91468b3ed9..c9c49ac895 100644 --- a/src/lib/middleware/request-logger.ts +++ b/src/lib/middleware/request-logger.ts @@ -8,21 +8,27 @@ import { const requestLogger: (config: IUnleashConfig) => RequestHandler = (config) => { const logger = config.getLogger('HTTP'); - const enable = config.server.enableRequestLogger; + const requestLoggerEnabled = config.server.enableRequestLogger; const impactMetrics = config.flagResolver.impactMetrics; return (req, res, next) => { - if (enable) { - res.on('finish', () => { - const { pathname } = url.parse(req.originalUrl); + const impactMetricsEnabled = + config.flagResolver.isEnabled('impactMetrics'); + + res.on('finish', () => { + if (impactMetricsEnabled && impactMetrics) { if (res.statusCode >= 400 && res.statusCode < 500) { - impactMetrics?.incrementCounter(CLIENT_ERROR_COUNT); + impactMetrics.incrementCounter(CLIENT_ERROR_COUNT); } if (res.statusCode >= 500) { - impactMetrics?.incrementCounter(SERVER_ERROR_COUNT); + impactMetrics.incrementCounter(SERVER_ERROR_COUNT); } + } + + if (requestLoggerEnabled) { + const { pathname } = url.parse(req.originalUrl); logger.info(`${res.statusCode} ${req.method} ${pathname}`); - }); - } + } + }); next(); }; }; From 8e0e9c834e30f727e6f5529790d1223f636d3274 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 4 Jul 2025 13:44:17 +0200 Subject: [PATCH 7/8] chore: Use fixed-width sidebar instead of dynamic modal. (#10315) Uses a fixed-width sidebar component instead of the dynamic sidebar component for the change request sidebar. This fixes a case where the modal would suddenly grow narrower when a change was sent to review (introduced in https://github.com/Unleash/unleash/pull/10307): Before submitting (in main) ![image](https://github.com/user-attachments/assets/a8409cf1-b066-4f97-8e28-cd2470646a9f) After submission (in main) ![image](https://github.com/user-attachments/assets/1735a07f-5792-452f-9a22-2309da9e28fa) Before submitting (on this branch) ![image](https://github.com/user-attachments/assets/4ffff55d-cb8a-4cb6-a22e-54da8182771b) After submission (on this branch) ![image](https://github.com/user-attachments/assets/1569163a-a8d6-4e2c-8239-6e99b9dcfdd0) I don't see any reason why the CR sidebar should be dynamic, so making it fixed width with the solution we already have seems pretty sensible to me. Keeps things consistent and prevents us from solving the same problem multiple times in multiple ways. Yes this change makes the sidebar a little wider, but I think that's fine. It's also closer to what it was previously, I think. Again, we can rethink this if necessary. And of course, the modal still smooshes together when it needs to: image --- .../ChangeRequestSidebar.tsx | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/frontend/src/component/changeRequest/ChangeRequestSidebar/ChangeRequestSidebar.tsx b/frontend/src/component/changeRequest/ChangeRequestSidebar/ChangeRequestSidebar.tsx index dadc379831..0baffc141e 100644 --- a/frontend/src/component/changeRequest/ChangeRequestSidebar/ChangeRequestSidebar.tsx +++ b/frontend/src/component/changeRequest/ChangeRequestSidebar/ChangeRequestSidebar.tsx @@ -1,6 +1,6 @@ import { useState, type VFC } from 'react'; import { Box, Button, styled, Typography } from '@mui/material'; -import { DynamicSidebarModal } from 'component/common/SidebarModal/SidebarModal'; +import { SidebarModal } from 'component/common/SidebarModal/SidebarModal'; import { PageContent } from 'component/common/PageContent/PageContent'; import { PageHeader } from 'component/common/PageHeader/PageHeader'; import CheckCircle from '@mui/icons-material/CheckCircle'; @@ -22,7 +22,6 @@ interface IChangeRequestSidebarProps { const StyledPageContent = styled(PageContent)(({ theme }) => ({ height: '100vh', overflow: 'auto', - maxWidth: 'max(40vw, 1000px)', padding: theme.spacing(6), [theme.breakpoints.down('md')]: { padding: theme.spacing(4, 2), @@ -106,11 +105,7 @@ export const ChangeRequestSidebar: VFC = ({ if (!loading && !data) { return ( - + } @@ -119,16 +114,12 @@ export const ChangeRequestSidebar: VFC = ({ {/* FIXME: empty state */} Close - + ); } return ( - + } @@ -159,6 +150,6 @@ export const ChangeRequestSidebar: VFC = ({ ))} - + ); }; From 37aaf60aa5dbfd71c2a4810ed7822db37db5468b Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 4 Jul 2025 14:18:02 +0200 Subject: [PATCH 8/8] feat(1-3873)/warn you when adding existing values (#10310) Makes it so that the constraint value input gives you an error if you try to add one or more values that **all** exist in the set of values already. E.g. if you have `a` and `b`, and try to add `a`, it'll tell you that "`a` has already been added". Likewise, if you try to add `a,b`, it'll tell you that all these values already exist. However, if at least one of the values does not exist, then it will allow you to submit the values (we already do deduplication before storing anyway). The background for this is that a user was confused thinking that just one specific value didn't get added to their constraints. As it turns out, they'd already added the value previously, so when it didn't show up at the end of the list, they thought it didn't work at all. image image --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../useEditableConstraint.test.tsx | 21 +++++++++++++++++++ .../useEditableConstraint.tsx | 20 +++++++++++++++--- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.test.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.test.tsx index bdc671ea45..ff63dae6c8 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.test.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.test.tsx @@ -154,6 +154,27 @@ describe('validators', () => { ]); }, ); + + test.each(multipleValueOperators)( + 'multi-value operator %s should reject fully duplicate inputs and accept new values', + (operator) => { + const initial: IConstraint = { + contextName: 'context-field', + operator: operator, + values: ['a', 'b'], + }; + + const { result } = renderHook(() => + useEditableConstraint(initial, () => {}), + ); + + checkValidator(result.current.validator, [ + ['a', false], + [['a', 'c'], true], + [['a', 'b'], false], + ]); + }, + ); }); describe('legal values', () => { diff --git a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.tsx b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.tsx index 701f003458..91f9ce0212 100644 --- a/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.tsx +++ b/frontend/src/component/feature/FeatureStrategy/FeatureStrategyConstraints/EditableConstraint/useEditableConstraint/useEditableConstraint.tsx @@ -4,6 +4,7 @@ import type { IConstraint } from 'interfaces/strategy'; import { type EditableConstraint, fromIConstraint, + isMultiValueConstraint, isSingleValueConstraint, toIConstraint, } from './editable-constraint-type.ts'; @@ -16,8 +17,8 @@ import { type ConstraintUpdateAction, } from './constraint-reducer.ts'; import { - type ConstraintValidationResult, constraintValidator, + type ConstraintValidationResult, } from './constraint-validator.ts'; import { getDeletedLegalValues, @@ -76,7 +77,20 @@ export const useEditableConstraint = ( [JSON.stringify(context), localConstraint.contextName], ); - const validator = constraintValidator(localConstraint.operator); + const baseValidator = constraintValidator(localConstraint.operator); + + const validator = (...values: string[]) => { + if ( + isMultiValueConstraint(localConstraint) && + values.every((value) => localConstraint.values.has(value)) + ) { + if (values.length === 1) { + return [false, `${values[0]} is already added.`]; + } + return [false, `All the values are already added`]; + } + return baseValidator(...values); + }; useEffect(() => { if ( @@ -104,7 +118,7 @@ export const useEditableConstraint = ( isSingleValueConstraint(localConstraint) ) { return getInvalidLegalValues( - (value) => validator(value)[0], + (value) => baseValidator(value)[0], contextDefinition.legalValues, ); }