feature: React TypeScript port of the Angular HN PWA + visual regression harness#336
feature: React TypeScript port of the Angular HN PWA + visual regression harness#336devin-ai-integration[bot] wants to merge 1 commit into
Conversation
…on harness Add react-hn (CRA + TypeScript) port of the Angular Hacker News PWA with full visual parity across all 5 feeds, item detail, user profile, settings, the 3 themes, and mobile/desktop. Add visual-tests/ Playwright + pixelmatch harness; all 28 views match the Angular baseline at <0.7% pixel mismatch (threshold 2%). Co-Authored-By: Lukas Burger <lukaskburger@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| points: number; | ||
| user: string; | ||
| time: number; | ||
| time_ago: number; |
There was a problem hiding this comment.
🟡 Story.time_ago typed as number but API returns a string
The Story model at react-hn/src/models/Story.ts:11 declares time_ago: number, but the HN API actually returns a string like "2 hours ago" (confirmed from the fixture data in visual-tests/fixtures/item.json and visual-tests/fixtures/news_p1.json). Notably, the Comment model in the same PR correctly types time_ago: string at react-hn/src/models/Comment.ts:6, making this an internal inconsistency. Because the API response is cast via res.json() as Promise<T> (react-hn/src/services/hackerNewsApi.ts:12), TypeScript trusts the assertion and won't catch the mismatch. This means TypeScript would incorrectly allow number operations on time_ago (e.g., item.time_ago.toFixed()) while preventing valid string operations (e.g., .includes()), undermining type safety.
| time_ago: number; | |
| time_ago: string; |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch on the inconsistency. This is intentional fidelity, though: the port's mandate was to mirror the Angular models 1:1, and the original src/app/shared/models/story.ts declares time_ago: number while comment.ts declares time_ago: string — so this exact inconsistency is inherited verbatim from the source app, not introduced here.
It has no runtime impact: Story.time_ago is only ever rendered as text ({item.time_ago} in FeedItem/ItemDetails) — no numeric operations are performed on it. I've left it as-is to keep the model a faithful copy, but happy to "fix-forward" to string (matching the API/fixture and the Comment model) if we'd rather correct the original's latent bug here.
| @@ -0,0 +1 @@ | |||
| export type FeedType = 'poll' | 'story' | 'job'; | |||
There was a problem hiding this comment.
🟡 FeedType union is missing 'link' and 'ask' values returned by the API
The FeedType type is defined as 'poll' | 'story' | 'job', but the HN API actually returns 'link' for regular stories and 'ask' for Ask HN posts (confirmed from fixtures: news_p1.json contains "type": "link", ask_p1.json contains "type": "ask"). The value 'story' is included but never appears in API responses. Since Story.type is typed as FeedType (react-hn/src/models/Story.ts:12), TypeScript would incorrectly reject valid comparisons like item.type === 'link' as always-false, preventing correct exhaustive type checks. Current code works because it only checks for 'job' and 'poll', but the type is factually wrong about the API contract.
| export type FeedType = 'poll' | 'story' | 'job'; | |
| export type FeedType = 'poll' | 'link' | 'job' | 'ask'; |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct that the API uses link/ask. Same rationale as the time_ago thread: this is a verbatim copy of the Angular original's src/app/shared/models/feed-type.type.ts ('poll' | 'story' | 'job'), kept identical per the 1:1-port requirement.
No runtime impact: the app only ever branches on type === 'job' (job-styling) and type === 'poll' (poll rendering); it never compares against 'story'/'link'/'ask', so the inaccurate union members are never exercised. Glad to widen it to 'poll' | 'link' | 'job' | 'ask' if we'd prefer the type to reflect the real API contract over matching the source app.
| setSpacing: (listSpace: string) => void; | ||
| } | ||
|
|
||
| const darkColorSchemeMedia = window.matchMedia('(prefers-color-scheme: dark)'); |
There was a problem hiding this comment.
🚩 Module-scope window.matchMedia call may break in test environments
At react-hn/src/contexts/SettingsContext.tsx:20, window.matchMedia('(prefers-color-scheme: dark)') is called at module scope (outside any component or function). In the Angular original (src/app/shared/services/settings.service.ts:17), this was a class property evaluated at construction time. The React version runs the moment the module is imported. In CRA's browser runtime this is fine, but Jest + jsdom (CRA's default test runner) does not provide window.matchMedia out of the box. Any test that imports a component using useSettings will fail unless a polyfill is added. Not a production bug, but will block unit testing without workarounds.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed it's a test-environment concern only, not a production issue. This mirrors the Angular original (settings.service.ts evaluates window.matchMedia(...) at construction time); evaluating at module scope is the React equivalent and works in CRA's browser runtime, the production build, and the Playwright visual suite (all green).
There are no Jest/jsdom unit tests in this PR, so nothing is currently broken. If unit tests are added later, the clean fix is a guard (typeof window !== 'undefined' && window.matchMedia ? window.matchMedia('(prefers-color-scheme: dark)') : null) or a matchMedia polyfill in setupTests.ts. Leaving as-is for now to keep parity with the source.
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| points: number; | ||
| user: string; | ||
| time: number; | ||
| time_ago: number; |
There was a problem hiding this comment.
🟡 Story.time_ago typed as number but API returns a string
Story.time_ago is declared as number at react-hn/src/models/Story.ts:11, but the HN API returns a string like "2 hours ago" (confirmed by the PR's own fixture data in visual-tests/fixtures/news_p1.json). Within the same PR, Comment.time_ago is correctly typed as string at react-hn/src/models/Comment.ts:6. While the current rendering code ({item.time_ago} in JSX) works for both types, this incorrect type definition means TypeScript will allow number-only operations (e.g., .toFixed(), arithmetic) on what is actually a string at runtime, which would crash.
| time_ago: number; | |
| time_ago: string; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| @@ -0,0 +1 @@ | |||
| export type FeedType = 'poll' | 'story' | 'job'; | |||
There was a problem hiding this comment.
🟡 FeedType union is missing 'link', the most common item type from the API
FeedType is defined as 'poll' | 'story' | 'job', but the HN API returns type: "link" for most stories (confirmed by every entry in the PR's own fixture files like visual-tests/fixtures/news_p1.json). Since Story.type is typed as FeedType, TypeScript will reject valid comparisons like item.type === 'link' and will incorrectly consider a switch over 'poll' | 'story' | 'job' to be exhaustive, missing the most common case.
| export type FeedType = 'poll' | 'story' | 'job'; | |
| export type FeedType = 'poll' | 'link' | 'story' | 'job'; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| <div | ||
| className="pollBar" | ||
| style={{ | ||
| width: `${(pollResult.points / item.poll_votes_count) * 100}%`, |
There was a problem hiding this comment.
🟡 Division by zero in poll bar width when all poll options have zero points
In ItemDetails.tsx:119, the poll bar width is computed as (pollResult.points / item.poll_votes_count) * 100. The poll_votes_count is the sum of all poll option points (react-hn/src/services/hackerNewsApi.ts:27-35). If every poll option has 0 points, poll_votes_count is 0, and the expression evaluates to 0/0 * 100 = NaN, producing width: "NaN%" — invalid CSS that causes the poll bars to render incorrectly.
| width: `${(pollResult.points / item.poll_votes_count) * 100}%`, | |
| width: `${item.poll_votes_count > 0 ? (pollResult.points / item.poll_votes_count) * 100 : 0}%`, |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| useEffect(() => { | ||
| const handleChange = (event: MediaQueryListEvent) => { | ||
| setTheme(event.matches ? 'night' : 'default'); | ||
| }; | ||
| darkColorSchemeMedia.addEventListener('change', handleChange); | ||
| return () => { | ||
| darkColorSchemeMedia.removeEventListener('change', handleChange); | ||
| }; | ||
| }, [setTheme]); |
There was a problem hiding this comment.
🚩 System color-scheme listener overrides user-saved theme preference
When the OS dark-mode preference changes, the useEffect at react-hn/src/contexts/SettingsContext.tsx:55-63 calls setTheme() which writes the new theme to localStorage. This means if a user explicitly chose 'default' (light) theme and later their OS switches to dark mode, their explicit preference is silently overwritten with 'night'. The Angular version likely had the same behavior, but it's worth considering whether the listener should only apply when no explicit user preference is saved.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| $theme-amoledblack-subtext-color, | ||
| $theme-amoledblack-secondary-color, | ||
| $theme-amoledblack-body-background-color, | ||
| $theme-amoledblack-secondary-color |
There was a problem hiding this comment.
🚩 AMOLED black theme passes a bare color as the $border parameter
In react-hn/src/scss/_themes.scss:238, the amoledblack theme invocation passes $theme-amoledblack-secondary-color (which is rgba(255, 255, 255, 0.6)) as the $border mixin parameter. Other themes pass full border shorthand like 2px solid #b92b27. Since CSS border-bottom: rgba(255, 255, 255, 0.6) has no width or style, borders won't render for the AMOLED theme. This is faithfully ported from the Angular original (src/app/shared/scss/_themes.scss:238) so it's a pre-existing issue, but may be worth fixing while the SCSS is being set up fresh.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
1 participant
Summary
Ports the Angular 9 Hacker News PWA to a React + TypeScript app (
react-hn/) with full visual parity, and adds a Playwright + pixelmatch visual-regression harness (visual-tests/) that proves it. The original Angular app is left untouched.All 28 views pass at <0.7% pixel mismatch (threshold 2%) — every feed (news/newest/show/ask/jobs), pagination, item detail, user profile, settings panel, all 3 themes (default/night/amoledblack), and the error state, each at desktop (1280×800) and mobile (375×812).
React app (
react-hn/)CRA + TypeScript,
react-router-dom@6,sass. Mirrors the Angular architecture 1:1:SettingsService→ ReactSettingsContext(localStorage +prefers-color-schememedia query).ObservableAPI service → Promise-based nativefetch(services/hackerNewsApi.ts), including the poll vote-aggregation logic.CommentPipe→utils/commentFormat.ts._theme_variables,_themes,_media) is copied verbatim so the global.{theme} .wrapper …selectors keep working.Key porting subtlety: replicating Angular's CSS encapsulation
Angular's
ViewEncapsulationscopes each component's element selectors (p,a,ul,h1) to that component's own template. React has no such isolation, so naive global SCSS bleeds across component boundaries. Two bleeds were the main sources of mismatch and are fixed:header h1 { display:inline-block; font-size:16px; font-weight:normal }was hitting the Settings popup<h1>(because<Settings/>rendered inside<header>), shrinking the "Settings" title. Fixed by rendering<Settings/>as a sibling of<header>.item-details'p { margin: 2px 0 }was compressing the comment-body paragraphs rendered viadangerouslySetInnerHTML(Angular keeps them at the UA default1em). Fixed by scoping that rule to.laptop > ponly.Each page's element-level styles are wrapped under a page class (
.feed,.item-page,.user-page,.comment) to emulate encapsulation; the shared.main-contentlives once instyles.scss.Visual-tests harness (
visual-tests/)capture-lib.tsintercepts the live HN API (node-hnapi.herokuapp.com) and serves JSONfixtures/, so both apps render identical data (the live API changes constantly).capture-angular.ts/capture-react.tscapture the matrix inconfig.ts;compare.tsrunspixelmatchand fails any view >2%.screenshots/angular/is the committed baseline;screenshots/react/andscreenshots/diffs/are generated output and git-ignored.Notes
/user/:idendpoint on the downsized Heroku API returns 404, so a syntheticfixtures/user.jsonis used for the user-profile captures (both apps render it identically).npx tsc --noEmitandnpm run buildboth pass clean.Link to Devin session: https://app.devin.ai/sessions/7df008ba60934dcca141fcaf8848b1d8
Requested by: @lburgers
Devin Review
15636d8