Repository navigation
fix: the boot spinner is invisible in both themes in two apps, and public-board comments show no timestamp - #253
Merged
Conversation
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
2 tasks done
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
force-pushed
the
fix/space-theme-assets-and-duplicate-helpers
branch
from
September 19, 2026 12:39
3a04543 to
f932e5b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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/spaceandapps/adminboth 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. Inapps/spacethat is the first screen every visitor to a public board sees (core/lib/instance-provider.tsx).2. The GitHub mark on the
apps/spacesign-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.
timeAgonormalises its argument through aswitchand then falls off the end of the function with noreturn, so it evaluates toundefinedfor 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/adminwas never compared at all and carried the same bug.Then the deduplication: eleven helpers and two enums that already existed in
@pace/utilsand@pace/constants.Type of Change
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-darksuffix 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.tsxandapps/admin/core/hooks/oauth/core.tsxare the correct siblings of the two files being fixed.A visual check of
apps/spaceandapps/adminin both themes, plus the sign-in page withis_github_enabledon, is worth doing before merge. If the convention were ever meant to be "the asset drawn in that colour", thenapps/webis 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.oxlinton the two apps:apps/space50 warnings against a declared baseline of 676,apps/admin24 against 759. Deleting files can only lower these.rg '@/helpers/(date-time|string|issue|state)\.helper' apps/spaceandrg '@/types/auth' apps/spaceboth return nothing.renderFormattedDatesignature change (string | null→string | undefined) was checked at both call sites rather than assumed:due-date.tsxusesformattedDate ? formattedDate : "No Date"and passes it to atooltipContentprop already typedstring | React.ReactNode | nulland optional;issue-properties.tsxrenders it bare. Neither compares tonull.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:
getEditorAssetSrcwas on the audit's delete-it-as-a-duplicate list. It is not a duplicate:apps/space/helpers/editor.helper.tsbuilds/api/public/assets/v2/anchor/{anchor}/{assetId}/from two positional arguments, whilepackages/utils/src/editor/common.tsbuilds/api/assets/v2/workspaces/...from an object. Different endpoint, different auth model. Deleting the space copy breaks asset loading in the public editor.authErrorHandlerand its 220-lineerrorCodeMessagestable exist three times:packages/utils/src/auth.tswith zero importers anywhere in the monorepo, plus a 450-line copy inapps/weband a 405-line copy inapps/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.renderEmojiinapps/space/helpers/emoji.helper.tsxhas zero importers. chore: remove 328 unreferenced files — 22% of the tracked tree — after a skeptic pass rescued 62 of the candidates #252 deletes itsapps/webtwin but leaves this one standing.groupReactionsdiverges meaningfully between the copies; picking a signature is a decision.timeAgo's sibling helpersgetEditorFileHandlersandqueryParamGeneratorhave no counterpart in@pace/utilsat all, andauthentication.helper.tsx(405 lines) is the bulk ofapps/space/helpersand stays.This PR is independent of the three already open (#250, #251, #252) — no file overlap. #252 deletes
apps/space/helpers/{file,common}.helper.tsfrom the same directory this one prunes, and the two sets are disjoint.