Skip to content

Item details and comment tree (migration 5/8) - #693

Open
charityquinn-cognition wants to merge 4 commits into
devin/react-migration-4-feedsfrom
devin/react-migration-5-item-details
Open

Item details and comment tree (migration 5/8)#693
charityquinn-cognition wants to merge 4 commits into
devin/react-migration-4-feedsfrom
devin/react-migration-5-item-details

Conversation

@charityquinn-cognition

@charityquinn-cognition charityquinn-cognition commented Aug 25, 2026

Copy link
Copy Markdown

Summary

Stacked on #692. Ports item-details and the recursive comment component, and adds /item/:id.

  • Item loads via fetchItemContent (same ignore-flag staleness guard as the feed), with the original loader/error states and message Could not load item comments.
  • Both the mobile and laptop headers are reproduced, including poll results with the percentage bars and the story content HTML.
  • Location.back()navigate(-1).
  • Comment recurses through comment.comments, keeping collapse/expand as the hidden attribute (so collapsed subtrees stay mounted, matching Angular) and keeping the deleted-comment markup. Child arrays are guarded (comment.comments ?? []) because the API omits the key on leaves — and item.comments is guarded the same way, since *ngFor over a missing array rendered nothing where .map would throw.
  • API-provided HTML (item.content, comment.content, poll option text) goes through a new sanitizeHtml() wrapper over DOMPurify:
export function sanitizeHtml(html: string): string {
    return DOMPurify.sanitize(html);
}

This is what keeps the port faithful rather than a hardening extra: Angular's [innerHTML] runs Angular's DOM sanitizer, so the original stripped scripts and event handlers out of this API-provided markup. Bare dangerouslySetInnerHTML does not, which would have made user-authored comment/poll HTML executable in a visitor's browser. Adds one runtime dependency, dompurify, pinned to 3.4.13 — 3.2.6 carries 20 open Snyk findings (16 medium, 4 low: XSS, prototype pollution) and this is a sanitizer shipped in the browser bundle, so it's worth being current on.

  • comment.component.scss used Angular's :host >>> piercing selector to style links inside that injected HTML. Outside view encapsulation :host matches nothing, so it's rescoped to .comment-text a — otherwise comment links would silently lose their bold/underline styling.

Verified: yarn react:build passes; nested comment trees render, collapse/expand works at depth, back returns to the originating feed page, and Ask HN text content renders. Sanitization leaves rich comment markup (links, paragraphs) intact.

Devin-Org: engineering

Link to Devin session: https://app.devin.ai/sessions/1ff25c6cf2f949458f82cc596fc79c65
Requested by: @charityquinn-cognition


Devin Review

Status Commit
⚪ Not started

Run Devin Review

Devin Review (Staging)
Open in Devin Review

@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 potential issues.

Open in Devin Review

Comment thread src-react/item-details/ItemDetails.tsx Outdated
Comment on lines +21 to +41
useEffect(() => {
let ignore = false;
window.scrollTo(0, 0);
setErrorMessage('');

fetchItemContent(itemID)
.then((nextItem) => {
if (!ignore) {
setItem(nextItem);
}
})
.catch(() => {
if (!ignore) {
setErrorMessage('Could not load item comments.');
}
});

return () => {
ignore = true;
};
}, [itemID]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Stale item shown during item-to-item navigation

The effect resets errorMessage but not item before fetching, so navigating between items keeps the previous item's content and comments visible with no loader until the new fetch resolves. This matches the original Angular behavior, so it is not a regression.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread src-react/item-details/comment/Comment.scss
devin-ai-integration Bot and others added 3 commits August 25, 2026 22:24
Co-Authored-By: Charity Quinn <charity.quinn@cognition.ai>
Co-Authored-By: Charity Quinn <charity.quinn@cognition.ai>
Co-Authored-By: Charity Quinn <charity.quinn@cognition.ai>
@devin-ai-integration
devin-ai-integration Bot force-pushed the devin/react-migration-5-item-details branch from 895e94c to 6e369ac Compare August 25, 2026 22:29
Co-Authored-By: Charity Quinn <charity.quinn@cognition.ai>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

Open in Devin Review

Comment thread package.json
"@angular/platform-browser-dynamic": "~9.0.1",
"@angular/router": "~9.0.1",
"@angular/service-worker": "~9.0.1",
"dompurify": "3.4.13",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 dompurify pin differs from description

package.json pins dompurify to 3.4.13 while the PR description says 3.2.6. Both are exact pins so builds stay deterministic; the mismatch is only between code and description. Confirm the intended version.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.4.13 is the intended version — the description was written against the original 3.2.6 pin and has since been updated. 3.2.6 carries 20 open Snyk findings (16 medium, 4 low: XSS, prototype pollution), which is a poor place to be for a sanitizer that ships in the browser bundle; 3.4.13 clears all of them within the same major and is confirmed gone from a rescan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant