-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create activity endpoint for bad practice detection #224
base: develop
Are you sure you want to change the base?
Conversation
92b2879
to
372ac51
Compare
72b78fa
to
98d9c8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some comments, you can also create follow-up issues and fix stuff there for some parts. Generally looks good though :)
return new ActivityDTO(openPullRequestsWithBadPractices); | ||
} | ||
|
||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this here? Usually we should not make use of @Transactional
only in rare cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise we get an error because of LOB access.
@Autowired | ||
private UserRepository userRepository; | ||
|
||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need @Transactional
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so because of LOB access. Maybe we could move it into the repository access methods.
} | ||
|
||
@PostMapping("/{login}/badpractices") | ||
public ResponseEntity<List<PullRequestBadPracticeDTO>> detectBadPracticesByUser(@PathVariable String login) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not doing any access checks, I guess any unauthorized user could post to that endpoint in theory. Is that intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily intended but also not an issue I think, since no data is passed along. We could protect it though, what do you think?
@JoinColumn(name = "pullrequest_id") | ||
private PullRequest pullrequest; | ||
|
||
private boolean resolved; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a timestamp in the future, then we automatically know when the user pressed resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the next PR this is set if the intelligence service marks the Bad practice as resolved. maybe for user resolved we add a new variable then?
PullRequest pullRequest, | ||
List<PullRequestBadPracticeDTO> badPractices | ||
) { | ||
return new PullRequestWithBadPracticesDTO( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we can do some better composition. I thought we have a base DTO for the pull request data that we are using for the activity card
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The info dto contains data that we dont need for the dashboard, so I thought that creating a dto without would be a good practice. I can also use the existing one.
webapp/src/app/user/bad-practice-card/bad-practice-card.stories.ts
Outdated
Show resolved
Hide resolved
state = input<PullRequestInfo.StateEnum>(); | ||
isDraft = input<boolean>(); | ||
isMerged = input<boolean>(); | ||
pullRequestLabels = input<Array<LabelInfo>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is possible to just have one input for the pull request base info or so, instead of having an input for each field. I think I did it like this somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other components also use multiple inputs instead of a single one. what is best practice?
webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts
Outdated
Show resolved
Hide resolved
webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts
Outdated
Show resolved
Hide resolved
webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts
Outdated
Show resolved
Hide resolved
WalkthroughThis pull request introduces new API endpoints and data models for managing user activities and detecting pull request bad practices in the Hephaestus application. On the backend, several new services, controllers, repositories, and database migrations have been added or updated. On the frontend, Angular routes, components, OpenAPI service interfaces, and Storybook stories have been implemented to support an activity dashboard and related UI elements displaying bad practices in pull requests. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant AC as ActivityController
participant AS as ActivityService
participant Repo as PullRequestBadPracticeRepository
participant Detector as PullRequestBadPracticeDetector
U->>AC: GET /activity/{login}
AC->>AS: getActivity(login)
AS->>Repo: Query pull requests & bad practices
Repo-->>AS: Return data
AS-->>AC: Construct and return ActivityDTO
AC-->>U: HTTP 200 with ActivityDTO
sequenceDiagram
participant U as User
participant AC as ActivityController
participant AS as ActivityService
participant Detector as PullRequestBadPracticeDetector
participant Repo as PullRequestBadPracticeRepository
U->>AC: POST /activity/{login}/badpractices
AC->>AS: detectBadPractices(login)
AS->>Detector: Process pull requests for bad practices
Detector->>Repo: Retrieve existing bad practices
Repo-->>Detector: Return bad practices list
Detector-->>AS: Send bad practices list
AS-->>AC: Return list of PullRequestBadPracticeDTO
AC-->>U: HTTP 200 with bad practices data
sequenceDiagram
participant User as User
participant ADC as ActivityDashboardComponent
participant AS as Angular ActivityService (client)
participant API as Hephaestus API
User->>ADC: Navigate to Activity Dashboard
ADC->>AS: getActivityByUser(login)
AS->>API: GET /activity/{login}
API-->>AS: Return Activity DTO
AS-->>ADC: Provide activity data
ADC->>User: Render dashboard with activity & bad practices info
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
♻️ Duplicate comments (4)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityController.java (1)
17-21
:⚠️ Potential issueAdd error handling and access control.
Both endpoints need:
- Access control to prevent unauthorized access
- Error handling for service exceptions
@GetMapping("/{login}") public ResponseEntity<ActivityDTO> getActivityByUser(@PathVariable String login) { + // Add authentication check + try { ActivityDTO activity = activityService.getActivity(login); return ResponseEntity.ok(activity); + } catch (Exception e) { + logger.error("Error getting activity for user {}: {}", login, e.getMessage()); + return ResponseEntity.internalServerError().build(); + } } @PostMapping("/{login}/badpractices") public ResponseEntity<List<PullRequestBadPracticeDTO>> detectBadPracticesByUser(@PathVariable String login) { + // Add authentication check + try { List<PullRequestBadPracticeDTO> badPractices = activityService.detectBadPractices(login); return ResponseEntity.ok(badPractices); + } catch (Exception e) { + logger.error("Error detecting bad practices for user {}: {}", login, e.getMessage()); + return ResponseEntity.internalServerError().build(); + } }Also applies to: 23-27
webapp/src/app/home/activity/activity-dashboard.component.ts (1)
28-32
:⚠️ Potential issueAdd error handling for the activity query.
The query lacks error handling for cases where:
- The user doesn't exist
- The API request fails
Also, the 400ms timer delay needs justification or removal.
Consider implementing error handling like this:
query = injectQuery(() => ({ queryKey: ['user', { id: this.userLogin }], enabled: !!this.userLogin, - queryFn: async () => lastValueFrom(combineLatest([this.activityService.getActivityByUser(this.userLogin!), timer(400)]).pipe(map(([activity]) => activity))) + queryFn: async () => { + try { + return await lastValueFrom(this.activityService.getActivityByUser(this.userLogin!)); + } catch (error) { + if (error.status === 404) { + throw new Error(`User ${this.userLogin} not found`); + } + throw error; + } + } }));server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/model/PullRequestWithBadPracticesDTO.java (1)
12-26
: 🛠️ Refactor suggestionConsider using composition with a base PullRequest DTO.
The record contains many fields that appear to be common pull request data. Consider extracting these into a base DTO and composing with it to reduce duplication and improve maintainability.
Example refactor:
-public record PullRequestWithBadPracticesDTO( - @NonNull Long id, - @NonNull Integer number, - @NonNull String title, - @NonNull Issue.State state, - @NonNull Boolean isDraft, - @NonNull Boolean isMerged, - List<LabelInfoDTO> labels, - RepositoryInfoDTO repository, - @NonNull Integer additions, - @NonNull Integer deletions, - @NonNull String htmlUrl, - OffsetDateTime createdAt, - List<PullRequestBadPracticeDTO> badPractices -) { +public record PullRequestWithBadPracticesDTO( + @NonNull PullRequestBaseDTO pullRequest, + List<PullRequestBadPracticeDTO> badPractices +) {server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java (1)
36-36
: 🛠️ Refactor suggestionRemove unnecessary
@Transactional
annotation.The
getActivity
method is read-only and doesn't modify any data. The@Transactional
annotation is unnecessary for read-only operations.- @Transactional public ActivityDTO getActivity(String login) {
🧹 Nitpick comments (12)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/model/ActivityDTO.java (1)
3-6
: Consider using standard null safety annotations.Replace
io.micrometer.common.lang.NonNull
withjakarta.validation.constraints.NotNull
for better integration with Spring's validation framework and consistency with Jakarta EE standards.-import io.micrometer.common.lang.NonNull; +import jakarta.validation.constraints.NotNull; -public record ActivityDTO(@NonNull List<PullRequestWithBadPracticesDTO> pullRequests) {} +public record ActivityDTO(@NotNull List<PullRequestWithBadPracticesDTO> pullRequests) {}server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/model/PullRequestBadPracticeDTO.java (1)
3-3
: Add null safety annotations to fields.Consider adding
@NotNull
annotations to the fields that should not be null. This helps with validation and provides better documentation of the contract.-public record PullRequestBadPracticeDTO(String title, String description, boolean resolved) { +public record PullRequestBadPracticeDTO( + @NotNull String title, + @NotNull String description, + boolean resolved) {server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityController.java (1)
14-15
: Use constructor injection instead of field injection.Field injection using
@Autowired
is discouraged as it makes the class harder to test and hides dependencies.- @Autowired - private ActivityService activityService; + private final ActivityService activityService; + + public ActivityController(ActivityService activityService) { + this.activityService = activityService; + }server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/PullRequestBadPracticeRepository.java (2)
12-19
: Optimize the JPQL query for findAssignedByLoginAndOpen.The current query performs case-insensitive comparison using LOWER function on both sides, which can impact performance.
@Query( """ SELECT prbp FROM PullRequestBadPractice prbp - WHERE LOWER(:assigneeLogin) IN (SELECT LOWER(u.login) FROM prbp.pullrequest.assignees u) AND prbp.pullrequest.state = 'OPEN' + WHERE :assigneeLogin IN ( + SELECT u.login FROM prbp.pullrequest.assignees u + WHERE LOWER(u.login) = LOWER(:assigneeLogin) + ) AND prbp.pullrequest.state = 'OPEN' """ )
21-28
: Replace custom query with derived query method.The custom JPQL query for findByPullRequestId can be replaced with a derived query method.
- @Query( - """ - SELECT prbp - FROM PullRequestBadPractice prbp - WHERE prbp.pullrequest.id = :pullRequestId - """ - ) - List<PullRequestBadPractice> findByPullRequestId(@Param("pullRequestId") long pullRequestId); + List<PullRequestBadPractice> findByPullrequestId(long pullRequestId);server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java (1)
17-18
: Use constructor injection instead of field injection.Field injection using
@Autowired
is discouraged as it makes the class harder to test and hides dependencies.- @Autowired - private PullRequestBadPracticeRepository pullRequestBadPracticeRepository; + private final PullRequestBadPracticeRepository pullRequestBadPracticeRepository; + + public PullRequestBadPracticeDetector(PullRequestBadPracticeRepository pullRequestBadPracticeRepository) { + this.pullRequestBadPracticeRepository = pullRequestBadPracticeRepository; + }webapp/src/app/core/modules/openapi/model/pull-request-with-bad-practices.ts (1)
17-31
: Add JSDoc comments to document the interface properties.While the interface is well-structured, adding JSDoc comments would improve code maintainability and developer experience.
Add documentation like this:
export interface PullRequestWithBadPractices { + /** Unique identifier of the pull request */ id: number; + /** Pull request number in the repository */ number: number; + /** Title of the pull request */ title: string; // ... add similar comments for other properties }webapp/src/app/core/modules/openapi/api/activity.serviceInterface.ts (1)
28-40
: Improve method documentation with detailed JSDoc comments.The method documentation is incomplete. Add comprehensive JSDoc comments including parameter descriptions and return value details.
Add documentation like this:
/** - * - * - * @param login + * Detects bad practices in pull requests for a specific user + * + * @param login GitHub username of the user + * @returns Observable of array of detected bad practices */ detectBadPracticesByUser(login: string, extraHttpRequestParams?: any): Observable<Array<PullRequestBadPractice>>; /** - * - * - * @param login + * Retrieves activity data for a specific user + * + * @param login GitHub username of the user + * @returns Observable of user's activity data */ getActivityByUser(login: string, extraHttpRequestParams?: any): Observable<Activity>;webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.stories.ts (1)
26-28
: Add # prefix to hexadecimal color values.The label colors are missing the # prefix for hexadecimal values.
- { id: 1, name: 'bug', color: 'f00000' }, - { id: 2, name: 'enhancement', color: '008000' } + { id: 1, name: 'bug', color: '#f00000' }, + { id: 2, name: 'enhancement', color: '#008000' }webapp/src/app/core/modules/openapi/api/activity.service.ts (2)
115-115
: Improve code readability with optional chaining.Use optional chaining to simplify the code and make it more readable.
- let localVarHttpHeaderAcceptSelected: string | undefined = options && options.httpHeaderAccept; + let localVarHttpHeaderAcceptSelected: string | undefined = options?.httpHeaderAccept; - let localVarHttpContext: HttpContext | undefined = options && options.context; + let localVarHttpContext: HttpContext | undefined = options?.context; - let localVarTransferCache: boolean | undefined = options && options.transferCache; + let localVarTransferCache: boolean | undefined = options?.transferCache;Also applies to: 127-127, 132-132
🧰 Tools
🪛 Biome (1.9.4)
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
178-178
: Improve code readability with optional chaining.Use optional chaining to simplify the code and make it more readable.
- let localVarHttpHeaderAcceptSelected: string | undefined = options && options.httpHeaderAccept; + let localVarHttpHeaderAcceptSelected: string | undefined = options?.httpHeaderAccept; - let localVarHttpContext: HttpContext | undefined = options && options.context; + let localVarHttpContext: HttpContext | undefined = options?.context; - let localVarTransferCache: boolean | undefined = options && options.transferCache; + let localVarTransferCache: boolean | undefined = options?.transferCache;Also applies to: 190-190, 195-195
🧰 Tools
🪛 Biome (1.9.4)
[error] 178-178: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
server/application-server/openapi.yaml (1)
785-794
: Enhance PullRequestBadPractice SchemaThe
PullRequestBadPractice
schema defines properties fortitle
,description
, andresolved
, but it does not specify which fields are mandatory. If these properties are essential, consider adding arequired
section to enforce validation and ensure data consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
server/application-server/openapi.yaml
(5 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityController.java
(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java
(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/PullRequestBadPracticeRepository.java
(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java
(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/model/ActivityDTO.java
(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/model/PullRequestBadPractice.java
(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/model/PullRequestBadPracticeDTO.java
(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/model/PullRequestWithBadPracticesDTO.java
(1 hunks)server/application-server/src/main/resources/db/changelog/1738063112622_changelog.xml
(1 hunks)server/application-server/src/main/resources/db/master.xml
(1 hunks)webapp/src/app/app.routes.ts
(2 hunks)webapp/src/app/core/header/header.component.html
(1 hunks)webapp/src/app/core/modules/openapi/.openapi-generator/FILES
(2 hunks)webapp/src/app/core/modules/openapi/api/activity.service.ts
(1 hunks)webapp/src/app/core/modules/openapi/api/activity.serviceInterface.ts
(1 hunks)webapp/src/app/core/modules/openapi/api/api.ts
(2 hunks)webapp/src/app/core/modules/openapi/model/activity.ts
(1 hunks)webapp/src/app/core/modules/openapi/model/models.ts
(1 hunks)webapp/src/app/core/modules/openapi/model/pull-request-bad-practice.ts
(1 hunks)webapp/src/app/core/modules/openapi/model/pull-request-with-bad-practices.ts
(1 hunks)webapp/src/app/home/activity/activity-dashboard.component.html
(1 hunks)webapp/src/app/home/activity/activity-dashboard.component.ts
(1 hunks)webapp/src/app/user/bad-practice-card/bad-practice-card.component.html
(1 hunks)webapp/src/app/user/bad-practice-card/bad-practice-card.component.ts
(1 hunks)webapp/src/app/user/bad-practice-card/bad-practice-card.stories.ts
(1 hunks)webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.html
(1 hunks)webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts
(1 hunks)webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.stories.ts
(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
webapp/src/app/app.routes.ts
[error] 51-51: expected ,
but instead found {
Remove {
(parse)
webapp/src/app/core/modules/openapi/api/activity.service.ts
[error] 115-115: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 127-127: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 132-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 178-178: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 190-190: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 195-195: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: Webapp QA
webapp/src/app/app.routes.ts
[error] 51-51: Parsing error: ',' expected
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Verify API Specs and Client of the Intelligence Service (add autocommit-openapi label to PR to au...
- GitHub Check: Run Chromatic
- GitHub Check: Mend Security Check
🔇 Additional comments (25)
webapp/src/app/core/modules/openapi/model/pull-request-bad-practice.ts (1)
14-18
:❌ Incorrect review comment
Verify field optionality in OpenAPI specification.
The TypeScript interface marks all fields as optional (
?
), but the corresponding Java DTO has required fields. This mismatch could lead to runtime errors.Please check the OpenAPI specification to ensure field requirements are correctly defined:
🏁 Script executed:
#!/bin/bash # Description: Check field requirements in OpenAPI spec for PullRequestBadPractice # Test: Search for PullRequestBadPractice definition in OpenAPI spec rg -A 10 "PullRequestBadPractice:" server/application-server/openapi.yamlLength of output: 293
Clarification on Field Optionality in
PullRequestBadPractice
After reviewing the OpenAPI specification, the definition for
PullRequestBadPractice
does not include arequired
property, which means all fields (i.e.,title
,description
, andresolved
) are optional. This aligns with the generated TypeScript interface. If the intent was for these fields to be mandatory—as suggested by the Java DTO—then the OpenAPI spec should be updated to reflect those requirements.
- File under review:
webapp/src/app/core/modules/openapi/model/pull-request-bad-practice.ts
(lines 14–18)- Action required (if applicable): Verify that the Java DTO’s intended field requirements are correctly represented. If these fields should indeed be required, update the OpenAPI specification accordingly.
Likely an incorrect or invalid review comment.
webapp/src/app/core/modules/openapi/model/activity.ts (1)
15-17
: LGTM!The interface correctly defines
pullRequests
as a required field, matching the Java DTO's contract.webapp/src/app/core/modules/openapi/model/models.ts (1)
6-6
: LGTM!The new exports for bad practice models are correctly added and maintain alphabetical ordering.
Also applies to: 10-10
webapp/src/app/user/bad-practice-card/bad-practice-card.component.ts (2)
8-8
: Removestandalone: true
.
13-20
: LGTM!The component is well-structured with:
- Properly defined input properties using the new input() syntax
- Protected icon declarations for encapsulation
webapp/src/app/user/bad-practice-card/bad-practice-card.stories.ts (2)
4-19
: LGTM!The Storybook configuration is well-structured with:
- Proper meta configuration for autodocs
- Meaningful default story with realistic example data
20-26
: Remove commented-out code.server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/model/PullRequestBadPractice.java (1)
31-31
: Consider using timestamp for resolved field.Using a timestamp instead of a boolean would provide more information about when the bad practice was resolved.
webapp/src/app/core/modules/openapi/api/api.ts (1)
1-3
: LGTM!The ActivityService integration follows the established pattern for service exports and maintains consistency with the existing codebase.
Also applies to: 25-25
webapp/src/app/home/activity/activity-dashboard.component.ts (2)
25-25
: Route parameter should be 'login' instead of 'id'.The route parameter name should reflect its content (GitHub login) rather than using a generic 'id'.
34-37
: Implement or remove the commented-out bad practice detection code.The current implementation only logs to console, and the actual functionality is commented out. Either implement the feature or remove the placeholder code.
webapp/src/app/app.routes.ts (1)
51-51
: Consider moving the activity route under the user path.To maintain consistency with other user-related pages, consider moving this route under the user path structure.
- { path: 'activity/:id', component: ActivityDashboardComponent, canActivate: [AuthGuard, AdminGuard]}, + { path: 'user/:id/activity', component: ActivityDashboardComponent, canActivate: [AuthGuard, AdminGuard]},🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: expected
,
but instead found{
Remove {
(parse)
🪛 GitHub Actions: Webapp QA
[error] 51-51: Parsing error: ',' expected
webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts (4)
8-8
: Update dayjs import path.Use the ESM version of dayjs to maintain consistency with other imports.
-import dayjs from 'dayjs'; +import dayjs from 'dayjs/esm';
33-33
: Remove standalone: true.As per previous feedback, remove the standalone flag from the component.
- standalone: true
42-55
: Consolidate pull request info inputs.Instead of having individual inputs for each pull request field, consider using a single input for the base pull request information.
- isLoading = input(false); - class = input(''); - title = input<string>(); - number = input<number>(); - additions = input<number>(); - deletions = input<number>(); - htmlUrl = input<string>(); - repositoryName = input<string>(); - createdAt = input<string>(); - state = input<PullRequestInfo.StateEnum>(); - isDraft = input<boolean>(); - isMerged = input<boolean>(); - pullRequestLabels = input<Array<LabelInfo>>(); + isLoading = input(false); + class = input(''); + pullRequestInfo = input<PullRequestInfo>();
57-57
: Create a shared date formatting helper.Since date formatting is used in multiple places, consider creating a shared helper function.
Create a new utility file
date.utils.ts
:import dayjs from 'dayjs/esm'; export const formatDate = (date: string) => dayjs(date);Then update the usage:
- displayCreated = computed(() => dayjs(this.createdAt())); + displayCreated = computed(() => formatDate(this.createdAt()));server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java (1)
77-94
: LGTM!The implementation is well-structured with proper error handling and user validation. The
@Transactional
annotation is correctly used here as the method modifies data throughdetectAndSyncBadPractices
.webapp/src/app/user/bad-practice-card/bad-practice-card.component.html (1)
1-13
: LGTM!The template is well-structured with proper spacing and follows accessibility best practices. The conditional rendering is implemented correctly.
server/application-server/src/main/resources/db/changelog/1738063112622_changelog.xml (2)
8-8
: Review the 'description' Column LengthThe "description" column is defined as
VARCHAR(255)
, which may be too restrictive if longer descriptions are expected. Please verify that this limitation is intentional or consider increasing the length and/or adding proper validation on the application side.
17-17
: Verify Foreign Key Constraint IntegrityThe foreign key constraint linking
pullrequest_id
inpull_request_bad_practice
toid
in theissue
table is configured appropriately with validation enabled. Just ensure that the referencedissue
table exists and retains the expected structure over time.webapp/src/app/core/header/header.component.html (1)
12-12
: Approve New Activity Link AdditionThe newly added "Activity" link is conditionally rendered for authenticated admin users, and its routerLink is correctly constructed using the user's username. This change aligns well with the intended access control and navigation improvements.
server/application-server/openapi.yaml (3)
291-310
: Approve New Bad Practices Detection EndpointThe POST endpoint
/activity/{login}/badpractices
is defined clearly with the appropriate path parameter and a response schema that returns an array ofPullRequestBadPractice
objects. This endpoint should integrate well with the backend service handling bad practice detection.
415-432
: Approve Activity Retrieval EndpointThe GET endpoint
/activity/{login}
is defined correctly to return anActivity
object that encapsulates pull requests with bad practices. Please ensure that the backend implementation adheres to this specification.
644-736
: Review New Schema Definitions for ActivityThe new schemas
PullRequestWithBadPractices
andActivity
appear to comprehensively capture the necessary fields for pull request details and user activity. It’s important to verify that these definitions accurately reflect the backend data model and that all required fields are included where necessary.webapp/src/app/core/modules/openapi/.openapi-generator/FILES (1)
5-6
:✅ Verification successful
LGTM! The new OpenAPI-generated files align with the PR objectives.
The added files properly support the activity endpoint and bad practice detection functionality:
- Service files for activity operations
- Model files for activities and bad practices
Let's verify the OpenAPI generator configuration:
Also applies to: 26-26, 32-32, 36-36
🏁 Script executed:
#!/bin/bash # Description: Verify OpenAPI generator configuration and generated files # Check if OpenAPI generator config exists if [ -f "webapp/src/app/core/modules/openapi/.openapi-generator-ignore" ]; then echo "OpenAPI generator ignore file exists" cat webapp/src/app/core/modules/openapi/.openapi-generator-ignore fi # Check if the generated files exist and are not empty for file in "webapp/src/app/core/modules/openapi/api/activity.service.ts" \ "webapp/src/app/core/modules/openapi/api/activity.serviceInterface.ts" \ "webapp/src/app/core/modules/openapi/model/activity.ts" \ "webapp/src/app/core/modules/openapi/model/pull-request-bad-practice.ts" \ "webapp/src/app/core/modules/openapi/model/pull-request-with-bad-practices.ts"; do if [ -s "$file" ]; then echo "File exists and is not empty: $file" else echo "File is missing or empty: $file" fi doneLength of output: 4443
Approval: OpenAPI-generated files verified and aligned with PR objectives
We've confirmed that the OpenAPI generator configuration is intact and all the new files are present and non-empty:
- The configuration file (
.openapi-generator-ignore
) is correctly in place.- Service files (
api/activity.service.ts
andapi/activity.serviceInterface.ts
) and model files (model/activity.ts
,model/pull-request-bad-practice.ts
, andmodel/pull-request-with-bad-practices.ts
) exist and contain valid code.LGTM—the changes support the activity endpoint and the bad practice detection as intended.
...server/src/main/java/de/tum/in/www1/hephaestus/activity/model/PullRequestBadPracticeDTO.java
Show resolved
Hide resolved
...on-server/src/main/java/de/tum/in/www1/hephaestus/activity/model/PullRequestBadPractice.java
Show resolved
Hide resolved
...a/de/tum/in/www1/hephaestus/activity/badpracticedetector/PullRequestBadPracticeDetector.java
Show resolved
Hide resolved
server/application-server/src/main/java/de/tum/in/www1/hephaestus/activity/ActivityService.java
Show resolved
Hide resolved
...pp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.html
Show resolved
Hide resolved
Co-authored-by: Felix T.J. Dietrich <[email protected]> Co-authored-by: Felix T.J. Dietrich <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Felix T.J. Dietrich <[email protected]> Co-authored-by: Felix T.J. Dietrich <[email protected]>
… into 223-creating-activity-dashboard # Conflicts: # server/application-server/src/main/resources/db/changelog/1738063112622_changelog.xml # server/application-server/src/main/resources/db/master.xml # webapp/src/app/app.routes.ts # webapp/src/app/core/header/header.component.html # webapp/src/app/home/activity/activity-dashboard.component.html # webapp/src/app/user/bad-practice-card/bad-practice-card.component.ts # webapp/src/app/user/bad-practice-card/bad-practice-card.stories.ts # webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (7)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserRepository.java (1)
50-58
: Consider using LEFT JOIN FETCH for consistent behavior withfindAllWithEagerTeams()
.The new method uses
JOIN FETCH
while the similar methodfindAllWithEagerTeams()
usesLEFT JOIN FETCH
. This inconsistency could lead to different behavior when users have no teams.Apply this diff to maintain consistency:
@Query( """ SELECT u FROM User u - JOIN FETCH u.teams + LEFT JOIN FETCH u.teams WHERE u.type = 'USER' """ ) List<User> findAllHumanInTeams();server/application-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/LeaderboardService.java (1)
116-141
: Consider extracting comment counting logic into a separate method.The sorting logic is complex and could benefit from being broken down into smaller, more focused methods for better readability and maintainability.
Consider extracting the comment counting logic into a separate method:
+ private int calculateTotalComments(Long userId, Map<Long, List<PullRequestReview>> reviewsByUserId, Map<Long, List<IssueComment>> issueCommentsByUserId) { + int reviewComments = reviewsByUserId + .getOrDefault(userId, List.of()) + .stream() + .mapToInt(review -> review.getComments().size()) + .sum(); + int issueComments = issueCommentsByUserId.getOrDefault(userId, List.of()).size(); + return reviewComments + issueComments; + } // In createLeaderboard method: List<Long> rankingByUserId = scoresByUserId .entrySet() .stream() .sorted((e1, e2) -> { int scoreCompare = e2.getValue().compareTo(e1.getValue()); if (scoreCompare != 0) { return scoreCompare; } - int e1ReviewComments = reviewsByUserId - .getOrDefault(e1.getKey(), List.of()) - .stream() - .mapToInt(review -> review.getComments().size()) - .sum(); - int e2ReviewComments = reviewsByUserId - .getOrDefault(e2.getKey(), List.of()) - .stream() - .mapToInt(review -> review.getComments().size()) - .sum(); - int e1IssueComments = issueCommentsByUserId.getOrDefault(e1.getKey(), List.of()).size(); - int e2IssueComments = issueCommentsByUserId.getOrDefault(e2.getKey(), List.of()).size(); - int e1TotalComments = e1ReviewComments + e1IssueComments; - int e2TotalComments = e2ReviewComments + e2IssueComments; + int e1TotalComments = calculateTotalComments(e1.getKey(), reviewsByUserId, issueCommentsByUserId); + int e2TotalComments = calculateTotalComments(e2.getKey(), reviewsByUserId, issueCommentsByUserId); return Integer.compare(e2TotalComments, e1TotalComments); })webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts (2)
18-34
: Remove trailing comma in imports array.The static analysis tool suggests removing the trailing comma after the last import.
BrnCollapsibleTriggerDirective, HlmButtonDirective, - GithubLabelComponent + GithubLabelComponent🧰 Tools
🪛 GitHub Check: Code Quality Checks
[failure] 33-33:
Delete,
61-84
: Simplify icon and color selection logic.The nested conditionals in
issueIconAndColor
could be simplified using a lookup object or switch statement for better readability and maintainability.issueIconAndColor = computed(() => { - var icon: string; - var color: string; - - if (this.state() === PullRequestInfo.StateEnum.Open) { - if (this.isDraft()) { - icon = octGitPullRequestDraft; - color = 'text-github-muted-foreground'; - } else { - icon = octGitPullRequest; - color = 'text-github-open-foreground'; - } - } else { - if (this.isMerged()) { - icon = octGitMerge; - color = 'text-github-done-foreground'; - } else { - icon = octGitPullRequestClosed; - color = 'text-github-closed-foreground'; - } - } - - return { icon, color }; + const iconConfig = { + draft: { + icon: octGitPullRequestDraft, + color: 'text-github-muted-foreground' + }, + open: { + icon: octGitPullRequest, + color: 'text-github-open-foreground' + }, + merged: { + icon: octGitMerge, + color: 'text-github-done-foreground' + }, + closed: { + icon: octGitPullRequestClosed, + color: 'text-github-closed-foreground' + } + }; + + if (this.state() === PullRequestInfo.StateEnum.Open && this.isDraft()) { + return iconConfig.draft; + } + if (this.state() === PullRequestInfo.StateEnum.Open) { + return iconConfig.open; + } + if (this.isMerged()) { + return iconConfig.merged; + } + return iconConfig.closed; });server/intelligence-service/README.md (1)
23-26
: Clarify and Enhance Poetry Update Instructions
The added instructions clearly inform users to update Poetry if running a version earlier than 2.0.0. For extra clarity, consider explicitly stating something like “If your Poetry version is below 2.0.0, please update by running …”. Additionally, linking to the official Poetry documentation could help users unfamiliar with the update process..vscode/settings.json (1)
17-19
: Removal of Python Terminal Activation Settings:
The keys"python.terminal.activateEnvironment"
,"python.terminal.activateEnvInCurrentTerminal"
, and"python.defaultInterpreterPath"
have been removed from the settings. Please ensure this change aligns with the updated development workflow and that any necessary documentation is updated so that developers are aware of the alternative setup procedures.server/intelligence-service/pyproject.toml (1)
15-18
: Synchronize langchain ecosystem versions.Consider using the same version constraint for all langchain-related packages to ensure compatibility:
- langchain
- langchain-openai
- langgraph
- langchain-community
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/intelligence-service/poetry.lock
is excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/generate-intelligence-service-client.yml
(2 hunks).github/workflows/intelligence-service-qa.yml
(2 hunks).vscode/settings.json
(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserRepository.java
(1 hunks)server/application-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/LeaderboardService.java
(2 hunks)server/application-server/src/main/resources/db/changelog/1738063112622_changelog.xml
(1 hunks)server/application-server/src/main/resources/db/master.xml
(1 hunks)server/intelligence-service/README.md
(1 hunks)server/intelligence-service/pyproject.toml
(1 hunks)webapp/src/app/app.routes.ts
(2 hunks)webapp/src/app/core/header/header.component.html
(1 hunks)webapp/src/app/home/activity/activity-dashboard.component.html
(1 hunks)webapp/src/app/user/bad-practice-card/bad-practice-card.component.ts
(1 hunks)webapp/src/app/user/bad-practice-card/bad-practice-card.stories.ts
(1 hunks)webapp/src/app/user/issue-card/issue-card.component.ts
(2 hunks)webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts
(1 hunks)webapp/src/app/utils.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- webapp/src/app/core/header/header.component.html
- server/application-server/src/main/resources/db/master.xml
- webapp/src/app/home/activity/activity-dashboard.component.html
- webapp/src/app/user/bad-practice-card/bad-practice-card.stories.ts
- webapp/src/app/user/bad-practice-card/bad-practice-card.component.ts
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/intelligence-service-qa.yml
[warning] 27-27: wrong indentation: expected 12 but found 10
(indentation)
🪛 GitHub Check: Code Quality Checks
webapp/src/app/app.routes.ts
[failure] 52-52:
Delete ⏎
webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts
[failure] 33-33:
Delete ,
🪛 GitHub Actions: Webapp QA
webapp/src/app/app.routes.ts
[error] 52-52: Delete ⏎
prettier/prettier
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Webhook Ingest / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webapp / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Intelligence Service / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Webhook Ingest / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webhook-ingest
- GitHub Check: Webapp / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/webapp
- GitHub Check: Application Server / Build linux/arm64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Intelligence Service / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/intelligence-service
- GitHub Check: Application Server / Build linux/amd64 Docker Image for ghcr.io/ls1intum/hephaestus/application-server
- GitHub Check: Run Chromatic
- GitHub Check: Mend Security Check
🔇 Additional comments (15)
server/application-server/src/main/resources/db/changelog/1738063112622_changelog.xml (2)
3-15
: New Table Creation and Column Type ReviewThe change set creates the
pull_request_bad_practice
table with the expected columns includingid
,description
,resolved
,title
, andpullrequest_id
. However, note that thedescription
column is defined as typeOID
, which is typically used to store references to large objects rather than inline textual data. A previous comment raised concerns about description lengths, and the type was changed accordingly. Please confirm that usingOID
here is intentional and aligns with your design requirements.
16-18
: Foreign Key Constraint VerificationThe foreign key constraint added on
pull_request_bad_practice.pullrequest_id
referencingissue.id
is implemented properly and ensures referential integrity.server/application-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/LeaderboardService.java (1)
87-90
: LGTM! Good addition to include all active users in the total leaderboard.The implementation correctly uses the new
findAllHumanInTeams()
method to fetch all human users when no specific team is provided.webapp/src/app/user/pull-request-bad-practice-card/pull-request-bad-practice-card.component.ts (2)
1-17
: LGTM! Well-organized imports.The imports are well-structured, following the correct pattern with
dayjs/esm
and utilizing shared utility functions.
42-55
: Consider consolidating related inputs.While the current approach of having multiple granular inputs aligns with other components in the codebase, consider consolidating related inputs into a single input object for better maintainability. For example, combine pull request metadata into a single input.
- title = input<string>(); - number = input<number>(); - additions = input<number>(); - deletions = input<number>(); - htmlUrl = input<string>(); - repositoryName = input<string>(); - createdAt = input<string>(); - state = input<PullRequestInfo.StateEnum>(); - isDraft = input<boolean>(); - isMerged = input<boolean>(); + pullRequestInfo = input<PullRequestInfo>();.github/workflows/intelligence-service-qa.yml (1)
40-42
: Review Dependency Installation Command
The dependency installation command now removes the--no-update
flag (i.e.,poetry lock && poetry install --no-interaction --no-root
). Please verify that allowing updates during locking is intended for the QA workflow and that it does not lead to unexpected dependency changes..github/workflows/generate-intelligence-service-client.yml (2)
46-50
: Ensure Consistency in Poetry Installation Configuration
The Poetry installation configuration (withversion: 2.1.1
and related options) appears aligned with the QA workflow. Please double-check that the indentation and option names follow project conventions throughout all workflow files for consistency.
59-61
: Verify Dependency Installation Behavior in Client Generation Workflow
The commandpoetry lock && poetry install --no-interaction --no-root
is used to install Python dependencies. As with the QA workflow, ensure that removing the--no-update
flag is intentional, allowing updates to the lock file when generating the API client. Verify that this behavior is acceptable for the client generation process.webapp/src/app/user/issue-card/issue-card.component.ts (1)
10-10
:⚠️ Potential issue✅ Verification successful
Update code to use the safer
formatTitle
implementation.The component needs to be updated to work with the safer
formatTitle
implementation that usesDomSanitizer
.Apply these changes:
import { Component, computed, input } from '@angular/core'; +import { DomSanitizer } from '@angular/platform-browser'; import { formatTitle } from '@app/utils'; @Component({ // ... other metadata }) export class IssueCardComponent { + constructor(private sanitizer: DomSanitizer) {} // ... other properties - displayTitle = computed(() => formatTitle(this.title() ?? '')); + displayTitle = computed(() => formatTitle(this.title() ?? '', this.sanitizer)); }Please ensure that the template uses
[innerHTML]
to render the formatted title:Also applies to: 37-37
🏁 Script executed:
#!/bin/bash # Description: Check if the template uses innerHTML for the formatted title rg -l "displayTitle" webapp/src/app/user/issue-card/Length of output: 164
🏁 Script executed:
#!/bin/bash # Check if issue-card.component.html uses [innerHTML] for the displayTitle binding. rg "\[innerHTML\].*displayTitle" webapp/src/app/user/issue-card/issue-card.component.htmlLength of output: 140
Update IssueCardComponent to Utilize the Safer
formatTitle
with DomSanitizer
- In webapp/src/app/user/issue-card/issue-card.component.ts:
- Import
DomSanitizer
from@angular/platform-browser
.- Inject
DomSanitizer
via the component constructor.- Update the
displayTitle
computed property to pass the sanitizer toformatTitle
:import { Component, computed, input } from '@angular/core'; +import { DomSanitizer } from '@angular/platform-browser'; import { formatTitle } from '@app/utils'; @Component({ // ... other metadata }) export class IssueCardComponent { + constructor(private sanitizer: DomSanitizer) {} // ... other properties - displayTitle = computed(() => formatTitle(this.title() ?? '')); + displayTitle = computed(() => formatTitle(this.title() ?? '', this.sanitizer)); }- In webapp/src/app/user/issue-card/issue-card.component.html:
- Confirmed that the template uses
[innerHTML]
for rendering with:<div [innerHTML]="displayTitle()"></div>All changes have been validated. Please ensure that similar updates (if any) in related sections (e.g., line 37) are also applied accordingly.
.vscode/settings.json (1)
20-22
: Retention of Pytest Testing Configuration:
The setting"python.testing.pytestEnabled": true
remains enabled, which is appropriate to ensure that tests run with pytest. This configuration is clear and supports the testing framework as expected.webapp/src/app/app.routes.ts (2)
16-16
: LGTM!The import statement is correctly formatted and aligns with the PR objectives.
51-51
: LGTM!The route path and guards are correctly configured, aligning with both the PR objectives and the suggested path structure from past review comments.
server/intelligence-service/pyproject.toml (3)
1-8
: LGTM!The project metadata section is well-structured and follows the standard format.
26-38
: LGTM!The poetry configuration is well-structured and follows standard practices.
10-10
:❓ Verification inconclusive
Verify Python version requirement.
The requirement for Python 3.12 is quite recent. Please ensure that:
- All team members and deployment environments can support this version.
- All dependencies are compatible with Python 3.12.
🌐 Web query:
What is the current stable version of Python and when was Python 3.12 released?
💡 Result:
The current stable version of Python is 3.13.2, released on February 4, 2025[1][5][9]. This maintenance update includes nearly 250 bug fixes and improvements since Python 3.13.1[1].
For Python 3.12 specifically:
- Initial stable release: October 2, 2023 (version 3.12.0)[6][8]
- Latest maintenance release: 3.12.9 (February 4, 2025)[5], though earlier notable updates include:
- Version 3.12.5 (August 6, 2024) with over 250 fixes[2]
- Version 3.12.6 (September 6, 2024) addressing critical security vulnerabilities[4]
Python 3.12 remains in active bugfix support until April 2025 and will receive security updates until October 2028[5]. The version introduced significant improvements including enhanced f-string parsing, better error messages, and performance optimizations through PEPs like 709 and 684[10].
Citations:
- 1: https://www.python.org/downloads/release/python-3132/
- 2: https://www.python.org/downloads/release/python-3125/
- 3: https://devguide.python.org/versions/
- 4: https://www.python.org/downloads/release/python-3126/
- 5: https://endoflife.date/python
- 6: https://www.python.org/downloads/release/python-3120/
- 7: https://www.python.org/downloads/
- 8: https://peps.python.org/pep-0693/
- 9: https://www.python.org/downloads/windows/
- 10: https://www.python.org/downloads/release/python-3123/
Action Required: Verify Python Version Requirement Compatibility
The Python requirement (
requires-python = ">=3.12,<4.0.0"
) specifies a minimum of Python 3.12, which—while production ready (initially released in October 2023 and maintained until April 2025)—may still present challenges if team environments or dependencies haven’t fully adopted it. Note that the current stable version is Python 3.13.2. Please ensure that:
- Development & Deployment: All team members’ environments, CI/CD pipelines, and deployment targets are equipped to support Python 3.12 and above.
- Dependency Compatibility: Every dependency has been verified to operate correctly with Python 3.12—and ideally with newer releases like 3.13.2—to avoid runtime issues.
If any gaps are found, consider revising the version constraint or updating the environments/dependencies accordingly.
@@ -24,7 +24,7 @@ jobs: | |||
- name: Install Poetry | |||
uses: snok/install-poetry@v1 | |||
with: | |||
version: 1.8.5 | |||
version: 2.1.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Correct YAML Indentation for Poetry Version Configuration
YAMLlint warns about incorrect indentation (expected 12 spaces but found 10) for the version
key. Please update the indentation as shown below to ensure proper YAML formatting:
- version: 2.1.1
+ version: 2.1.1
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
version: 2.1.1 | |
version: 2.1.1 |
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 27-27: wrong indentation: expected 12 but found 10
(indentation)
export function formatTitle(title: string): string { | ||
return title.replace(/`([^`]+)`/g, '<code class="textCode">$1</code>'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address potential XSS vulnerabilities and edge cases.
The current implementation has the following concerns:
- Direct HTML injection via string replacement could lead to XSS vulnerabilities.
- No handling of edge cases like unmatched or nested backticks.
Consider this safer implementation:
-export function formatTitle(title: string): string {
- return title.replace(/`([^`]+)`/g, '<code class="textCode">$1</code>');
-}
+import { DomSanitizer, SafeHtml } from '@angular/platform-browser';
+
+export function formatTitle(title: string, sanitizer: DomSanitizer): SafeHtml {
+ // Handle unmatched backticks by counting them
+ const count = (title.match(/`/g) || []).length;
+ if (count % 2 !== 0) {
+ return sanitizer.bypassSecurityTrustHtml(title);
+ }
+
+ // Escape HTML entities in the content
+ const escaped = title.replace(/[&<>"']/g, char => ({
+ '&': '&',
+ '<': '<',
+ '>': '>',
+ '"': '"',
+ "'": '''
+ }[char]));
+
+ // Replace backticked content with code elements
+ const formatted = escaped.replace(/`([^`]+)`/g, '<code class="textCode">$1</code>');
+
+ return sanitizer.bypassSecurityTrustHtml(formatted);
+}
Add JSDoc comments to document the function's purpose and parameters:
/**
* Formats a title string by converting backtick-enclosed text into HTML code elements.
* @param title - The title string to format
* @param sanitizer - Angular's DomSanitizer instance for XSS protection
* @returns A sanitized HTML string with backticked text wrapped in code elements
*/
@@ -46,5 +47,7 @@ | |||
{ path: 'user/:id', component: UserProfileComponent, canActivate: [AuthGuard] }, | |||
{ path: 'settings', component: SettingsComponent, canActivate: [AuthGuard] }, | |||
{ path: 'mentor', component: MentorComponent, canActivate: [AuthGuard, MentorGuard] }, | |||
{ path: 'workspace', component: WorkspaceComponent, canActivate: [AuthGuard, AdminGuard] } | |||
{ path: 'workspace', component: WorkspaceComponent, canActivate: [AuthGuard, AdminGuard] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove duplicate workspace route.
This route appears to be a duplicate of the existing workspace route defined at lines 23-40. Having duplicate routes can lead to routing conflicts.
- { path: 'workspace', component: WorkspaceComponent, canActivate: [AuthGuard, AdminGuard] },
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
{ path: 'workspace', component: WorkspaceComponent, canActivate: [AuthGuard, AdminGuard] }, | |
// (This route has been removed as it was a duplicate) |
{ path: 'user/:id/activity', component: ActivityDashboardComponent, canActivate: [AuthGuard, AdminGuard] } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix formatting: remove extra newline.
Remove the extra newline to fix the formatting issue flagged by the pipeline.
- { path: 'user/:id/activity', component: ActivityDashboardComponent, canActivate: [AuthGuard, AdminGuard] }
-
+ { path: 'user/:id/activity', component: ActivityDashboardComponent, canActivate: [AuthGuard, AdminGuard] }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
{ path: 'user/:id/activity', component: ActivityDashboardComponent, canActivate: [AuthGuard, AdminGuard] } | |
{ path: 'user/:id/activity', component: ActivityDashboardComponent, canActivate: [AuthGuard, AdminGuard] } |
🧰 Tools
🪛 GitHub Check: Code Quality Checks
[failure] 52-52:
Delete ⏎
🪛 GitHub Actions: Webapp QA
[error] 52-52: Delete ⏎
prettier/prettier
dependencies = [ | ||
"fastapi[standard] == 0.115.6", | ||
"pydantic-settings == 2.6.1", | ||
"langchain == 0.3.10", | ||
"langchain-openai == 0.2.11", | ||
"langgraph == 0.2.56", | ||
"langchain-community == 0.3.10", | ||
"langgraph-checkpoint-postgres (>=2.0.8,<3.0.0)", | ||
"langchain-postgres (>=0.0.12,<0.1.0)", | ||
"psycopg-pool (>=3.2.4,<4.0.0)", | ||
"ipython (>=8.31.0,<9.0.0)", | ||
"psycopg[binary,c] (>=3.2.4,<4.0.0)", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify dependency versions and security advisories.
Please verify that:
- The specified versions are the latest stable releases.
- There are no known security vulnerabilities.
🏁 Script executed:
#!/bin/bash
# Description: Check for latest versions and security advisories of Python packages.
# Check PyPI for latest versions
for package in fastapi pydantic-settings langchain langchain-openai langgraph langchain-community langgraph-checkpoint-postgres langchain-postgres psycopg-pool ipython psycopg; do
echo "Checking $package..."
curl -s "https://pypi.org/pypi/$package/json" | jq -r '.info.version'
done
# Check for security advisories
gh api graphql -f query='
query {
securityVulnerabilities(first: 10, ecosystem: PIP, package: "fastapi") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 4303
Update dependency versions and synchronize langchain packages
After verifying against PyPI, several dependencies in server/intelligence-service/pyproject.toml
are behind the latest stable releases. In particular:
- fastapi[standard]: currently pinned to 0.115.6 while the latest is 0.115.8.
- pydantic-settings: currently 2.6.1 vs 2.7.1 available.
- langchain: currently 0.3.10 versus the latest 0.3.19.
- langchain-openai: currently 0.2.11 while 0.3.6 is available.
- langgraph: currently 0.2.56 while 0.2.74 is available.
- langchain-community: currently 0.3.10 while 0.3.18 is available.
The remaining dependencies use flexible version ranges that already accommodate the latest fixes.
I recommend updating the exact pinned dependencies to their latest stable versions where possible. Doing so not only leverages recent bug fixes and improvements but also helps in keeping the langchain ecosystem packages synchronized to avoid compatibility issues. Note that the security advisories (e.g., for FastAPI vulnerabilities affecting versions < 0.65.2 and <= 0.109.0) do not impact your current configuration, but updating may still bring additional security enhancements.
Motivation
We create a dashboard that displays current pull requests together with detected bad practices
Description
Create an activity endpoint to get and detect bad practices in PRs.
Visualize PRs and bad practices in a dashboard
Testing Instructions
Run application server and client.
Access: http://localhost:4200/activity/iam-flo
Screenshots (if applicable)
General
Client (if applicable)
Server (if applicable)
Summary by CodeRabbit