Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scroll to Event ID for links to Events #2541

Merged
merged 11 commits into from
Feb 24, 2025
1 change: 1 addition & 0 deletions src/lib/components/event/event-link.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
namespace: link.workflowEvent.namespace,
workflow: link.workflowEvent.workflowId,
run: link.workflowEvent.runId,
eventId: link.workflowEvent?.eventRef?.eventId,
});
</script>

Expand Down
1 change: 1 addition & 0 deletions src/lib/components/event/event-summary-row.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@
class:canceled
class:terminated
class:typedError
data-eventid={event.id}
data-testid="event-summary-row"
on:click|stopPropagation={onLinkClick}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@
namespace: link.workflowEvent.namespace,
workflow: link.workflowEvent.workflowId,
run: link.workflowEvent.runId,
eventId: link.workflowEvent?.eventRef?.eventId,
})}>{link.workflowEvent.workflowId}</Link
>
</div>
Expand Down
20 changes: 18 additions & 2 deletions src/lib/holocene/table/paginated-table/paginated.svelte
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
<script lang="ts">
import { tick } from 'svelte';

import { page } from '$app/stores';

import Button from '$lib/holocene/button.svelte';
Expand Down Expand Up @@ -32,7 +34,8 @@

$: url = $page.url;
$: perPageParam = url.searchParams.get(perPageKey) ?? pageSizeOptions[0];
$: currentPageParam = url.searchParams.get(currentPageKey) ?? '1';
$: currentPageParam = url.searchParams.get(currentPageKey) || '1';
$: hash = $page.url.hash;
$: store = pagination(items, perPageParam, currentPageParam);

// keep the 'page-size' url search param within the supported options
Expand Down Expand Up @@ -83,8 +86,21 @@
});
};

const scrollToHashEvent = async () => {
store.jumpToHashPage(hash);
await tick();
let id = hash.slice(1);
const row = document?.querySelector(`[data-eventid="${id}"]`);
if (row) {
setTimeout(() => {
row?.scrollIntoView({ behavior: 'smooth' });
}, 500);
}
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how I feel about this logic being in a holocene component and enabled by default. Was there a reason you didn't add this to the parent wherever it was needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was on the fence about that too. I could expose jumpToHash to the slot but then you would need to run the scrollToHashEvent function on every row, which doesn't work if the row isn't rendered (i.e. on the same page you are looking at).

I do think we'll want this eventually for other paginated tables but agreed that this isn't ideal in that it's for events only. I added a hashField prop so that it only runs if that's provided and then you can pass whatever data-{hashField} attribute you want to scroll

$: {
if (currentPageParam) store.jumpToPage(currentPageParam);
if (currentPageParam && !hash) store.jumpToPage(currentPageParam);
if (hash && !url.searchParams.get(currentPageKey)) scrollToHashEvent();
if (perPageParam) store.adjustPageSize(perPageParam);
}
</script>
Expand Down
21 changes: 20 additions & 1 deletion src/lib/pages/workflow-history-event.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@
import { page } from '$app/stores';

import EventSummaryTable from '$lib/components/event/event-summary-table.svelte';
import { groupEvents } from '$lib/models/event-groups';
import { fetchAllEvents } from '$lib/services/events-service';
import { eventFilterSort } from '$lib/stores/event-view';
import { fullEventHistory } from '$lib/stores/events';
import { workflowRun } from '$lib/stores/workflow-run';

$: ({ namespace, workflow: workflowId, run: runId } = $page.params);

Expand All @@ -29,9 +32,25 @@
$: fetchEvents(namespace, workflowId, runId);

$: updating = !$fullEventHistory.length;

$: ({ workflow } = $workflowRun);
$: pendingActivities = workflow?.pendingActivities;
$: pendingNexusOperations = workflow?.pendingNexusOperations;

$: ascendingGroups = groupEvents(
$fullEventHistory,
'ascending',
pendingActivities,
pendingNexusOperations,
);
$: groups =
$eventFilterSort === 'ascending'
? ascendingGroups
: [...ascendingGroups].reverse();

$: visibleItems = $fullEventHistory.filter((e) => e.id === $page.params.id);
</script>

<div class="px-8" data-testid="event-summary-table">
<EventSummaryTable items={visibleItems} {updating} openExpanded />
<EventSummaryTable items={visibleItems} {groups} {updating} openExpanded />
</div>
37 changes: 37 additions & 0 deletions src/lib/stores/pagination.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
getStartingIndexForPage,
getTotalPages,
getValidPage,
hasId,
pagination,
perPageFromSearchParameter,
} from './pagination';
Expand Down Expand Up @@ -542,3 +543,39 @@ describe('perPageFromSearchParameter', () => {
expect(perPageFromSearchParameter({} as any)).toBe(100);
});
});

describe('getStartingIndexForPage', () => {
it('should return 0 for the first page', () => {
expect(getStartingIndexForPage(1, 20, oneHundredResolutions)).toBe(0);
});

it('should return the first index of the second page for the something on the second page', () => {
expect(getStartingIndexForPage(2, 20, oneHundredResolutions)).toBe(20);
});

it('should return the first index of the last page for the something out of bounds', () => {
expect(getStartingIndexForPage(100, 20, oneHundredResolutions)).toBe(80);
});

it('should return 0 for the something out of bounds if the total number of items is less than itemsPerPage', () => {
expect(getStartingIndexForPage(3, 101, oneHundredResolutions)).toBe(0);
});

it('should return 0 if given a negative number for the page', () => {
expect(getStartingIndexForPage(-10, 20, oneHundredResolutions)).toBe(0);
});

it('should return 0 if given NaN', () => {
expect(getStartingIndexForPage(NaN, 20, oneHundredResolutions)).toBe(0);
});
});

describe('hash included in pagination store', () => {
it('should return true if object has id', () => {
expect(hasId({ id: '1234', name: 'cats' })).toBe(true);
});

it('should return false if object does not have id', () => {
expect(hasId({ name: 'cats', startedId: 'asdf' })).toBe(false);
});
});
31 changes: 25 additions & 6 deletions src/lib/stores/pagination.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ type PaginationMethods<T> = {
next: () => void;
previous: () => void;
jumpToPage: (x: number | string) => void;
jumpToHashPage: (hash: string) => void;
jumpToIndex: (x: number | string) => void;
findIndex: (fn: (item: T) => boolean) => number;
findPage: (fn: (item: T) => boolean) => number;
Expand Down Expand Up @@ -151,21 +152,23 @@ export const outOfBounds = (
return false;
};

export const hasId = (item: unknown): item is { id: string } => {
return (
typeof item === 'object' && Object.prototype.hasOwnProperty.call(item, 'id')
);
};

/**
* Creates a Svelte store for viewing pages of a larger data set.
*/
export const pagination = <T>(
items: Readonly<T[]> = [],
perPage: number | string = defaultItemsPerPage,
startingIndex: string | number = 0,
currentPage: string | number = 0,
): PaginationStore<T> => {
perPage = perPageFromSearchParameter(perPage);

const start = getNearestStartingIndex(
toNumber(startingIndex),
perPage,
items,
);
const start = getNearestStartingIndex(toNumber(currentPage), perPage, items);

const pageSize = writable(perPage);
const index = writable(start);
Expand Down Expand Up @@ -214,6 +217,21 @@ export const pagination = <T>(
jumpToPage(page);
};

const jumpToHashPage = (hash: string) => {
const hashId = hash?.slice(1);
if (hashId) {
const itemIndex = items.findIndex(
(item: unknown) => hasId(item) && item?.id === hashId,
);
if (itemIndex !== -1) {
const hashPage = getPageForIndex(itemIndex, get(pageSize));
if (hashPage !== currentPage) {
jumpToPage(hashPage);
}
}
}
};

const findIndex = (fn: (item: T) => boolean): number => {
for (let i = 0; i < items.length; i++) {
if (fn(items[i])) return i;
Expand Down Expand Up @@ -284,6 +302,7 @@ export const pagination = <T>(
previous,
jumpToPage,
jumpToIndex,
jumpToHashPage,
findIndex,
findPage,
nextRow,
Expand Down
6 changes: 3 additions & 3 deletions src/lib/utilities/route-for.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ type RouteParameters = {
run: string;
view?: EventView | string;
queryParams?: Record<string, string>;
eventId: string;
eventId?: string;
scheduleId: string;
queue: string;
schedule: string;
Expand All @@ -38,7 +38,7 @@ export type ScheduleParameters = Pick<
>;
export type EventHistoryParameters = Pick<
RouteParameters,
'namespace' | 'workflow' | 'run' | 'view' | 'queryParams'
'namespace' | 'workflow' | 'run' | 'eventId' | 'view' | 'queryParams'
>;
export type EventParameters = Pick<
RouteParameters,
Expand Down Expand Up @@ -164,7 +164,7 @@ export const routeForEventHistory = ({
...parameters
}: EventHistoryParameters): string => {
const eventHistoryPath = `${routeForWorkflow(parameters)}/history`;
return toURL(`${eventHistoryPath}`, queryParams);
return toURL(`${eventHistoryPath}`, queryParams, parameters?.eventId);
};

export const routeForEventHistoryEvent = ({
Expand Down
14 changes: 14 additions & 0 deletions src/lib/utilities/to-url.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ describe('toURL', () => {
expect(toURL('/workflows')).toBe('/workflows');
});

it('should take a string and hash for the URL and return it', () => {
expect(toURL('/workflows', undefined, '123')).toBe('/workflows#123');
});

it('should turn the query params into a query string', () => {
const params = new URLSearchParams({ a: 'hello' });
expect(toURL('/workflows', params)).toBe('/workflows?a=hello');
Expand All @@ -16,4 +20,14 @@ describe('toURL', () => {
const params = { a: 'hello' };
expect(toURL('/workflows', params)).toBe('/workflows?a=hello');
});

it('should turn an object into a query string and include it with a hash', () => {
const params = { a: 'hello' };
expect(toURL('/workflows', params, '123')).toBe('/workflows?a=hello#123');
});

it('should turn the query params into a query string with a hash', () => {
const params = new URLSearchParams({ a: 'hello' });
expect(toURL('/workflows', params, '1')).toBe('/workflows?a=hello#1');
});
});
4 changes: 3 additions & 1 deletion src/lib/utilities/to-url.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
export const toURL = (
url: string,
params?: URLSearchParams | Record<string, string>,
hash?: string,
): string => {
const isURLSearchParams = params instanceof URLSearchParams;
if (params && !isURLSearchParams) params = new URLSearchParams(params);
if (params) return `${url}?${params}`;
if (params) url = `${url}?${params}`;
if (hash) url = `${url}#${hash}`;
return url;
};
Loading