Feat/angular frontend#42
Merged
Merged
Conversation
… learning-resource domain
…getResourcesByFilter
…to respect the api contract
…proach for AND logic
|
✅ All CI checks passed! The code is ready for review. |
![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: 1
🧹 Nitpick comments (5)
apps/api/.env.example (1)
6-6: Consider adding a blank line at end of file.The dotenv-linter flags a missing blank line at the end of the file, which is a POSIX convention for text files. While not functionally important, it helps ensure consistent formatting across the codebase.
📝 Proposed fix
CORS_ORIGIN=http://localhost:4200 +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/.env.example` at line 6, The file ending with the line "CORS_ORIGIN=http://localhost:4200" is missing a newline at EOF; open the file containing that CORS_ORIGIN entry and add a single blank line (newline character) at the end of the file so the file terminates with an empty line to satisfy POSIX/dotenv-linter expectations.apps/web/src/app/features/learning-resource/infrastructure/learning-resource-http.repository.ts (1)
114-115: Prefer strict parsing for required timestamps instead ofnew Date()fallback.Lines 114-115 currently mask missing/invalid API timestamps by substituting current time, which can corrupt ordering/history views. Fail fast (or explicitly surface parse failure) for required fields.
Proposed fix
+ private parseRequiredDate(value: string, field: string): Date { + const parsed = new Date(value); + if (Number.isNaN(parsed.getTime())) { + throw new Error(`Invalid ${field}: ${value}`); + } + return parsed; + } private toDomain(dto: LearningResourceDto): LearningResource { return { @@ - createdAt: dto.createdAt ? new Date(dto.createdAt) : new Date(), - updatedAt: dto.updatedAt ? new Date(dto.updatedAt) : new Date(), + createdAt: this.parseRequiredDate(dto.createdAt, 'createdAt'), + updatedAt: this.parseRequiredDate(dto.updatedAt, 'updatedAt'),🤖 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/infrastructure/learning-resource-http.repository.ts` around lines 114 - 115, The mapping currently substitutes missing/invalid timestamps with new Date() for createdAt/updatedAt in learning-resource-http.repository.ts which can hide API errors; change the mapping logic in the function that converts the DTO (the code that sets createdAt and updatedAt) to strict-parse required timestamps: attempt to construct Date from dto.createdAt/dto.updatedAt, validate with isNaN(Date.parse(...)) or check date.getTime(), and if parsing fails throw or return an explicit parse error (or a Result/Left) instead of using new Date(); ensure callers of this mapper handle the thrown/returned error so invalid/missing timestamps surface rather than being silently replaced.apps/web/src/app/features/learning-resource/presentation/home/home.component.html (3)
119-144: Consider addingaria-pressedfor screen reader accessibility.The view toggle buttons use
titleattributes but could benefit fromaria-pressedto properly communicate toggle state to assistive technologies.♿ Proposed accessibility enhancement
<button [class]=" viewMode === 'grid' ? 'p-1.5 rounded-md bg-accent-soft dark:bg-accent/20 text-accent' : 'p-1.5 rounded-md text-ink-muted dark:text-slate-400 hover:text-ink transition-colors' " (click)="viewMode = 'grid'" title="Grid view" + [attr.aria-pressed]="viewMode === 'grid'" + aria-label="Grid view" >Apply similar changes to the list view button.
🤖 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.html` around lines 119 - 144, Add an accessible pressed state to the view toggle buttons by binding aria-pressed to the component's viewMode value (e.g., set aria-pressed="{{ viewMode === 'grid' }}" on the grid button and aria-pressed="{{ viewMode === 'list' }}" on the list button) so screen readers can announce the toggle state; update the buttons that set viewMode (referenced as viewMode and the click handlers setting viewMode = 'grid' and viewMode = 'list') to include the aria-pressed attribute reflecting the current selection.
297-301: URL display inconsistency between grid and list views.In grid view, the URL is a clickable link with
target="_blank"andrel="noopener noreferrer". In list view, it's just display text. If clicking the URL in list view should also open the resource, consider making it an anchor as well.✨ Proposed fix for consistency
`@if` (resource.url) { - <span class="text-xs text-ink-muted dark:text-slate-400 truncate block">{{ - resource.url - }}</span> + <a + [href]="resource.url" + target="_blank" + rel="noopener noreferrer" + class="text-xs text-ink-muted dark:text-slate-400 truncate block hover:text-accent transition-colors" + (click)="$event.stopPropagation()" + > + {{ resource.url }} + </a> }🤖 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.html` around lines 297 - 301, The list view currently renders resource.url as plain text while the grid view uses an anchor with target="_blank" and rel="noopener noreferrer"; update the list-view template in home.component.html where resource.url is rendered (the block that checks `@if` (resource.url)) to render it as an <a> anchor with the same attributes used in the grid view so clicking the URL in list view opens the resource in a new tab and preserves noopener noreferrer security behavior.
95-104: Missingnameattribute on status select for consistency.The difficulty and energy selects have
nameattributes, but the status select is missing one. While this doesn't affect functionality withngModel, adding it maintains consistency and can help with form serialization and debugging.✨ Proposed fix
<select + name="status" class="bg-white dark:bg-slate-800 border border-slate-200 dark:border-slate-700 rounded-lg px-3 py-1.5 text-sm text-ink dark:text-slate-200 hover:border-accent transition-colors cursor-pointer" [(ngModel)]="statusFilterValue" (ngModelChange)="applyFilter()"🤖 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.html` around lines 95 - 104, The status select is missing a name attribute; add a name (e.g., name="status") on the <select> that binds to statusFilterValue and triggers (ngModelChange)="applyFilter()" so it matches the other selects (difficulty/energy) and supports form serialization/debugging; update the select that iterates over statuses (referencing statusFilterValue, applyFilter, and statuses) to include the new name attribute.
🤖 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/infrastructure/learning-resource-http.repository.ts`:
- Around line 96-99: The code currently casts arbitrary API strings to the
ResourceStatus union (in method capitalizeStatus and the toApiStatus/path where
lines 107-109 cast values) which can allow invalid enum values; update
capitalizeStatus (and the code paths around the casts at lines 107-109) to
validate input against a whitelist/mapping of allowed API -> domain values
(e.g., a const map from 'in_progress' | 'completed' | ... to ResourceStatus) and
return either a safe default (like 'Unknown' or a defined fallback) or throw a
controlled error when the API value is unrecognized, instead of using `as
ResourceStatus`; ensure to reuse the same mapping in toApiStatus so conversions
are symmetric and no invalid domain value can be produced by a blind cast.
---
Nitpick comments:
In `@apps/api/.env.example`:
- Line 6: The file ending with the line "CORS_ORIGIN=http://localhost:4200" is
missing a newline at EOF; open the file containing that CORS_ORIGIN entry and
add a single blank line (newline character) at the end of the file so the file
terminates with an empty line to satisfy POSIX/dotenv-linter expectations.
In
`@apps/web/src/app/features/learning-resource/infrastructure/learning-resource-http.repository.ts`:
- Around line 114-115: The mapping currently substitutes missing/invalid
timestamps with new Date() for createdAt/updatedAt in
learning-resource-http.repository.ts which can hide API errors; change the
mapping logic in the function that converts the DTO (the code that sets
createdAt and updatedAt) to strict-parse required timestamps: attempt to
construct Date from dto.createdAt/dto.updatedAt, validate with
isNaN(Date.parse(...)) or check date.getTime(), and if parsing fails throw or
return an explicit parse error (or a Result/Left) instead of using new Date();
ensure callers of this mapper handle the thrown/returned error so
invalid/missing timestamps surface rather than being silently replaced.
In
`@apps/web/src/app/features/learning-resource/presentation/home/home.component.html`:
- Around line 119-144: Add an accessible pressed state to the view toggle
buttons by binding aria-pressed to the component's viewMode value (e.g., set
aria-pressed="{{ viewMode === 'grid' }}" on the grid button and aria-pressed="{{
viewMode === 'list' }}" on the list button) so screen readers can announce the
toggle state; update the buttons that set viewMode (referenced as viewMode and
the click handlers setting viewMode = 'grid' and viewMode = 'list') to include
the aria-pressed attribute reflecting the current selection.
- Around line 297-301: The list view currently renders resource.url as plain
text while the grid view uses an anchor with target="_blank" and rel="noopener
noreferrer"; update the list-view template in home.component.html where
resource.url is rendered (the block that checks `@if` (resource.url)) to render it
as an <a> anchor with the same attributes used in the grid view so clicking the
URL in list view opens the resource in a new tab and preserves noopener
noreferrer security behavior.
- Around line 95-104: The status select is missing a name attribute; add a name
(e.g., name="status") on the <select> that binds to statusFilterValue and
triggers (ngModelChange)="applyFilter()" so it matches the other selects
(difficulty/energy) and supports form serialization/debugging; update the select
that iterates over statuses (referencing statusFilterValue, applyFilter, and
statuses) to include the new name attribute.
🪄 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: a62c0659-4a01-4b29-aed9-66a7f975f094
📒 Files selected for processing (7)
apps/api/.env.exampleapps/api/src/learning-resource/dto/request/add-resource.dto.tsapps/api/src/main.tsapps/web/src/app/features/learning-resource/infrastructure/learning-resource-http.repository.tsapps/web/src/app/features/learning-resource/presentation/add-resource/guided/guided-form.component.tsapps/web/src/app/features/learning-resource/presentation/home/home.component.htmllearning-resource/application/src/use-cases/learning-resource/get-resources-by-filter.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/main.ts
- apps/api/src/learning-resource/dto/request/add-resource.dto.ts
- learning-resource/application/src/use-cases/learning-resource/get-resources-by-filter.ts
- apps/web/src/app/features/learning-resource/presentation/add-resource/guided/guided-form.component.ts
…lter validation fails
|
✅ All CI checks passed! The code is ready for review. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/src/app/features/learning-resource/presentation/add-resource/add-resource-hub.component.html (1)
8-11:⚠️ Potential issue | 🟠 MajorAdd an accessible name to the icon-only theme toggle button.
Line 8 defines an icon-only control without an
aria-label, so screen readers won’t announce its purpose. This was previously flagged and is still present.Suggested fix
<button + type="button" (click)="toggleTheme()" + [attr.aria-label]="themeService.isDarkMode() ? 'Switch to light theme' : 'Switch to dark theme'" class="p-2 rounded-md text-ink-muted dark:text-slate-400 hover:text-accent transition-colors" >🤖 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/add-resource/add-resource-hub.component.html` around lines 8 - 11, The icon-only theme toggle button in add-resource-hub.component.html (the element that calls toggleTheme()) lacks an accessible name; update the button element to provide one by adding an aria-label (or aria-labelledby) attribute such as aria-label="Toggle theme" (or a localized string) and/or a visible offscreen label so screen readers announce its purpose, ensuring the label matches the action performed by the toggleTheme() handler and follows localization if applicable.
🧹 Nitpick comments (1)
learning-resource/application/src/use-cases/learning-resource/get-resources-by-filter.spec.ts (1)
171-179: Use a unique test name for the{ filters: {} }case.This test title duplicates the one above, which makes Vitest failure output ambiguous. Consider renaming this one to explicitly mention the empty
filtersobject.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@learning-resource/application/src/use-cases/learning-resource/get-resources-by-filter.spec.ts` around lines 171 - 179, The test title for the case invoking getResourcesByFilter with { filters: {} } is a duplicate and should be made unique; update the test name string in the test(...) call (the test that calls getResourcesByFilter({ learningResourceRepository }, { filters: {} })) to something explicit like "Should return all resources when filters is an empty object" so Vitest failure output is unambiguous.
🤖 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/infrastructure/learning-resource-http.repository.ts`:
- Around line 131-133: The mapping in learning-resource-http.repository.ts that
assigns lastViewed, createdAt, and updatedAt from dto can silently accept
malformed dates and currently defaults createdAt/updatedAt to new Date(),
corrupting ordering; replace the inline conversions with a validated parse:
create or use a helper (e.g., parseIsoDateOrThrow) and call it for
dto.lastViewed, dto.createdAt, and dto.updatedAt, where the helper returns a
Date only if new Date(value) is valid (date.getTime() is not NaN) and otherwise
throws or returns a rejection so the repository fails fast instead of
fabricating dates; update the mapping in the function that builds the entity
(the code doing lastViewed: dto.lastViewed ? ...) to use that validated parser
and do not fallback to new Date().
In
`@apps/web/src/app/features/learning-resource/presentation/home/home.component.html`:
- Around line 119-127: The view-mode toggle buttons rely only on title which is
inaccessible; update the button for grid (and the corresponding list button) to
provide an explicit accessible name and pressed state by adding an aria-label
(or a visually-hidden text node) and binding aria-pressed to the expression
(viewMode === 'grid') for the grid button (and (viewMode === 'list') for the
list button), while keeping the existing (click)="viewMode = 'grid'" handler and
class binding; ensure the same changes are applied to the other button
referenced around the list view (lines 145-153) so both controls expose a clear
label and current pressed state to assistive tech.
---
Duplicate comments:
In
`@apps/web/src/app/features/learning-resource/presentation/add-resource/add-resource-hub.component.html`:
- Around line 8-11: The icon-only theme toggle button in
add-resource-hub.component.html (the element that calls toggleTheme()) lacks an
accessible name; update the button element to provide one by adding an
aria-label (or aria-labelledby) attribute such as aria-label="Toggle theme" (or
a localized string) and/or a visible offscreen label so screen readers announce
its purpose, ensuring the label matches the action performed by the
toggleTheme() handler and follows localization if applicable.
---
Nitpick comments:
In
`@learning-resource/application/src/use-cases/learning-resource/get-resources-by-filter.spec.ts`:
- Around line 171-179: The test title for the case invoking getResourcesByFilter
with { filters: {} } is a duplicate and should be made unique; update the test
name string in the test(...) call (the test that calls getResourcesByFilter({
learningResourceRepository }, { filters: {} })) to something explicit like
"Should return all resources when filters is an empty object" so Vitest failure
output is unambiguous.
🪄 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: f90f5873-b5fa-418d-b62d-3e182355f4ea
📒 Files selected for processing (14)
apps/api/src/learning-resource/dto/request/add-resource.dto.tsapps/web/.vscode/launch.jsonapps/web/README.mdapps/web/src/app/app.spec.tsapps/web/src/app/app.tsapps/web/src/app/core/toast/toast.component.tsapps/web/src/app/features/learning-resource/infrastructure/learning-resource-http.repository.tsapps/web/src/app/features/learning-resource/infrastructure/learning-resource.dto.tsapps/web/src/app/features/learning-resource/presentation/add-resource/add-resource-hub.component.htmlapps/web/src/app/features/learning-resource/presentation/home/home.component.htmlapps/web/src/app/features/learning-resource/presentation/home/home.component.tsapps/web/src/styles.csslearning-resource/application/src/use-cases/learning-resource/get-resources-by-filter.spec.tslearning-resource/application/src/use-cases/learning-resource/get-resources-by-filter.ts
✅ Files skipped from review due to trivial changes (4)
- apps/web/README.md
- apps/web/.vscode/launch.json
- apps/web/src/app/features/learning-resource/infrastructure/learning-resource.dto.ts
- apps/web/src/app/app.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- learning-resource/application/src/use-cases/learning-resource/get-resources-by-filter.ts
- apps/api/src/learning-resource/dto/request/add-resource.dto.ts
- apps/web/src/app/core/toast/toast.component.ts
- apps/web/src/app/features/learning-resource/presentation/home/home.component.ts
…ew toggle buttons
|
✅ All CI checks passed! The code is ready for review. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/web/src/app/features/learning-resource/presentation/home/home.component.html (1)
119-157:⚠️ Potential issue | 🟡 MinorRemove stray
>characters from the view-toggle buttons.Line 129 and Line 157 render a literal
>in the UI (and to assistive tech).🔧 Proposed fix
<button @@ - > <svg @@ <button @@ - > <svg🤖 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.html` around lines 119 - 157, Remove the stray ">" characters rendered inside the two view-toggle buttons that produce literal chevrons and leak to assistive tech; in the Home component template remove the extra ">" text nodes that appear before each <svg> in the buttons that set viewMode to 'grid' and 'list' (the button elements with (click)="viewMode = 'grid'" and (click)="viewMode = 'list'") so the buttons only contain the SVG icons and attributes (also verify no other stray text nodes remain in those button elements).
🧹 Nitpick comments (1)
apps/web/src/app/features/learning-resource/presentation/home/home.component.html (1)
8-13: Expose theme toggle state for assistive tech.
aria-label="Toggle theme"is good; adding pressed/state semantics would make the control clearer.♿ Suggested accessibility improvement
<button (click)="toggleTheme()" class="p-2 rounded-md text-ink-muted dark:text-slate-400 hover:text-accent transition-colors" - aria-label="Toggle theme" + [attr.aria-label]="themeService.isDarkMode() ? 'Switch to light theme' : 'Switch to dark theme'" + [attr.aria-pressed]="themeService.isDarkMode()" >🤖 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.html` around lines 8 - 13, The toggle button currently only has aria-label; update it to expose its on/off state by adding an ARIA pressed attribute bound to the theme state (e.g. add [attr.aria-pressed]="themeService.isDarkMode()" or [aria-pressed]="themeService.isDarkMode()") so assistive tech knows whether the control is active, and ensure the existing toggleTheme() call still toggles themeService.isDarkMode() state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/web/src/app/features/learning-resource/presentation/home/home.component.html`:
- Around line 119-157: Remove the stray ">" characters rendered inside the two
view-toggle buttons that produce literal chevrons and leak to assistive tech; in
the Home component template remove the extra ">" text nodes that appear before
each <svg> in the buttons that set viewMode to 'grid' and 'list' (the button
elements with (click)="viewMode = 'grid'" and (click)="viewMode = 'list'") so
the buttons only contain the SVG icons and attributes (also verify no other
stray text nodes remain in those button elements).
---
Nitpick comments:
In
`@apps/web/src/app/features/learning-resource/presentation/home/home.component.html`:
- Around line 8-13: The toggle button currently only has aria-label; update it
to expose its on/off state by adding an ARIA pressed attribute bound to the
theme state (e.g. add [attr.aria-pressed]="themeService.isDarkMode()" or
[aria-pressed]="themeService.isDarkMode()") so assistive tech knows whether the
control is active, and ensure the existing toggleTheme() call still toggles
themeService.isDarkMode() state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c064c9b-efef-4f02-9bcb-90a5c04e825b
📒 Files selected for processing (2)
apps/web/src/app/features/learning-resource/infrastructure/learning-resource-http.repository.tsapps/web/src/app/features/learning-resource/presentation/home/home.component.html
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/features/learning-resource/infrastructure/learning-resource-http.repository.ts
This was referenced Apr 1, 2026
11 tasks
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: Angular 21 frontend foundation — v0.4.0
Context
This PR implements Phase 4 of the EAP-Ecosystem roadmap: the first usable UI
for managing learning resources from the browser. Built with Angular 21,
standalone components, Signals, and Tailwind CSS v4.
What Changed
Application Foundation
apps/web/with Clean Architecture structure--color-accent,--color-ink,--color-ink-muted,--color-accent-soft)dark:prefixes and@custom-variantThemeServicewith system preference detection andlocalStoragepersistence,guarded against non-browser environments
ToastServiceandToastComponentfor user feedbackApiConfigcentralized incore/configtsconfig.json(@core/*,@shared/*,@features/*)Feature: Learning Resource
Domain:
LearningResourcemodel withDifficultyLevel,EnergyLevel,ResourceStatusLearningResourceFilterinterfaceLearningResourceRepositoryabstract class (injectable token)TopicandResourceTypedomain models and repository contractsInfrastructure:
LearningResourceHttpRepository— GET all, GET by filter, GET by id, POSTTopicHttpRepository— GET all topicsResourceTypeHttpRepository— GET all resource typesApplication:
LearningResourceService— signals-based state (resources,loading,error),loadAll,loadByFilter,addResourceTopicService— signals-based state,loadAllResourceTypeService— signals-based state,loadAllPresentation:
HomeComponent— resource list with grid/list toggle, filters by difficulty,energy and status, dark mode, empty state with CTA
AddResourceHubComponent— entry point at/addwith method selectorGuidedFormComponent— 2-step wizard at/add/guidedwith topic pills,visual difficulty/energy cards, skeleton loading, toast on success
/add/url,/add/voice,/add/import(coming soon)Bug Fixes
getResourcesByFilteruse case now applies AND logic whenmultiple filters are combined
@IsUrl()validator relaxed to accept standardhttp/httpsURLsclearFiltersnow correctly resetsngModel-bound propertiesaddResourceLearningreturn type changed tovoidmatching API contractdark:variants instead of manual CSS overridesArchitecture Notes
the domain remains framework-agnostic
abstract classis used for repository contracts instead of interfacesbecause Angular's DI system requires runtime tokens
DataSourcepattern mirrors the backend: service holds state via Signals,components only read signals and call service methods
routes, no broken navigation
How to Run
Frontend available at
http://localhost:4200API available at
http://localhost:3000Checklist
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores