diff --git a/e2e/README.md b/e2e/README.md index 19c6dc6b..3c39530a 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -69,7 +69,7 @@ Three settings exist for the tests rather than for realism, and each is there fo Each test gets its own comment thread from a query string on the demo page, since the demo page passes `window.location.href` as `remark_config.url` and remark42 keys comments by it. A per-run id keeps threads apart from those an earlier run left behind. -The thread URL carries no underscores on purpose: collapse persistence stores its localStorage keys as `siteID_url_commentID` and splits them on `_`, so an underscore anywhere in the page URL makes the entry unreadable on the next load. +Thread URLs deliberately keep the underscores a test name carries, since collapse persistence keys off the page url and a url containing an underscore is the case worth covering. ## Browsers diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index c02f161e..9a20132d 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -372,11 +372,10 @@ func threadURL(t *testing.T) string { // default one. every thread url goes through here so the rewriting below cannot be missed func threadURLOn(t *testing.T, base string) string { t.Helper() - // no underscores: collapse persistence stores its localStorage keys as - // "siteID_url_commentID" and splits them on "_" (store/thread/utils.ts), so an underscore - // anywhere in the page URL makes the entry unreadable on the next load. subtest names - // also carry a "/", which has no business in a query value - name := strings.NewReplacer("_", "-", "/", "-").Replace(strings.ToLower(t.Name())) + // underscores are left in on purpose: collapse persistence keys off the page url, and a + // url carrying the separator its storage used to join on is the case that broke it. only + // the "/" of a subtest name is dropped, having no business in a query value + name := strings.ReplaceAll(strings.ToLower(t.Name()), "/", "-") return fmt.Sprintf("%s/web/?e2e=%s-%s", base, name, runID) } diff --git a/frontend/.dockerignore b/frontend/.dockerignore deleted file mode 100644 index bfa867bf..00000000 --- a/frontend/.dockerignore +++ /dev/null @@ -1,3 +0,0 @@ -/.vscode/ -/.idea/ - diff --git a/frontend/apps/remark42/app/common/constants.ts b/frontend/apps/remark42/app/common/constants.ts index 09ef39fe..dbe2b2e0 100644 --- a/frontend/apps/remark42/app/common/constants.ts +++ b/frontend/apps/remark42/app/common/constants.ts @@ -48,3 +48,6 @@ export const IS_THIRD_PARTY: boolean = (() => { return true; } })(); + +/** clock difference against the server beyond which the reading is treated as unusable */ +export const MAX_CLOCK_SKEW_MS = 24 * 60 * 60 * 1000; diff --git a/frontend/apps/remark42/app/common/fetcher.test.ts b/frontend/apps/remark42/app/common/fetcher.test.ts index 08c8e54a..bb82acba 100644 --- a/frontend/apps/remark42/app/common/fetcher.test.ts +++ b/frontend/apps/remark42/app/common/fetcher.test.ts @@ -14,6 +14,7 @@ import { AUTH_COOKIE_TTL_SECONDS, } from './fetcher'; import * as cookies from './cookies'; +import { StaticStore } from './static-store'; type FetchImplementationProps = { status?: number; @@ -357,4 +358,45 @@ describe('fetcher', () => { await expect(apiFetcher.get(apiUri)).rejects.toEqual(new RequestError('Something went wrong.', 0)); }); }); + + describe('server clock skew', () => { + beforeEach(() => { + delete StaticStore.serverClientTimeDiffMs; + }); + + it('records the skew in milliseconds', async () => { + // consumers add this to an epoch in milliseconds, so seconds would be out by 1000x and + // the correction it exists to apply would effectively not happen + mockFetch({ headers: { date: new Date(Date.now() - 60_000).toUTCString() } }); + await apiFetcher.get('/anything'); + + expect(StaticStore.serverClientTimeDiffMs).toBeGreaterThan(55_000); + expect(StaticStore.serverClientTimeDiffMs).toBeLessThan(65_000); + }); + + it('leaves the skew alone when the response carries no usable date', async () => { + // an unparsable header used to fall back to a zero timestamp, which makes the "skew" + // the whole epoch and pushes anything derived from it decades into the future + StaticStore.serverClientTimeDiffMs = 1234; + + mockFetch({ headers: {} }); + await apiFetcher.get('/anything'); + expect(StaticStore.serverClientTimeDiffMs).toBe(1234); + + mockFetch({ headers: { date: 'not a date' } }); + await apiFetcher.get('/anything'); + expect(StaticStore.serverClientTimeDiffMs).toBe(1234); + }); + + it('ignores a skew too large to be a clock difference', async () => { + // Date.parse is lenient: '0' is a valid date to it, so an implausible reading arrives + // through the branch that parses rather than the one that rejects + StaticStore.serverClientTimeDiffMs = 1234; + + mockFetch({ headers: { date: '0' } }); + await apiFetcher.get('/anything'); + + expect(StaticStore.serverClientTimeDiffMs).toBe(1234); + }); + }); }); diff --git a/frontend/apps/remark42/app/common/fetcher.ts b/frontend/apps/remark42/app/common/fetcher.ts index 6cdfc582..09aa21ed 100644 --- a/frontend/apps/remark42/app/common/fetcher.ts +++ b/frontend/apps/remark42/app/common/fetcher.ts @@ -3,7 +3,7 @@ import { errorMessages, RequestError } from 'utils/errorUtils'; import { siteId } from './settings'; import { getCookie, setAuthCookie, clearAuthCookie } from './cookies'; import { StaticStore } from './static-store'; -import { BASE_URL, API_BASE } from './constants'; +import { BASE_URL, API_BASE, MAX_CLOCK_SKEW_MS } from './constants'; /** Header name for JWT token */ export const JWT_HEADER = 'X-JWT'; @@ -93,10 +93,17 @@ const createFetcher = (baseUrl: string = ''): Methods => { try { const res = await fetch(url, { ...params, headers }); - // TODO: it should be clarified when frontend gets this header and what could be in it to simplify this logic and cover by tests - const date = (res.headers.has('date') && res.headers.get('date')) || ''; - const timestamp = isNaN(Date.parse(date)) ? 0 : Date.parse(date); - StaticStore.serverClientTimeDiff = (new Date().getTime() - timestamp) / 1000; + // milliseconds, because every consumer adds it to an epoch in milliseconds. + // + // an implausible result is dropped rather than stored. the previous code fell back to a + // zero timestamp on a missing header, which made the "skew" the whole epoch, and + // Date.parse is lenient enough to turn junk into a date of its own accord, so trusting + // whatever comes back would keep a deadline computed from it open indefinitely + const timestamp = Date.parse(res.headers.get('date') || ''); + const diff = new Date().getTime() - timestamp; + if (!isNaN(diff) && Math.abs(diff) < MAX_CLOCK_SKEW_MS) { + StaticStore.serverClientTimeDiffMs = diff; + } // backend could update jwt in any time. so, we should handle it if (res.headers.has(JWT_HEADER)) { diff --git a/frontend/apps/remark42/app/common/static-store.ts b/frontend/apps/remark42/app/common/static-store.ts index 15d74c26..2486a026 100644 --- a/frontend/apps/remark42/app/common/static-store.ts +++ b/frontend/apps/remark42/app/common/static-store.ts @@ -2,8 +2,9 @@ import type { Config } from './types'; interface StaticStoreType { config: Config; - /** used in fetcher, fer example to set comment edit timeout */ - serverClientTimeDiff?: number; + /** how far the client clock is ahead of the server, in milliseconds. set by the fetcher, + * read wherever a server timestamp meets the local clock, such as the comment edit deadline */ + serverClientTimeDiffMs?: number; } /** diff --git a/frontend/apps/remark42/app/components/comment/comment-votes.spec.tsx b/frontend/apps/remark42/app/components/comment/comment-votes.spec.tsx index 1d45007e..c2c124b8 100644 --- a/frontend/apps/remark42/app/components/comment/comment-votes.spec.tsx +++ b/frontend/apps/remark42/app/components/comment/comment-votes.spec.tsx @@ -13,7 +13,6 @@ describe('', () => { expect(screen.getByTitle('Vote up')).toBeVisible(); expect(screen.getByTitle('Vote down')).toBeVisible(); expect(screen.getByTitle('Votes score')).toBeVisible(); - // expect(screen.getByTitle('Votes score')).toHaveAttribute('title', '0.00'); }); it('should render vote component with positive score', () => { @@ -104,4 +103,27 @@ describe('', () => { expect(screen.getByTitle('Vote up')).toBeVisible(); expect(screen.getByTitle('Vote up')).not.toBeDisabled(); }); + + it('shows the vote count as the tooltip when there is no controversy', () => { + render(); + + expect(screen.getByTitle('Votes score')).toHaveTextContent('3'); + expect(screen.queryByTitle(/Controversy/)).not.toBeInTheDocument(); + }); + + // what actually arrives: the backend marks the field omitempty, so an uncontroversial + // comment carries no controversy at all rather than a zero + it('shows the vote count as the tooltip when controversy is absent', () => { + render(); + + expect(screen.getByTitle('Votes score')).toHaveTextContent('3'); + expect(screen.queryByTitle(/Controversy/)).not.toBeInTheDocument(); + }); + + it('shows the controversy as the tooltip when there is some', () => { + render(); + + expect(screen.getByTitle('Controversy: 1.23')).toHaveTextContent('3'); + expect(screen.queryByTitle('Votes score')).not.toBeInTheDocument(); + }); }); diff --git a/frontend/apps/remark42/app/components/comment/comment-votes.tsx b/frontend/apps/remark42/app/components/comment/comment-votes.tsx index 038d4fe1..9efe7a37 100644 --- a/frontend/apps/remark42/app/components/comment/comment-votes.tsx +++ b/frontend/apps/remark42/app/components/comment/comment-votes.tsx @@ -21,7 +21,7 @@ type Props = { disabled?: boolean; }; -export function CommentVotes({ id, votes, vote, disabled }: Props) { +export function CommentVotes({ id, votes, vote, controversy, disabled }: Props) { const intl = useIntl(); const dispatch = useDispatch(); const [loadingState, setLoadingState] = useState<{ vote: number; votes: number } | null>(null); @@ -72,8 +72,11 @@ export function CommentVotes({ id, votes, vote, disabled }: Props) { }} >
0, diff --git a/frontend/apps/remark42/app/components/comment/comment.tsx b/frontend/apps/remark42/app/components/comment/comment.tsx index 33ed2fcd..b159c50d 100644 --- a/frontend/apps/remark42/app/components/comment/comment.tsx +++ b/frontend/apps/remark42/app/components/comment/comment.tsx @@ -108,7 +108,7 @@ export class Comment extends Component { newState.editDeadline = Infinity; } else { const editDuration = StaticStore.config.edit_duration; - const timeDiff = StaticStore.serverClientTimeDiff || 0; + const timeDiff = StaticStore.serverClientTimeDiffMs || 0; const editDeadline = new Date(props.data.time).getTime() + timeDiff + editDuration * 1000; newState.editDeadline = editDeadline > Date.now() ? editDeadline : undefined; diff --git a/frontend/apps/remark42/app/store/thread/utils.test.ts b/frontend/apps/remark42/app/store/thread/utils.test.ts new file mode 100644 index 00000000..3ed032a1 --- /dev/null +++ b/frontend/apps/remark42/app/store/thread/utils.test.ts @@ -0,0 +1,82 @@ +import { LS_COLLAPSE_KEY } from 'common/constants'; +import { siteId, url } from 'common/settings'; + +import { getCollapsedComments, saveCollapsedComments } from './utils'; + +jest.mock('common/settings', () => ({ siteId: 'remark', url: 'https://example.com/a_b/' })); + +function stored(): unknown { + return JSON.parse(localStorage.getItem(LS_COLLAPSE_KEY) as string); +} + +describe('collapsed comments storage', () => { + beforeEach(() => { + localStorage.clear(); + }); + + it('restores what it saved for the current page', () => { + saveCollapsedComments(siteId, url, ['c1', 'c2']); + + expect(getCollapsedComments()).toEqual(['c1', 'c2']); + }); + + it('restores ids containing an underscore', () => { + saveCollapsedComments(siteId, url, ['id_with_underscores']); + + expect(getCollapsedComments()).toEqual(['id_with_underscores']); + }); + + it('leaves another page of the same site untouched', () => { + saveCollapsedComments(siteId, 'https://example.com/other/', ['other']); + saveCollapsedComments(siteId, url, ['mine']); + + expect(getCollapsedComments()).toEqual(['mine']); + expect(stored()).toMatchObject({ remark: { 'https://example.com/other/': ['other'] } }); + }); + + // the shape these two exercise is what a single joined key cannot represent: the separator + // occurs inside the values, so the pieces cannot be told apart again + it('keeps a url that extends the current one with the separator', () => { + saveCollapsedComments(siteId, `${url}_extra/`, ['deeper']); + saveCollapsedComments(siteId, url, ['mine']); + + expect(getCollapsedComments()).toEqual(['mine']); + expect(stored()).toMatchObject({ remark: { [`${url}_extra/`]: ['deeper'] } }); + }); + + it('does not confuse a site and url pair with one split differently', () => { + saveCollapsedComments('remark_https://example.com', '/a_b/', ['elsewhere']); + saveCollapsedComments(siteId, url, ['mine']); + + expect(getCollapsedComments()).toEqual(['mine']); + expect(stored()).toMatchObject({ 'remark_https://example.com': { '/a_b/': ['elsewhere'] } }); + }); + + it('forgets the page once its last thread is expanded', () => { + saveCollapsedComments(siteId, 'https://example.com/other/', ['other']); + saveCollapsedComments(siteId, url, ['mine']); + saveCollapsedComments(siteId, url, []); + + expect(getCollapsedComments()).toEqual([]); + expect(stored()).toEqual({ remark: { 'https://example.com/other/': ['other'] } }); + }); + + it('forgets the site once its last page is expanded', () => { + saveCollapsedComments(siteId, url, ['mine']); + saveCollapsedComments(siteId, url, []); + + expect(stored()).toEqual({}); + }); + + it('reads as empty when the stored value is of another shape', () => { + localStorage.setItem(LS_COLLAPSE_KEY, JSON.stringify(['remark_https://example.com/a_b/_c1'])); + + expect(getCollapsedComments()).toEqual([]); + }); + + it('reads as empty when the entry for the page is not a list of ids', () => { + localStorage.setItem(LS_COLLAPSE_KEY, JSON.stringify({ [siteId]: { [url]: 'c1' } })); + + expect(getCollapsedComments()).toEqual([]); + }); +}); diff --git a/frontend/apps/remark42/app/store/thread/utils.ts b/frontend/apps/remark42/app/store/thread/utils.ts index 1d67a85f..b8bfe4c4 100644 --- a/frontend/apps/remark42/app/store/thread/utils.ts +++ b/frontend/apps/remark42/app/store/thread/utils.ts @@ -3,30 +3,57 @@ import { LS_COLLAPSE_KEY } from 'common/constants'; import { setItem as localStorageSetItem, getItem as localStorageGetItem } from 'common/local-storage'; import type { Comment } from 'common/types'; -function getFromLocalStorage(): string[] { - return JSON.parse(localStorageGetItem(LS_COLLAPSE_KEY) || '[]'); +/** + * collapsed comment ids, keyed by site and then by page url. + * + * nested rather than joined into one string: any separator can also occur inside a site id or + * a url, and then "site_url_id" cannot be taken apart again. it is what made a page whose url + * contains an underscore lose its collapsed threads on reload, and what let one page's entries + * be read or deleted as another's + */ +type CollapsedComments = Record>; + +function getFromLocalStorage(): CollapsedComments { + const stored: unknown = JSON.parse(localStorageGetItem(LS_COLLAPSE_KEY) || '{}'); + + // anything of another shape, including the flat list this used to keep, reads as empty: + // collapsed threads are a view preference, so re-expanding them once costs the reader + // nothing worth a migration + if (typeof stored !== 'object' || stored === null || Array.isArray(stored)) { + return {}; + } + return stored as CollapsedComments; } /** - * returns list of serialized comments of type "site-id_url_comment-id" + * returns the ids of the collapsed comments on the current page */ -export const getCollapsedComments = (): string[] => - getFromLocalStorage().reduce((acc: string[], v: string) => { - const components = v.split('_'); - if (components[0] === siteId && components[1] === url) { - acc.push(components[2]); - } - return acc; - }, []); +export const getCollapsedComments = (): Comment['id'][] => { + const ids = getFromLocalStorage()[siteId]?.[url]; + + // the check above covers the top level only, and a leaf of another type would reach the + // reducer, which reduces over it + return Array.isArray(ids) ? ids : []; +}; /** * @param siteId site id * @param url url of the page with comments - * @param info list of string of type "site-id_url_comment-id + * @param info ids of the comments collapsed on that page */ export const saveCollapsedComments = (siteId: string, url: string, info: Comment['id'][]): void => { - const data = info.map((i) => `${siteId}_${url}_${i}`); - const notForThisPost = getFromLocalStorage().filter((entry) => entry.indexOf(`${siteId}_${url}`) === -1); - const all = new Set([...notForThisPost, ...data]); - localStorageSetItem(LS_COLLAPSE_KEY, JSON.stringify([...all])); + const stored = getFromLocalStorage(); + const site = { ...stored[siteId], [url]: info }; + const next = { ...stored, [siteId]: site }; + + // an empty list is dropped rather than stored: every page a reader collapses and expands + // again would otherwise keep an entry of its own for good + if (info.length === 0) { + delete site[url]; + } + if (Object.keys(site).length === 0) { + delete next[siteId]; + } + + localStorageSetItem(LS_COLLAPSE_KEY, JSON.stringify(next)); };