Feature/resource detail base#58
Merged
Merged
Conversation
Review Summary by QodoImplement resource detail page with metadata, Markdown notes, and two-column layout
WalkthroughsDescription• Implement resource detail page at /resources/:id with full metadata display • Add imageUrl and mentalState fields to backend GetResourceById response • Create two-column responsive layout: metadata/notes on left, insights panel on right • Integrate Markdown rendering for notes using marked library and custom pipe • Update home component to navigate to detail view instead of opening external URLs Diagramflowchart LR
A["Backend: GetResourceById"] -->|"Add imageUrl, mentalState"| B["Response Model"]
C["Frontend: Home Component"] -->|"Navigate on card click"| D["Resource Detail Page"]
D -->|"Load resource"| E["LearningResourceService.getById"]
E -->|"Fetch data"| F["HTTP Repository"]
D -->|"Render metadata"| G["Left Column: Title, Badges, Image"]
D -->|"Render notes"| H["Right Column: Markdown Notes"]
I["MarkdownPipe"] -->|"Convert to HTML"| H
J["marked library"] -->|"Parse Markdown"| I
File Changes1. apps/web/src/app/features/learning-resource/application/learning-resource.service.ts
|
Code Review by Qodo
1.
|
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 27 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds a complete resource detail page feature, including a new service method Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant HomeComponent
participant Router
participant ResourceDetailComponent
participant LearningResourceService
participant Repository/API
User->>HomeComponent: Click resource card
HomeComponent->>Router: navigate(['/resources', resource.id])
Router->>ResourceDetailComponent: Activate with route param id
ResourceDetailComponent->>ResourceDetailComponent: Extract id from ActivatedRoute
ResourceDetailComponent->>LearningResourceService: loadResource(id)
LearningResourceService->>LearningResourceService: Set loading = true
LearningResourceService->>Repository/API: getById(id)
Repository/API-->>LearningResourceService: Return LearningResource
LearningResourceService->>LearningResourceService: Set loading = false<br/>Store resource in signal
LearningResourceService-->>ResourceDetailComponent: Resource loaded
ResourceDetailComponent->>ResourceDetailComponent: Render with resource,<br/>type metadata, badges
ResourceDetailComponent-->>User: Display resource detail view
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ All CI checks passed! The code is ready for review. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/web/src/app/features/learning-resource/presentation/home/home.component.ts (1)
171-171: Handle navigation promise explicitly.
Line 171drops theRouter.navigate(...)promise. Make the discard explicit (or add.catch) to avoid silent navigation failures and floating-promise lint noise.Proposed tweak
- this.router.navigate(['/resources', resource.id]); + void this.router.navigate(['/resources', resource.id]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/features/learning-resource/presentation/home/home.component.ts` at line 171, The call to this.router.navigate(['/resources', resource.id]) currently drops the returned Promise; make this explicit by handling the navigation promise in the HomeComponent (where the call lives) — either await the call by making the surrounding method async (await this.router.navigate(...)) or explicitly discard with the void operator and add a .catch handler (void this.router.navigate(...).catch(err => /* log or handle err */)); ensure you reference the router.navigate invocation in HomeComponent so linting and runtime navigation errors are properly surfaced.apps/web/src/app/shared/pipes/markdown.pipe.ts (1)
11-12: Harden markdown output with explicit sanitization.
Line 11parses markdown to HTML, but no sanitizer is applied before returning. Add a sanitize step in the pipe so it remains safe even if reused outside currently safe binding contexts.Proposed hardening
-import { Pipe, PipeTransform } from '@angular/core'; +import { inject, Pipe, PipeTransform } from '@angular/core'; +import { DomSanitizer, SecurityContext } from '@angular/platform-browser'; import { marked } from 'marked'; @@ export class MarkdownPipe implements PipeTransform { + private readonly sanitizer = inject(DomSanitizer); + async transform(value: string | null | undefined): Promise<string> { if (!value) return ''; const html = await marked.parse(value); - return html; + return this.sanitizer.sanitize(SecurityContext.HTML, html) ?? ''; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/shared/pipes/markdown.pipe.ts` around lines 11 - 12, The pipe currently returns raw HTML from marked.parse without sanitization; update the MarkdownPipe (the transform method that calls marked.parse) to sanitize the HTML before returning by injecting Angular's DomSanitizer and calling this.sanitizer.sanitize(SecurityContext.HTML, html) (fallback to empty string if sanitize returns null); reference marked.parse, MarkdownPipe, transform, DomSanitizer and SecurityContext.HTML when making the change so the output is always cleaned even if the pipe is reused outside safe binding contexts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/web/src/app/features/learning-resource/presentation/resource-detail/resource-detail.component.html`:
- Around line 172-179: The "Preview Abstract" button in
resource-detail.component.html (and the similar controls referenced) are
dead-click CTAs; either wire them to a handler in ResourceDetailComponent (e.g.,
add an onPreviewAbstract() method and bind (click)="onPreviewAbstract()" to show
a preview modal/toast) or mark them clearly as non-interactive by adding a
disabled state (aria-disabled="true", title="Coming soon", and CSS classes like
opacity-50 pointer-events-none or a specific "disabled" class) plus a
tooltip/toast for clarity; update every matching button (including the others
noted) to use one consistent pattern and ensure accessibility attributes (role,
aria-disabled) and a clear visual disabled style are present.
- Around line 289-293: The textarea in resource-detail.component.html lacks an
accessible label: add an associated <label> for the textarea (give the textarea
a unique id and set label's for attribute) to provide a persistent,
screen-reader-visible name; if a visible label would break the design, include a
visually-hidden label (e.g., class="sr-only") or add an explicit aria-label on
the textarea, ensuring the identifier used matches the textarea id and
preserving the existing placeholder and classes.
- Line 150: The template is applying titlecase directly to underscored values
(r.mentalState), producing labels like "Deep_focus"; fix by normalizing
underscores to spaces before titlecasing—either add a helper method
formatMentalState(state: string) in the ResourceDetailComponent that returns
state.replace(/_/g, ' ') and use formatMentalState(r.mentalState) | titlecase in
the template, or implement a reusable HumanizePipe that replaces underscores
with spaces and then titlecases the result, and use that pipe in place of the
direct titlecase call.
- Line 192: The markdown pipe currently returns raw HTML from marked.parse and
the template binds it via [innerHTML] (r.notes | markdown | async), creating an
XSS risk; update the pipe (its transform method in the MarkdownPipe class) to
inject Angular's DomSanitizer, sanitize the generated HTML with
sanitizer.sanitize(SecurityContext.HTML, html) or (if you intentionally trust
marked) wrap it with sanitizer.bypassSecurityTrustHtml(html), and change the
pipe's return type to SafeHtml so the async binding and [innerHTML] receive a
sanitized/trusted value.
In `@apps/web/src/styles.css`:
- Around line 22-23: The global stylesheet contains a component-scoped selector
":host ::ng-deep .prose" which is ineffective in global CSS; replace it with a
plain global selector (e.g., ".prose") so the markdown/prose styles apply
site-wide—update the rule that currently targets ":host ::ng-deep .prose" to
target ".prose" (or another appropriate global class) and remove the ":host" and
"::ng-deep" tokens from the selector.
---
Nitpick comments:
In
`@apps/web/src/app/features/learning-resource/presentation/home/home.component.ts`:
- Line 171: The call to this.router.navigate(['/resources', resource.id])
currently drops the returned Promise; make this explicit by handling the
navigation promise in the HomeComponent (where the call lives) — either await
the call by making the surrounding method async (await
this.router.navigate(...)) or explicitly discard with the void operator and add
a .catch handler (void this.router.navigate(...).catch(err => /* log or handle
err */)); ensure you reference the router.navigate invocation in HomeComponent
so linting and runtime navigation errors are properly surfaced.
In `@apps/web/src/app/shared/pipes/markdown.pipe.ts`:
- Around line 11-12: The pipe currently returns raw HTML from marked.parse
without sanitization; update the MarkdownPipe (the transform method that calls
marked.parse) to sanitize the HTML before returning by injecting Angular's
DomSanitizer and calling this.sanitizer.sanitize(SecurityContext.HTML, html)
(fallback to empty string if sanitize returns null); reference marked.parse,
MarkdownPipe, transform, DomSanitizer and SecurityContext.HTML when making the
change so the output is always cleaned even if the pipe is reused outside safe
binding contexts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1066abcc-5562-4ad1-946f-5f8317a4d0f9
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
apps/web/package.jsonapps/web/src/app/features/learning-resource/application/learning-resource.service.tsapps/web/src/app/features/learning-resource/presentation/home/home.component.tsapps/web/src/app/features/learning-resource/presentation/learning-resource.routes.tsapps/web/src/app/features/learning-resource/presentation/resource-detail/resource-detail.component.htmlapps/web/src/app/features/learning-resource/presentation/resource-detail/resource-detail.component.tsapps/web/src/app/shared/pipes/markdown.pipe.tsapps/web/src/styles.csslearning-resource/application/src/use-cases/learning-resource/get-resource-by-id.spec.tslearning-resource/application/src/use-cases/learning-resource/get-resource-by-id.ts
…ep only breaks and gfm
|
✅ All CI checks passed! The code is ready for review. |
|
✅ All CI checks passed! The code is ready for review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: resource detail view — full metadata, two‑column layout and Markdown notes
Context
This PR implements the first slice of the v0.7.1 series: a dedicated detail
page for learning resources (
/resources/:id). It connects the existinggetResourceByIduse case to the frontend and displays all resource fields(title, badges, image, metadata, notes) in a clean two‑column layout.
The detail view becomes the central hub for interacting with a resource.
From here, future PRs will add edit, delete, quick toggles, and the
“captured insights” feature (deferred to a later version).
What Changed
Backend
GetResourceByIdResponseModelnow includesimageUrlandmentalStatefields (they were missing from the original response).
Frontend
LearningResourceService.getById(id)method added.ResourceDetailComponent– standalone page with:mental state chip, duration, image, timestamps, URL link
markedlibrary)MarkdownPipe– converts Markdown strings to safe HTML./resources/:idregistered inside the shell layout.HomeComponentnow navigates to the detail page when a resource card isclicked (replaces
window.openfor external URL)..proseclass (headings, lists, links) tomatch the design mockup.
Architecture Notes
(
getDifficultyClass,getEnergyClass,getStatusClass) asHomeComponent, ensuring visual consistency.marked. The pipe is asynchronous,so
asyncpipe is used in the template.the right” pattern from the design. The right column currently only shows
notes, but will host the “Captured Insights” feature in a future PR.implemented in v0.7.2 and v0.7.3.
How to Test
Screenshots:
Summary by CodeRabbit
Release Notes