Skip to content

fix: the boot spinner is invisible in both themes in two apps, and public-board comments show no timestamp - #253

Merged
houko merged 4 commits into
mainfrom
fix/space-theme-assets-and-duplicate-helpers
Sep 19, 2026
Merged

houko merged 4 commits into
mainfrom
fix/space-theme-assets-and-duplicate-helpers

Conversation

@houko

@houko houko commented Sep 18, 2026

Copy link
Copy Markdown
Member

Description

Three live bugs found while checking whether apps/space's duplicated helpers were safe to delete. The deduplication is the small part of this PR; the bugs are the reason to merge it.

1. The boot spinner is invisible in both themes, in two apps. apps/space and apps/admin both had the light/dark ternary inverted, so each served the asset drawn for the opposite background — a near-black glyph on the dark theme, a near-white one on the light theme. In apps/space that is the first screen every visitor to a public board sees (core/lib/instance-provider.tsx).

2. The GitHub mark on the apps/space sign-in page is inverted too. github-dark.svg's only fill is #B0B4BB — a light grey for dark backgrounds — so the light theme got the washed-out grey mark and the dark theme got the black one. apps/admin's copy of the same hook already had it right.

3. Every comment on every public board renders "commented " with no timestamp. timeAgo normalises its argument through a switch and then falls off the end of the function with no return, so it evaluates to undefined for every input. It has exactly one call site: commented {timeAgo(comment.created_at)}.

The prior audit scored the spinner pair at 0.96 similarity and never read the 4-line diff. A similarity score cannot see an inverted ternary — that is the general lesson, and it is also why apps/admin was never compared at all and carried the same bug.

Then the deduplication: eleven helpers and two enums that already existed in @pace/utils and @pace/constants.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Code refactoring

Screenshots and Media (if applicable)

None, and that is the gap in this PR. The three theme fixes are verified from the asset bytes and from the repo's own convention, not from a browser:

  • logo-spinner-dark.gif's palette has exactly two entries, black and (211,211,211). A light grey is the ink you use on a dark background, so the -dark suffix names the theme the asset is for.
  • resolvedTheme === "dark" ? <dark asset> : <light asset> holds at 11 sites across the three apps. The 3 sites that disagree are precisely the ones this PR changes.
  • apps/web/core/components/common/logo-spinner.tsx and apps/admin/core/hooks/oauth/core.tsx are the correct siblings of the two files being fixed.

A visual check of apps/space and apps/admin in both themes, plus the sign-in page with is_github_enabled on, is worth doing before merge. If the convention were ever meant to be "the asset drawn in that colour", then apps/web is the outlier and all three of these changes go the other way — the 11-to-3 split and the palette are why I am confident it is not.

Test Scenarios

  • pnpm turbo run build check:types check:lint check:format — 59/59, exit 0.
  • oxlint on the two apps: apps/space 50 warnings against a declared baseline of 676, apps/admin 24 against 759. Deleting files can only lower these.
  • rg '@/helpers/(date-time|string|issue|state)\.helper' apps/space and rg '@/types/auth' apps/space both return nothing.
  • The renderFormattedDate signature change (string | null → string | undefined) was checked at both call sites rather than assumed: due-date.tsx uses formattedDate ? formattedDate : "No Date" and passes it to a tooltipContent prop already typed string | React.ReactNode | null and optional; issue-properties.tsx renders it bare. Neither compares to null.
  • The auth enums are string enums with identical members, so repointing them is a runtime no-op.

Not verified: anything visual, per the section above. The comment-timestamp fix also changes the copy to "commented 3 days ago" (the shared helper passes addSuffix: true) — worth a glance if you'd rather it read differently.

References

Deliberately left alone, each for a stated reason:

  • getEditorAssetSrc was on the audit's delete-it-as-a-duplicate list. It is not a duplicate: apps/space/helpers/editor.helper.ts builds /api/public/assets/v2/anchor/{anchor}/{assetId}/ from two positional arguments, while packages/utils/src/editor/common.ts builds /api/assets/v2/workspaces/... from an object. Different endpoint, different auth model. Deleting the space copy breaks asset loading in the public editor.
  • authErrorHandler and its 220-line errorCodeMessages table exist three times: packages/utils/src/auth.ts with zero importers anywhere in the monorepo, plus a 450-line copy in apps/web and a 405-line copy in apps/space. The shared one is the dead one. Resolving that is a real decision (which copy is canonical, and do the two apps genuinely need different error copy) rather than a sweep, so it is not in this PR.
  • renderEmoji in apps/space/helpers/emoji.helper.tsx has zero importers. chore: remove 328 unreferenced files — 22% of the tracked tree — after a skeptic pass rescued 62 of the candidates #252 deletes its apps/web twin but leaves this one standing.
  • groupReactions diverges meaningfully between the copies; picking a signature is a decision.
  • timeAgo's sibling helpers getEditorFileHandlers and queryParamGenerator have no counterpart in @pace/utils at all, and authentication.helper.tsx (405 lines) is the bulk of apps/space/helpers and stays.

