Fix collapsed threads not restoring, and the clock skew correction (#2188)

* Fix collapsed threads not restoring, and the clock skew correction Collapse state was kept as a flat list of `siteID_url_commentID` strings and read back by splitting on `_`. Any underscore in the url, the site id or the comment id made the pieces impossible to tell apart, so a page whose url contains one lost its collapsed threads on every reload, and one page's entries could be read or deleted as another's: `/post` matched everything stored for `/post_2`, and a site id of `blog` matched `blog_ru`. No separator fixes that, since every candidate can occur inside the values, so the ids are now nested under the site and the url instead. Anything stored in the old shape reads as empty: collapsed threads are a view preference, and re-expanding them once is not worth a migration. The e2e suite had been stripping underscores out of its own thread urls to work around this, which left its collapse test unable to fail on the bug it covers. That workaround is gone, and the test now fails without this fix. `serverClientTimeDiff` was written in seconds and added to an epoch in milliseconds, so the correction it exists to apply was a thousandth of the real skew. It is now milliseconds, and named for the unit. A response with no usable `date` used to fall back to a zero timestamp, which already made the "skew" about twenty days and would have made it fifty-five years once the units were right. Nothing is stored now unless the reading is plausible, since `Date.parse` is lenient enough to turn junk into a date and let an absurd value through the branch that parses. The score tooltip reports controversy again when there is any. It has been dead since the vote component was rewritten in 0e4ae6e0, which moved the score into its own component and left the line behind commented out; the value has been passed in and dropped ever since. Unlike the original it stays out of the way when there is no controversy, which the backend sends as an absent field rather than a zero. * Drop the nested frontend dockerignore Docker reads `.dockerignore` from the build context root only, and nothing builds from `frontend/`: every context in the repo is the repository root, apart from the site, which has its own. There is no Dockerfile under `frontend/` any more either. So the file was never consulted, and both lines it carried, `/.vscode/` and `/.idea/`, are already in the root `.dockerignore` verbatim.
2026-08-21 22:12:19 -05:00
parent 4fca268dc6
commit b6975af63c
12 changed files with 220 additions and 37 deletions
@@ -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

@@ -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)
 }

@@ -1,3 +0,0 @@
-/.vscode/
-/.idea/
-
@@ -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;
@@ -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);
+    });
+  });
 });
@@ -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)) {
@@ -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;
 }

 /**
@@ -13,7 +13,6 @@ describe('<CommentVote />', () => {
    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('<CommentVote />', () => {
    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(<CommentVotes id="1" vote={0} votes={3} controversy={0} />);
+
+    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(<CommentVotes id="1" vote={0} votes={3} controversy={undefined} />);
+
+    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(<CommentVotes id="1" vote={0} votes={3} controversy={1.234} />);
+
+    expect(screen.getByTitle('Controversy: 1.23')).toHaveTextContent('3');
+    expect(screen.queryByTitle('Votes score')).not.toBeInTheDocument();
+  });
 });
@@ -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) {
        }}
      >
        <div
-          title={intl.formatMessage(messages.score)}
-          // title={intl.formatMessage(messages.controversy, { value: controversy })}
+          title={
+            controversy
+              ? intl.formatMessage(messages.controversy, { value: controversy.toFixed(2) })
+              : intl.formatMessage(messages.score)
+          }
          className={clsx(styles.votes, {
            [styles.votesNegative]: votes < 0,
            [styles.votesPositive]: votes > 0,
@@ -108,7 +108,7 @@ export class Comment extends Component<CommentProps, State> {
        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;
@@ -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([]);
+  });
+});
@@ -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<string, Record<string, Comment['id'][]>>;
+
+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));
 };