Skip to content

Commit

Permalink
Dedupe unknown query params from feature flag query params. (#6824)
Browse files Browse the repository at this point in the history
Since #6784 we have seen the unusual behavior where query params for
feature flags will be duplicated on the command line.

For example, if we load `localhost:6006/?showFlags=` it will be
rewritten as `localhost:6006/?showFlags=&showFlags=`.

This change fixes this by deduplicating "unknown" flags with known
feature flag query params.
  • Loading branch information
bmd3k committed Jun 5, 2024
1 parent c3fd2b5 commit f1591d3
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 10 deletions.
22 changes: 12 additions & 10 deletions tensorboard/webapp/routes/dashboard_deeplink_provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ limitations under the License.
import {Injectable} from '@angular/core';
import {Store} from '@ngrx/store';
import {combineLatest, Observable} from 'rxjs';
import {map} from 'rxjs/operators';
import {combineLatestWith, map} from 'rxjs/operators';
import {DeepLinkProvider} from '../app_routing/deep_link_provider';
import {SerializableQueryParams} from '../app_routing/types';
import {State} from '../app_state';
Expand Down Expand Up @@ -152,16 +152,18 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider {
return [{key: RUN_FILTER_KEY, value}];
})
),
store
.select(selectors.getUnknownQueryParams)
.pipe(
map((queryParams) =>
Object.entries(queryParams).map(([key, value]) => ({key, value}))
)
),
]).pipe(
map((queryParamList) => {
return queryParamList.flat();
combineLatestWith(store.select(selectors.getUnknownQueryParams)),
map(([queryParamList, unknownQueryParams]) => {
// Filter out any known params from unknownQueryParams.
const knownQueryParamKeys = new Set(
queryParamList.flat().map((qp) => qp.key)
);
const filteredUnknownQueryParams = Object.entries(unknownQueryParams)
.filter(([key]) => !knownQueryParamKeys.has(key))
.map(([key, value]) => ({key, value}));

return [...queryParamList, ...filteredUnknownQueryParams].flat();
})
);
}
Expand Down
24 changes: 24 additions & 0 deletions tensorboard/webapp/routes/dashboard_deeplink_provider_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,30 @@ describe('core deeplink provider', () => {
]);
});

it('dedupes unknown query params with feature flags', () => {
store.overrideSelector(selectors.getOverriddenFeatureFlags, {
enabledExperimentalPlugins: ['foo', 'bar', 'baz'],
showFlags: '',
});
store.overrideSelector(selectors.getUnknownQueryParams, {
unknown1: 'unknown1_value',
unknown2: 'unknown2_value',
// Should be ignored since it is also a feature flag query param.
experimentalPlugin:
'this_is_known_but_has_different_value_for_some_reason',
// Should be ignored since it is also a feature flag query param.
showFlags: 'this_is_also_known_and_also_has_different_value',
});
store.refreshState();

expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([
{key: 'experimentalPlugin', value: 'foo,bar,baz'},
{key: 'showFlags', value: ''},
{key: 'unknown1', value: 'unknown1_value'},
{key: 'unknown2', value: 'unknown2_value'},
]);
});

describe('runs', () => {
describe('color group', () => {
it('does not put state in the URL when user set color group is null', () => {
Expand Down

0 comments on commit f1591d3

Please sign in to comment.