Migrate app from Angular 9 to React 18 + TypeScript (Vite)#435
Migrate app from Angular 9 to React 18 + TypeScript (Vite)#435eashansinha wants to merge 4 commits into
Conversation
Co-Authored-By: Eashan Sinha <eashan.sinha@cognition.ai>
Original prompt from Eashan
|
🤖 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}
| useEffect(() => { | ||
| const darkColorSchemeMedia = window.matchMedia('(prefers-color-scheme: dark)'); | ||
| 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.
📝 Info: System color scheme change overrides user's manually selected theme
In src/contexts/SettingsContext.tsx:37-44, the useEffect listens for OS-level prefers-color-scheme changes and calls setTheme() unconditionally. If a user has manually selected 'amoledblack' (or any theme) via the settings panel, and then their OS switches between light/dark mode, the user's choice will be overwritten with 'night' or 'default'. This matches the old Angular service's behavior, so it's not a regression, but it's a UX issue worth noting — the listener should arguably only apply when no explicit user preference is saved in localStorage.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — this intentionally mirrors the old Angular SettingsService behavior (system scheme changes always call setTheme). Left as-is to keep the migration behavior-preserving; happy to gate it on a saved preference in a follow-up.
| if (story.type === 'poll') { | ||
| const pollResults = await Promise.all( | ||
| story.poll.map((_, i) => fetchPollContent(story.id + i + 1, signal)) | ||
| ); | ||
| story.poll = pollResults; | ||
| story.poll_votes_count = pollResults.reduce((sum, result) => sum + result.points, 0); |
There was a problem hiding this comment.
📝 Info: Poll data type mismatch between API response and TypeScript interface
In fetchItemContent (src/services/hackernews-api.ts:18-23), story.poll is typed as PollResult[] but the actual API response likely contains an array of numeric IDs (not objects with points and content). The code works correctly at runtime because it only uses story.poll.map((_, i) => ...) (ignoring the array values, using only the length/index), then immediately reassigns story.poll = pollResults with actual PollResult objects. This is safe but relies on the type assertion in fetchJSON being inaccurate for the intermediate state.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — correct, only the array length/indices of the raw poll field are used before it's reassigned with real PollResult objects, matching the original Angular logic. The intermediate typing is loose but safe.
Co-Authored-By: Eashan Sinha <eashan.sinha@cognition.ai>
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
There was a problem hiding this comment.
📝 Info: Static manifest removal is consistent with vite-plugin-pwa auto-injection
The incremental change deletes public/manifest.json and removes the <link rel="manifest"> from index.html:35. This is consistent with vite.config.ts:9-31, where VitePWA is configured with an inline manifest block and thus generates manifest.webmanifest and auto-injects the manifest link at build time. Keeping the static reference would have produced a duplicate/conflicting manifest link, so removing it is the correct approach.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>Angular 2 HN</title> | ||
| <base href="/"> |
There was a problem hiding this comment.
🟡 Page logo and icons break when the site is opened directly on a nested URL
The document's root link was removed (<base href="/"> at index.html:6) while the page keeps relative asset paths, so opening the app directly on a nested address like /item/123 makes the loading logo and icons resolve against the wrong folder and fail to load.
Impact: Users who deep-link or refresh on any page other than the home page see a broken loading logo and missing favicons/touch icons.
Why relative URLs break without a base element
This is a single-page app served with index.html as the fallback for every route. The pre-loader markup still uses relative URLs, e.g. assets/images/logo.svg at index.html:55, and the icon/meta links at index.html:14,38-44 are also relative. With <base href="/"> these resolved against the site root. After its removal, on a URL such as /item/123 the browser resolves assets/images/logo.svg to /item/assets/images/logo.svg, which 404s. The Angular version kept <base href="/"> precisely to avoid this. Note the in-app Header uses absolute paths (/assets/images/logo.svg), so only the pre-React loader image and the document icons are affected.
Prompt for agents
The <base href="/"> element was removed from index.html during the Angular-to-Vite migration. Because this is an SPA whose index.html is served as the fallback for deep routes (e.g. /item/123, /user/foo), the remaining relative asset URLs in index.html (the pre-loader logo at 'assets/images/logo.svg', favicon 'favicon.ico', touch icons under 'assets/icons/...', and og/twitter image 'assets/images/logo-loading.png') now resolve relative to the current path instead of the site root, causing 404s on nested routes. Fix by either re-adding <base href="/"> in <head>, or converting these relative asset references to root-absolute paths (leading '/').
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| useEffect(() => { | ||
| const darkColorSchemeMedia = window.matchMedia('(prefers-color-scheme: dark)'); | ||
| 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 dark-mode change overrides and overwrites the user's explicit theme choice
The media-query listener in src/contexts/SettingsContext.tsx:37-44 calls setTheme(event.matches ? 'night' : 'default') on every OS color-scheme change, and setTheme (src/contexts/SettingsContext.tsx:32-35) also writes to localStorage. This means if a user has explicitly selected e.g. amoledblack, an OS theme switch will silently replace it with night/default and clobber their saved preference. This mirrors the original Angular SettingsService behavior (which always subscribed and always called setTheme), so it is a preserved pre-existing quirk rather than a new regression — but since the settings/theme logic was fully rewritten here, it is worth confirming whether this override is intended.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Devin Review found 5 new potential issues.
⚠️ 1 issue in files not directly in the diff
⚠️ Contributor setup guide is now broken and will mislead new contributors (CONTRIBUTING.md:4-10)
The contribution guide is not updated to reflect the new build system (CONTRIBUTING.md:4-10), so contributors following it will be told to install Angular CLI, run ng init, use npm run precache, and use npm run static-serve — none of which exist any more.
Impact: Any new contributor following the documented setup steps will fail immediately.
Outdated instructions that no longer work after the Angular-to-React migration
The PR updated README.md (lines 76–85) with the new Vite/React build instructions (npm start for Vite dev server, npm run build, npm run preview), but CONTRIBUTING.md still references:
- Step 4:
npm install -g angular-cli@latest— Angular CLI is removed from the project - Step 5:
ng init— no longer applicable - Step 7:
localhost:4200— Vite defaults to port 5173 - Step 9:
npm run static-serve— script removed frompackage.json - Step 10:
npm run precacheandnpm run static-serve— both removed; replaced bynpm run preview
| const value = useMemo<SettingsContextValue>( | ||
| () => ({ | ||
| settings, | ||
| setTheme, | ||
| toggleSettings: () => setSettings((prev) => ({ ...prev, showSettings: !prev.showSettings })), | ||
| toggleOpenLinksInNewTab: () => | ||
| setSettings((prev) => { | ||
| const openLinkInNewTab = !prev.openLinkInNewTab; | ||
| localStorage.setItem('openLinkInNewTab', JSON.stringify(openLinkInNewTab)); | ||
| return { ...prev, openLinkInNewTab }; | ||
| }), | ||
| setFont: (titleFontSize: string) => { | ||
| setSettings((prev) => ({ ...prev, titleFontSize })); | ||
| localStorage.setItem('titleFontSize', titleFontSize); | ||
| }, | ||
| setSpacing: (listSpacing: string) => { | ||
| setSettings((prev) => ({ ...prev, listSpacing })); | ||
| localStorage.setItem('listSpacing', listSpacing); | ||
| }, | ||
| }), | ||
| [settings, setTheme] | ||
| ); |
There was a problem hiding this comment.
📝 Info: Context value recreated on every settings change, causing broad re-renders
The useMemo at src/contexts/SettingsContext.tsx:46-67 includes settings in its dependency array. Since toggleSettings, toggleOpenLinksInNewTab, setFont, and setSpacing all use the functional updater form of setSettings and don't actually read settings, they could be wrapped in useCallback independently. As-is, every settings change (even toggling the settings panel) creates a new context value object with new function references, causing all useSettings() consumers to re-render. For this app's scale it's unlikely to matter, but it's worth noting for future optimization.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — agreed the setters could be memoized independently; leaving as-is given the app's small consumer count, can optimize in a follow-up if needed.
| manifest: { | ||
| name: 'Angular 2 HN', | ||
| short_name: 'Angular 2 HN', | ||
| icons: [ | ||
| { src: 'assets/icons/android-chrome-144x144.png', sizes: '144x144', type: 'image/png' }, | ||
| { src: 'assets/icons/android-chrome-192x192.png', sizes: '192x192', type: 'image/png' }, | ||
| { src: 'assets/icons/android-chrome-256x256.png', sizes: '256x256', type: 'image/png' }, | ||
| { src: 'assets/icons/android-chrome-512x512.png', sizes: '512x512', type: 'image/png' }, | ||
| ], | ||
| theme_color: '#b92b27', | ||
| background_color: '#ffffff', | ||
| display: 'standalone', | ||
| orientation: 'portrait', | ||
| start_url: './?utm_source=web_app_manifest', | ||
| }, |
There was a problem hiding this comment.
🔍 Potential duplicate manifest links in production build
The index.html:35 has a hardcoded <link rel="manifest" href="/manifest.json"> pointing to the static file in public/manifest.json. Meanwhile, vite-plugin-pwa with the manifest option in vite.config.ts:12-26 generates its own manifest file and typically injects a <link rel="manifest"> tag during build. This could result in two manifest links in the built HTML. Since both manifests have identical content, this won't cause functional issues, but it means changes to the manifest config in vite.config.ts may not take effect if the browser uses the first link. Consider removing either the static public/manifest.json + the HTML link, or the manifest option from the Vite PWA config.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed — removed the hardcoded <link rel="manifest"> from index.html and deleted the static public/manifest.json; the built HTML now contains only the single manifest.webmanifest link injected by vite-plugin-pwa.
| <div className="subtext">{pollResult.points} points</div> | ||
| <div | ||
| className="pollBar" | ||
| style={{ width: `${(pollResult.points / item.poll_votes_count) * 100}%` }} |
There was a problem hiding this comment.
📝 Info: Poll bar width calculation can produce NaN when total votes are zero
At src/pages/ItemDetails.tsx:117, the poll bar width is computed as (pollResult.points / item.poll_votes_count) * 100. If all poll options have 0 points, poll_votes_count (computed at src/services/hackernews-api.ts:26) will be 0, resulting in division by zero and a NaN% CSS width. This is a pre-existing issue from the Angular code which had the identical calculation. In practice, polls with zero total votes are extremely rare on HN, so the impact is minimal.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — pre-existing behavior carried over from the Angular code; zero-vote polls are extremely rare, so leaving as-is to keep the migration behavior-preserving.
| useEffect(() => { | ||
| const controller = new AbortController(); | ||
| setItems(null); | ||
| setErrorMessage(''); | ||
| fetchFeed(feedType, pageNum, controller.signal) | ||
| .then((stories) => { | ||
| setItems(stories); | ||
| window.scrollTo(0, 0); | ||
| }) | ||
| .catch((error: unknown) => { | ||
| if (!(error instanceof DOMException && error.name === 'AbortError')) { | ||
| setErrorMessage(`Could not load ${feedType} stories.`); | ||
| } | ||
| }); | ||
| return () => controller.abort(); | ||
| }, [feedType, pageNum]); |
There was a problem hiding this comment.
📝 Info: Well-structured abort controller pattern for data fetching
The data fetching in Feed.tsx:17-32, ItemDetails.tsx:21-34, and UserPage.tsx:17-29 all correctly use AbortController with cleanup in the useEffect return, and properly filter out AbortError in the catch handler. This is a significant improvement over the old Angular code which used RxJS subscriptions with a manual cancel token pattern. The abort signal is also correctly threaded through to the fetchJSON helper at src/services/hackernews-api.ts:8.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-Authored-By: Eashan Sinha <eashan.sinha@cognition.ai>
Co-Authored-By: Eashan Sinha <eashan.sinha@cognition.ai>
|
Addressed the CONTRIBUTING.md issue flagged by Devin Review — setup steps now reflect the Vite workflow ( |
End-to-end test results (production build,
|
| Desktop feed | Item details with comments |
|---|---|
| 🟢 Night theme persisted after reload | Mobile feed layout |
|---|---|
More evidence (collapse toggle, settings panel, mobile item, user error UI)
Comment collapsed to [+] |
Settings panel (Night selected) |
|---|---|
| Mobile item details | |
|---|---|
PWA shell evidence:
sw.js: 200
manifest.webmanifest: 200
SW registrations: http://localhost:4173/sw.js (CDP Runtime.evaluate)
Devin session: https://app.devin.ai/sessions/b26082ca0cf14af2824526f0bd24933f
1 participant
Summary
Full rewrite of the app layer from Angular 9 to React 18 + TypeScript, replacing the Angular CLI toolchain with Vite. Behavior, routes, styling, and theming are preserved 1:1.
Tooling (WS0):
angular.json,karma.conf.js,tslint.json,ngsw-config.json,e2e/(Protractor), rxjs/zone.js are gone. New:vite.config.ts(withvite-plugin-pwareplacing@angular/service-worker— WorkboxgenerateSW, same web manifest), Vitest + React Testing Library, ESLint flat config,tsc -btypecheck innpm run build. Firebase hosting config (firebase.json,.firebaserc) untouched;dist/output location unchanged.src/assets+manifest.jsonmoved topublic/.Models (WS1):
src/models/— classes converted to interfaces; added missingcontentfield onStory(was used untyped by templates).Services (WS2):
HackerNewsAPIService's RxJSlazyFetch/cancelToken→ plain async fns insrc/services/hackernews-api.tstaking anAbortSignal(fetchFeed,fetchItemContent,fetchPollContent,fetchUser). Poll vote aggregation now usesPromise.allinstead of nested subscriptions.SettingsService→SettingsContextprovider +useSettings()hook, preservinglocalStoragepersistence and theprefers-color-schememedia-query subscription (system changes switch theme unless overridden).Routing (WS3):
react-router-dominApp.tsx—/→/news/1,/:feed/:pagefor the 5 feeds (feedType passed as prop),item/:id+user/:idviaReact.lazy/Suspense(mirrors Angular lazy modules). GA pageview tracking ported as auseLocationeffect.Components (WS4/5): each
@Component→ function component;ngOnInit/subscriptions →useEffectwithAbortControllercleanup;CommentPipe→commentLabel()helper;[innerHTML]→dangerouslySetInnerHTML; recursiveCommentItemkeeps per-node collapse state.Styling note (deviation from the plan): component SCSS was NOT converted to CSS Modules. The shared theme engine (
_themes.scssmixin) targets literal class names (.subtext,.meta,.loader, …) across components, which hashed CSS Module names would break. Instead, each component's SCSS is nested under a component root class (.hn-item,.feed-page,.comment-item, …) to emulate Angular's view encapsulation while keepingsrc/styles/_themes.scssglobal and intact (converted to@use/math.divfor modern Sass).Verified locally:
npm run build,npm test(5 tests),npm run lintall pass; feeds/item/user pages, pagination, settings (theme/font/spacing persistence), and PWA service worker generation work.Link to Devin session: https://app.devin.ai/sessions/b26082ca0cf14af2824526f0bd24933f
Requested by: @eashansinha
Devin Review
1ed8695