This PR is independent of the three already open (#250, #251, #252) — no file overlap. #252 deletes apps/space/helpers/{file,common}.helper.ts from the same directory this one prunes, and the two sets are disjoint.

@github-actions

github-actions Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

React Doctor found no new issues. 🎉

Reviewed by React Doctor for commit f932e5b.

Three sites had the light/dark ternary the wrong way round, so each served the asset drawn for the opposite background.

The convention is `resolvedTheme === "dark" ? <dark asset> : <light asset>`, followed at eleven sites across the three apps -- the suffix names the theme the asset is FOR, not the ink it is drawn in. `logo-spinner-dark.gif`'s palette carries a light grey (211,211,211) as its only non-black entry, which settles it: it is the asset for a dark background.

apps/space and apps/admin both had the spinner inverted, so it painted a near-black glyph on the dark theme and a near-white one on the light theme -- invisible either way. In apps/space that is the first screen every visitor to a public board sees, via core/lib/instance-provider.tsx. apps/space also dropped the `object-contain` that apps/web has, so the glyph could stretch.

The third is the GitHub mark on the apps/space sign-in page: `github-dark.svg`'s only fill is `#B0B4BB`, a light grey for dark backgrounds, so serving it on the light theme washed it out while the black mark went out on the dark theme. apps/admin's copy of the same hook already had it right.

None of this is verified in a browser -- it is verified from the asset bytes and from the eleven sites that agree. A visual check of each app in each theme is worth doing before merge.
`timeAgo` in apps/space/helpers/date-time.helper.ts normalises its argument through a switch and then falls off the end of the function with no return, so it evaluates to undefined for every input. Its one call site renders `commented {timeAgo(comment.created_at)}`, which means every comment on every public work-item board has read "commented " with nothing after it.

`calculateTimeAgo` in @pace/utils is the real implementation and is what apps/web uses. The rendered text becomes "commented 3 days ago", since it passes `addSuffix: true`. Empty input behaves as before: the shared helper returns "" where the local one returned undefined, and both render as nothing.
apps/space kept its own copies of eleven helpers that already exist in @pace/utils. Nine are byte-identical; `getDate` differs only in the name it binds the caught error to, and `addSpaceIfCamelCase` differs only in that the shared version guards a null input. Their six importers now take them from the package, which every one of these files already imports.

This costs nothing in payload, which is worth stating because it is the usual objection: apps/space already declares @pace/utils and imports it in thirty files including its vite config, and @pace/utils builds to one bundled module with no `sideEffects` marker, so the whole barrel is unconditionally in the space graph already. Naming more symbols from it adds no module-graph edges. The heavy parts of @pace/utils -- the sanitizer, chroma-js and the markdown chain -- were moved behind subpaths separately, and none of these eleven touches them.

One behaviour change: `renderFormattedDate` returns `string | undefined` rather than `string | null`, and falls back rather than throwing on an unparseable format token. Both call sites were checked; neither compares to null. due-date.tsx uses `formattedDate ? formattedDate : "No Date"` and passes it to a `tooltipContent` prop already typed to accept null and optional, and issue-properties.tsx renders it bare.

apps/space also re-declared `EAuthModes` and `EAuthSteps` byte-identically to @pace/constants, alongside three interfaces duplicating @pace/types that nothing imported -- both of the app's own consumers of `IEmailCheckData` already take it from @pace/types. The file is gone and its four importers point at @pace/constants. These are string enums with identical members, so the swap is a no-op at runtime.
@houko
houko force-pushed the fix/space-theme-assets-and-duplicate-helpers branch from 3a04543 to f932e5b Compare September 19, 2026 12:39
@houko
houko merged commit 3fce768 into main Sep 19, 2026
14 checks passed
@houko
houko deleted the fix/space-theme-assets-and-duplicate-helpers branch September 19, 2026 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant