feat(react): migrate Angular2-HN PWA to React (Vite + TS)#430
feat(react): migrate Angular2-HN PWA to React (Vite + TS)#430devanshi-gpta wants to merge 3 commits into
Conversation
Full feature-parity React port under react-app/ (Angular source retained): - Models, native-fetch hnApi (incl. poll expansion), SettingsContext (localStorage + prefers-color-scheme), formatCommentCount utility - Components: App, Header, Footer, Settings, Feed, Item, ItemDetails, Comment (recursive), User, Loader, ErrorMessage with ported SCSS - Lazy-loaded item/user routes, vite-plugin-pwa (Workbox) service worker - Guarded Google Analytics pageview effect on route change - Vitest + RTL unit tests and Playwright e2e (rewritten from stale Protractor) Co-Authored-By: Devanshi Gupta <devanshi.gupta@cognition.ai>
🤖 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:
|
Runtime E2E test resultsRan the production
Caveat: the public Item detail — recursive comments (Night theme)Pagination — /news/2 numbering starts at 31News feed — default (light) themeDevin session: https://app.devin.ai/sessions/93e6eb012ae04869a7c0091c223b51fe |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @@ -0,0 +1 @@ | |||
| export type FeedType = 'poll' | 'story' | 'job'; | |||
There was a problem hiding this comment.
📝 Info: FeedType union is incomplete — missing 'link' and 'ask' types returned by the HN API
The FeedType type at react-app/src/models/feed-type.type.ts:1 is defined as 'poll' | 'story' | 'job', but the HN API actually returns additional types like 'link' (for regular link stories) and 'ask' (for Ask HN posts). This is copied from the original Angular code at src/app/shared/models/feed-type.type.ts:1. Currently no runtime errors occur because the code only compares against 'job' and 'poll', and TypeScript types are erased at runtime. However, if someone later adds exhaustive type checking (e.g., a switch statement with a never default), the missing variants would cause issues. The e2e test data at react-app/e2e/app.spec.ts:11 and react-app/e2e/app.spec.ts:23 already uses 'ask' and 'link' types.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — informational. This is a verbatim port of the Angular feed-type.type.ts union, kept identical for parity. It's only used nominally (compared against 'job'/'poll'), so the missing 'link'/'ask' variants don't cause runtime issues today. If we later add exhaustive switch handling, expanding the union (and updating the Angular source in lockstep) would be the fix.
| const handleChange = (event: MediaQueryListEvent) => { | ||
| setTheme(event.matches ? 'night' : 'default'); |
There was a problem hiding this comment.
🔍 System color scheme changes always override user's manually selected theme
The handleChange callback at react-app/src/context/SettingsContext.tsx:48-49 calls setTheme unconditionally when the OS color scheme changes, which also persists the new theme to localStorage via react-app/src/context/SettingsContext.tsx:40. This means if a user manually selects 'amoledblack' and then the system switches from light to dark, their choice is permanently replaced with 'night'. This behavior IS explicitly tested at react-app/src/context/SettingsContext.test.tsx:89-95, so it appears intentional. However, it may surprise users who expect their manual theme choice to be sticky. A more user-friendly approach would be to only auto-switch themes when the user hasn't made a manual selection.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is intentional and faithful to the Angular original: SettingsService.handleSystemPreferredColorSchemeChange (src/app/shared/services/settings.service.ts:28-36) also calls setTheme unconditionally on every OS color-scheme change, persisting it. The spec for this migration was to preserve that subscription behavior, and the unit test at SettingsContext.test.tsx locks it in. Making the manual choice "sticky" would be a deliberate behavior change from the source app, so I'm leaving it as-is for parity — happy to change it if you'd prefer the stickier UX.
| async function lazyFetch<T>(url: string, options?: RequestInit): Promise<T> { | ||
| const res = await fetch(url, options); | ||
| return res.json() as Promise<T>; | ||
| } |
There was a problem hiding this comment.
📝 Info: API fetch wrapper does not check HTTP status codes
The lazyFetch function at react-app/src/api/hnApi.ts:7-10 calls fetch and immediately calls res.json() without checking res.ok. The fetch API only rejects on network failures, not on HTTP error statuses (4xx, 5xx). If the API returns a non-2xx response with a non-JSON body, res.json() will throw a parse error, which is caught by useFetch's .catch() handler — so the app won't crash. However, if the API returns a JSON error body (e.g., {"error": "not found"}), it would be silently treated as valid data and could cause confusing rendering. This matches the original Angular service at src/app/shared/services/hackernews-api.service.ts which also doesn't check status codes.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — informational, and as noted this matches the Angular original which also didn't check status codes. A non-2xx non-JSON response rejects in res.json() and is caught by useFetch's .catch, rendering the error state. Adding an explicit if (!res.ok) throw would make error handling more robust/explicit; leaving out of this parity-focused PR but noting as a reasonable follow-up.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, deps); |
There was a problem hiding this comment.
📝 Info: useFetch hook uses raw deps array with eslint-disable — fragile but functional
The useFetch hook at react-app/src/hooks/useFetch.ts:41 passes the caller-supplied deps array directly to useEffect with an eslint-disable-next-line react-hooks/exhaustive-deps comment. This means the fetcher function and errorMessage string are not tracked as dependencies. This works correctly today because all callers include the relevant variables (feedType, pageNum, itemID, userID) in their deps arrays, and those same variables are captured by the inline fetcher closures. However, this pattern is fragile — a future caller could forget to include a dependency, and the eslint rule suppression would hide the issue.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — informational. The deps array is intentional to mirror the callers' route-param dependencies, and every current caller includes the variables its fetcher closes over. Leaving as-is for now; if this hook grows more callers, wrapping fetcher in useCallback at the call sites (and dropping the eslint-disable) would be the cleaner long-term shape.
Co-Authored-By: Devanshi Gupta <devanshi.gupta@cognition.ai>
| const handleChange = (event: MediaQueryListEvent) => { | ||
| setTheme(event.matches ? 'night' : 'default'); |
There was a problem hiding this comment.
🟡 System color-scheme change overrides the user's manually chosen theme
The OS color-scheme listener unconditionally replaces the active theme (setTheme(event.matches ? 'night' : 'default') at react-app/src/context/SettingsContext.tsx:49) even after the user has explicitly picked a different theme, so a user who selected "Black (AMOLED)" loses that choice whenever their OS toggles between light and dark mode.
Impact: A user's explicit theme preference is silently discarded when the operating system's color scheme changes.
Mechanism: the handleChange listener lacks the same guard used during initialization
At react-app/src/context/SettingsContext.tsx:54-56, the initial theme setup correctly checks whether a theme is already saved:
if (!localStorage.getItem('theme')) {
setTheme(media.matches ? 'night' : 'default');
}However, the handleChange listener registered at line 48-49 has no such guard:
const handleChange = (event: MediaQueryListEvent) => {
setTheme(event.matches ? 'night' : 'default');
};Since setTheme also writes to localStorage (react-app/src/context/SettingsContext.tsx:40), the user's saved preference is permanently overwritten.
To fix this, the handleChange should either:
- Only apply if the user hasn't explicitly chosen a theme (e.g., track whether the theme was user-set vs system-set)
- Or at minimum, only override if the current theme is one of the two system-derived themes ('default' or 'night')
Prompt for agents
The handleChange callback in SettingsContext.tsx (lines 48-49) unconditionally overrides the theme when the OS color scheme changes. It should respect the user's explicit theme choice. One approach: only apply the system preference if the current theme is one of the two system-derived values ('default' or 'night'). You could read the current settings state inside the handler (using a ref or the setState updater pattern) and skip the override if the theme is something else like 'amoledblack'. Another approach: introduce a boolean flag (e.g., 'userExplicitlySetTheme') that gets set to true when the user manually changes the theme in Settings, and have handleChange skip the override when that flag is true.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is a faithful port of the Angular original, which has the identical behavior: handleSystemPreferredColorSchemeChange (src/app/shared/services/settings.service.ts:28-36) unconditionally calls setTheme('night'|'default') on any OS color-scheme change, with no guard for a user's manually-chosen theme (e.g. amoledblack). So an OS toggle wiping an AMOLED selection reproduces in the Angular app too. I've kept parity here rather than silently changing behavior. Happy to add a guard (skip the override when the current theme isn't one of the two system-derived values) if you'd like the React port to improve on the original — just confirm and I'll apply it.
|
|
||
| export function SettingsProvider({ children }: { children: ReactNode }) { | ||
| const [settings, setSettings] = useState<Settings>(createInitialSettings); | ||
| const darkColorSchemeMedia = useRef<MediaQueryList | null>(null); |
There was a problem hiding this comment.
📝 Info: Unused ref: darkColorSchemeMedia is assigned but never read
The darkColorSchemeMedia ref (react-app/src/context/SettingsContext.tsx:37) is assigned inside the effect at line 46 but is never read anywhere else in the component. It appears to be leftover scaffolding or intended for future use. It doesn't cause any harm but is dead code that could be removed for clarity.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Removed in a23a856. The darkColorSchemeMedia ref was leftover scaffolding — the effect now uses a local media const and the ref (plus the useRef import) is gone.
| return { | ||
| showSettings: false, | ||
| openLinkInNewTab: openLinkInNewTab ? JSON.parse(openLinkInNewTab) : false, | ||
| theme: localStorage.getItem('theme') || 'default', |
There was a problem hiding this comment.
📝 Info: Initial theme flash: state defaults to 'default' before effect applies system preference
When no theme is saved in localStorage and the system prefers dark mode, createInitialSettings() returns theme: 'default' (line 29). The useEffect then runs and calls setTheme('night') (line 55). This creates a brief render cycle where the 'default' theme class is applied before switching to 'night', potentially causing a visible flash of the light theme. This could be avoided by reading matchMedia synchronously inside createInitialSettings when no localStorage value exists.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in a23a856. createInitialSettings now reads matchMedia('(prefers-color-scheme: dark)') synchronously when no saved theme exists, so the first render already uses night for system-dark users — no flash. The effect still runs setTheme once to persist the system-derived theme to localStorage (matching Angular's initTheme dispatch), but since state already holds the correct value there's no visible transition. Existing unit tests (system-dark defaults to night, light defaults to default + persists) still pass.
| </div> | ||
| <div className="comment-tree"> | ||
| <div hidden={collapse}> | ||
| <p className="comment-text" dangerouslySetInnerHTML={{ __html: comment.content }}></p> |
There was a problem hiding this comment.
🟨 Unsanitized HTML from third-party API rendered via dangerouslySetInnerHTML
Multiple components render HTML content from the third-party HN API (node-hnapi.herokuapp.com) directly into the DOM using dangerouslySetInnerHTML without any sanitization. If the API is compromised or serves malicious content, arbitrary JavaScript could execute in users' browsers. Affected locations: react-app/src/components/Comment.tsx:35, react-app/src/components/ItemDetails.tsx:104, react-app/src/components/ItemDetails.tsx:120, react-app/src/components/User.tsx:43.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged. This mirrors the Angular original's [innerHTML] bindings on the same fields (comment/story/user content), and the content comes pre-sanitized from the node-hnapi proxy. The real difference is that Angular's DomSanitizer scrubs it automatically whereas React's dangerouslySetInnerHTML does not, so a shared DOMPurify helper is the right hardening for the React port. I've flagged this to the repo owner as a follow-up decision rather than adding a dependency unilaterally; will wire up DOMPurify across all four call sites (Comment/ItemDetails x2/User) if they want it in this PR.
Co-Authored-By: Devanshi Gupta <devanshi.gupta@cognition.ai>
1 participant
Summary
Full feature-parity migration of the Angular 9 Hacker News PWA to React 18 + Vite + TypeScript + react-router-dom, living in a new
react-app/directory. The Angular source is left untouched for reference.The port preserves behavior and visuals 1:1: same HN API (
https://node-hnapi.herokuapp.com), samelocalStoragekeys, same theming (default / night / amoledblack), same routes, same SCSS (ported and manually scoped since React has no Angular-style view encapsulation), and code-splitting parity viaReact.lazyfor the item/user routes. Service-worker/PWA support moves from@angular/service-worker(ngsw-config.json) tovite-plugin-pwa(WorkboxgenerateSW, with aNetworkFirstruntime cache for the HN API).Angular → React mapping
shared/models/*.tssrc/models/*.tsshared/services/hackernews-api.service.ts(RxJS)src/api/hnApi.ts(plain module,fetch→ Promises) +src/hooks/useFetch.tsshared/services/settings.service.tssrc/context/SettingsContext.tsx(Context provider)shared/pipes/comment.pipe.tssrc/utils/formatCommentCount.tsapp.component.*src/components/App.tsx+router.tsxcore/header,core/footer,core/settingsHeader.tsx,Footer.tsx,Settings.tsxfeeds/feed,feeds/itemFeed.tsx,Item.tsxitem-details,item-details/commentItemDetails.tsx,Comment.tsx(recursive)userUser.tsxshared/components(loader, error)Loader.tsx,ErrorMessage.tsxapp.routes.tssrc/router.tsx(createBrowserRouter, lazy item/user)ngsw-config.json+app.moduleSW registrationvite-plugin-pwainvite.config.tsPoll expansion is preserved:
fetchItemContentfetches each poll option (id+i+1) in parallel and accumulatespoll_votes_count.Analytics decision (scope step 8)
Kept, guarded.
App.tsxfires a Google Analytics pageview on every route change, but only when GA is actually present, so it is a no-op in dev/test/without the snippet:Test suite (newly established)
The Angular repo had no unit specs and a single stale Protractor e2e asserting an obsolete welcome message. Rather than porting, a fresh suite was created:
npm test) — 34 tests, all passing. CovershnApi(mockedfetch, incl. poll expansion),SettingsContext(localStorage +prefers-color-schemesubscription),formatCommentCount, and rendering ofItem,Comment(recursion + collapse + deleted),Feed(pagination + job header + error),ItemDetails(proportional poll bars),User, andSettings(theme/font/spacing/new-tab toggles).npm run e2e) — 4 tests, all passing. Rewritten from the obsolete Protractor spec to the real UI with API responses mocked via route interception:/redirects to/news/1, feed renders, navigation to an item detail (with comments) and a user profile, and toggling the Night theme flips the wrapper class (div.default→div.night).Verification
npm run build— passes (tsc + vite build; PWAsw.jsgenerated;ItemDetails/Useremitted as separate lazy chunks).npm run lint— passes (0 errors; 1 benignreact-refreshwarning on the Context file).npm test— 34/34 passing.npm run e2e— 4/4 passing.Screenshots (live API)
Link to Devin session: https://app.devin.ai/sessions/93e6eb012ae04869a7c0091c223b51fe
Requested by: @devanshi-gpta
Devin Review