feat: migrate from Angular 9 to React 19 infrastructure#274
feat: migrate from Angular 9 to React 19 infrastructure#274devin-ai-integration[bot] wants to merge 7 commits into
Conversation
- Replace Angular 9 with React 19, Vite 6, TypeScript 5.8 - Add React Router v7 for client-side routing - Add TanStack Query v5 for data fetching (replaces RxJS) - Create API service layer using official HN Firebase API - Port TypeScript interfaces from Angular models - Create SettingsContext (replaces SettingsService) - Port SCSS theme system (default, night, AMOLED black) - Implement all page components (Feed, ItemDetails, User) - Add shared components (Header, Footer, Settings, Loader, ErrorMessage) - Add ESLint 9 + Prettier for code quality - Add GitHub Actions CI (replaces Travis CI) - Update Firebase config for Vite output - Remove all Angular-specific files and dependencies Co-Authored-By: Matthew Guerra <matthew.guerra@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:
|
Co-Authored-By: Matthew Guerra <matthew.guerra@cognition.ai>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: Matthew Guerra <matthew.guerra@cognition.ai>
E2E Test Results — React HN Infrastructure MigrationRan the app locally ( Result: All 8 tests passed. Core Flow Tests
ScreenshotsFeed Page — 30 stories with live dataPagination — page 2,
|
| Default | Night |
|---|---|
Settings Persistence After Reload
Root Redirect → /news/1
Notes
- Tested against live HN Firebase API (
https://hacker-news.firebaseio.com/v0/) - Poll data fetching fix (commit 042629d) was verified at the code level; no active poll items appeared on the front page during testing
- Root div
classNameverified via browser console for theme tests <ol start>attribute verified via browser console for pagination test- Devin session
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Set theme default to 'default' instead of '' to avoid flash of unstyled content on first visit (Devin Review) - Return hasMore flag from fetchFeed based on total IDs count instead of relying on items.length === 30, which breaks when deleted items are filtered out (Devin Review) Co-Authored-By: Matthew Guerra <matthew.guerra@cognition.ai>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The useEffect guard checked settings.theme which is always truthy
('default'), making the system color scheme detection dead code.
Check localStorage.getItem('theme') directly so first-time users
with no saved preference get the system dark/light mode applied.
Co-Authored-By: Matthew Guerra <matthew.guerra@cognition.ai>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…division by zero - Add APPLY_SYSTEM_THEME reducer action that updates state without writing to localStorage, so system dark mode detection stays active across page reloads until the user explicitly picks a theme - Guard poll bar width calculation against division by zero when poll_votes_count is 0 Co-Authored-By: Matthew Guerra <matthew.guerra@cognition.ai>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ar 'second' - Check localStorage inside media query change handler so an explicit user theme choice stops system theme from overriding it mid-session - Fix '1 seconds ago' → '1 second ago' for consistency with other units Co-Authored-By: Matthew Guerra <matthew.guerra@cognition.ai>
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| <span className={styles.time}> {comment.time_ago}</span> | ||
| </div> | ||
| <div className={styles.commentTree}> | ||
| <p className={styles.commentText} dangerouslySetInnerHTML={{ __html: comment.content }} /> |
There was a problem hiding this comment.
🔴 Untrusted Hacker News HTML is injected without sanitization
The React port renders Hacker News comment text with dangerouslySetInnerHTML, and the same pattern is used for poll/story text and user profiles (src/pages/ItemDetailsPage.tsx:136, src/pages/ItemDetailsPage.tsx:149, src/pages/UserPage.tsx:37). Those fields come from remote user-generated content via fetchItemContent/fetchUser, and React does not sanitize dangerouslySetInnerHTML, so unsafe markup such as event handlers or unsafe links returned by the API can execute in this app’s origin. The previous Angular [innerHTML] bindings were sanitized by Angular by default, so this is a security regression introduced by the migration.
Prompt for agents
Sanitize all remote HTML before rendering it in React. The affected sinks are CommentItem in src/pages/ItemDetailsPage.tsx, poll result content, story/item content, and user about text in src/pages/UserPage.tsx. Introduce a shared sanitizer/HTML rendering helper that whitelists the small subset of HTML needed for Hacker News content, and use it consistently anywhere dangerouslySetInnerHTML is currently used. Alternatively, render these fields as escaped text if preserving formatting is not required.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| const items = await Promise.all(ids.map((id) => fetchItem(id))); | ||
|
|
||
| const comments = await Promise.all( | ||
| items | ||
| .filter((item): item is HNItem => item !== null) | ||
| .map(async (item) => { | ||
| const childComments = | ||
| item.kids && item.kids.length > 0 | ||
| ? await fetchComments(item.kids, level + 1) |
There was a problem hiding this comment.
🔴 Item pages block on fetching every descendant comment
Opening any item with comments now waits for fetchComments to recursively fetch every child thread via nested Promise.all calls before the query resolves. For popular Hacker News posts this can fan out into hundreds or thousands of browser requests; a single failed descendant request rejects the whole item query, so the item page remains on the loader for a long time or falls back to “Could not load item comments” even though the story itself loaded. This is a regression from the previous single aggregated item API call, and it is triggered by fetchItemContent awaiting fetchComments(item.kids) before returning the story at src/services/hackernews-api.ts:124-125.
Prompt for agents
The React migration switched item details from the aggregated node-hnapi endpoint to recursively fetching every Hacker News Firebase comment in src/services/hackernews-api.ts. This makes ItemDetailsPage wait for all descendants before rendering and creates unbounded concurrent request fan-out. Rework the item-details data path so the story can render without waiting for the entire comment tree and so comment fetching is bounded/resilient. Possible approaches include using an aggregated API endpoint again, fetching top-level comments first and loading children progressively, limiting concurrency, and handling individual comment fetch failures without rejecting the whole item query.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| ServiceWorkerModule.register('ngsw-worker.js', { | ||
| enabled: environment.production, | ||
| }), |
There was a problem hiding this comment.
🟡 Offline/PWA support was removed without a replacement
The deleted Angular bootstrap registered the service worker in production, and the PR also deletes ngsw-config.json without adding any Vite/React service-worker registration. The repository advertises offline support in README.md:44-46, so production builds will no longer install a service worker or cache the app shell/content for offline use.
Prompt for agents
Restore the production PWA/offline behavior that was provided by ServiceWorkerModule.register and ngsw-config.json. In the React/Vite app, add an equivalent service worker/PWA setup, include the manifest and static assets, and register it only for production builds. Verify that the Firebase-hosted build still loads previously visited pages/assets offline as described in README.md.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| pre { | ||
| white-space: pre-wrap; | ||
| } |
There was a problem hiding this comment.
🟡 Responsive mobile/laptop sections are never hidden
The React pages still render elements with global mobile and laptop classes (src/pages/ItemDetailsPage.tsx:72-94, src/pages/UserPage.tsx:24-28), but the PR deleted the old component SCSS rules that hid .mobile on desktop and .laptop on mobile, and the new global stylesheet only defines pre. As a result, item pages render both the mobile header and the desktop header on all viewport sizes, and user pages show the mobile back header on desktop.
| pre { | |
| white-space: pre-wrap; | |
| } | |
| pre { | |
| white-space: pre-wrap; | |
| } | |
| @media only screen and (max-width: 768px) { | |
| .laptop { | |
| display: none; | |
| } | |
| } | |
| @media only screen and (min-width: 769px) { | |
| .mobile { | |
| display: none; | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| after_success: | ||
| - firebase use default | ||
| - firebase deploy --token $FIREBASE_TOKEN |
There was a problem hiding this comment.
🟡 Replacing Travis CI drops Firebase deployment
The removed Travis job deployed to Firebase after a successful master build using firebase deploy --token $FIREBASE_TOKEN, but the new GitHub Actions workflow only runs typecheck/lint/build/test (.github/workflows/ci.yml:21-33) and has no deployment step. After this PR, pushes to master can pass CI but the Firebase-hosted app will never be updated automatically.
Prompt for agents
If GitHub Actions is replacing Travis for this repository, port the existing Firebase deployment behavior into the new workflow instead of dropping it. Add an authenticated deploy job or step that runs only after successful master builds, using GitHub Actions secrets for the Firebase token/service account and preserving the previous hosting target.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| ))} | ||
| </ol> | ||
| <div className="nav"> | ||
| {listStart !== 1 && ( | ||
| <Link to={`/${feedType}/${pageNum - 1}`} className={styles.prev}> | ||
| ‹ Prev | ||
| </Link> | ||
| )} |
There was a problem hiding this comment.
🟡 Route changes no longer reset scroll position
The old feed component scrolled to the top after each feed fetch (src/app/feeds/feed/feed.component.ts:42-45) and item details did so on initialization (src/app/item-details/item-details.component.ts:32-39). The new pagination links only change the React Router location, and there is no ScrollRestoration/scroll-to-top handler anywhere in the new app, so clicking “More” from the bottom of a feed or opening an item can leave the user at the old scroll offset on the new page.
Prompt for agents
Add scroll restoration for SPA route changes in the React app. A small component mounted under BrowserRouter can call window.scrollTo(0, 0) whenever the pathname/search changes, or the feed pagination/item navigation handlers can explicitly reset scroll. Ensure the behavior matches the old Angular pages, especially feed pagination and item/user detail navigation.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| "dev": "vite", | ||
| "build": "tsc -b && vite build", | ||
| "preview": "vite preview", | ||
| "lint": "eslint .", | ||
| "typecheck": "tsc -b --noEmit", | ||
| "test": "vitest run", | ||
| "test:watch": "vitest" |
There was a problem hiding this comment.
🔴 npm start no longer works despite contributor setup requiring it
CONTRIBUTING.md:14 explicitly tells contributors to run npm start and open localhost:4200, but the rewritten scripts only expose dev, build, preview, etc. Running npm start now fails with npm's “Missing script: start”, so the repository's mandatory setup instructions are broken by this PR.
| "dev": "vite", | |
| "build": "tsc -b && vite build", | |
| "preview": "vite preview", | |
| "lint": "eslint .", | |
| "typecheck": "tsc -b --noEmit", | |
| "test": "vitest run", | |
| "test:watch": "vitest" | |
| "dev": "vite", | |
| "start": "vite", | |
| "build": "tsc -b && vite build", | |
| "preview": "vite preview", | |
| "lint": "eslint .", | |
| "typecheck": "tsc -b --noEmit", | |
| "test": "vitest run", | |
| "test:watch": "vitest" |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| function mapItemToStory(item: HNItem): Story { | ||
| return { | ||
| id: item.id, | ||
| title: item.title ?? '', | ||
| points: item.score ?? 0, | ||
| user: item.by ?? '', | ||
| time: item.time ?? 0, | ||
| time_ago: item.time ? timeAgo(item.time) : '', | ||
| type: item.type === 'job' ? 'job' : item.type === 'poll' ? 'poll' : 'story', | ||
| url: item.url ?? '', | ||
| domain: extractDomain(item.url), | ||
| comments: [], | ||
| comments_count: item.descendants ?? 0, | ||
| poll: [], | ||
| poll_votes_count: 0, | ||
| content: item.text ?? '', | ||
| deleted: item.deleted ?? false, | ||
| dead: item.dead ?? false, | ||
| }; | ||
| } | ||
|
|
||
| export interface FeedResult { | ||
| items: Story[]; | ||
| hasMore: boolean; | ||
| } | ||
|
|
||
| export async function fetchFeed(feedType: string, page: number): Promise<FeedResult> { | ||
| const endpoint = FEED_ENDPOINTS[feedType] ?? 'topstories'; | ||
| const ids = await fetchJSON<number[]>(`${BASE_URL}/${endpoint}.json`); | ||
|
|
||
| const start = (page - 1) * ITEMS_PER_PAGE; | ||
| const end = start + ITEMS_PER_PAGE; |
There was a problem hiding this comment.
🚩 Direct Firebase API changes the data contract from the previous HN API
The old service used https://node-hnapi.herokuapp.com, which returned hydrated feed/item/user objects, while the new service maps raw Firebase REST records into the old app types (src/services/hackernews-api.ts:74-92). This is intentional for the migration and most fields are covered, but behavior differs in subtle ways: deleted/dead stories are not filtered out, time_ago is computed only at fetch time and then cached by React Query, and comments_count depends on Firebase descendants. These are not clear bugs without product expectations, but reviewers should be aware that the API swap is not just a transport change.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
1 participant
Summary
Complete Angular 9 → React 19 infrastructure migration. Replaces the entire Angular frontend with:
https://hacker-news.firebaseio.com/v0/) — replaces defunct Heroku proxyKey fixes from Devin Review:
item.partsfield (commit 042629d)'default'instead of empty string to prevent FOUC (commit 46175fe)hasMoreflag from total IDs count instead ofitems.length === 30(commit 46175fe)Review & Testing Checklist for Human
/news/1and verify 30 stories load with live HN data (titles, points, usernames, time ago, domains)/— verify redirect to/news/1Notes
Link to Devin session: https://app.devin.ai/sessions/a443e4df1fb14856b719f2d703bcb65c
Requested by: @matthewguerra-cog
Devin Review
b6f4137