feat(server): finding feedback entity and API for contributor reactions - #911
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an append-only FindingFeedback model, DB migration, workspace-scoped REST endpoints, service/repository logic, DTOs, frontend SDK/react-query wrappers, and tests to record and aggregate contributor reactions (APPLIED, DISPUTED, NOT_APPLICABLE) to practice findings. Changes
Sequence Diagram(s)sequenceDiagram
participant Web as Web Client
participant SDK as WebApp SDK
participant Ctrl as FindingFeedbackController
participant Svc as FindingFeedbackService
participant Repo as FindingFeedbackRepository
participant DB as Database
Web->>SDK: submitFeedback(payload)
SDK->>Ctrl: POST /workspaces/{ws}/practices/findings/{id}/feedback
Ctrl->>Svc: submitFeedback(workspaceCtx, findingId, dto)
Svc->>Repo: save(new FindingFeedback...)
Repo->>DB: INSERT INTO finding_feedback (...)
DB-->>Repo: OK (id, created_at)
Repo-->>Svc: persisted entity
Svc-->>Ctrl: FindingFeedbackDTO
Ctrl-->>SDK: 201 Created (Location + body)
SDK-->>Web: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
documentation
Contributor
📚 Documentation Preview
|
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces an append-only “finding feedback” capability in the practices module, allowing contributors to react to AI-generated practice findings (APPLIED / DISPUTED / NOT_APPLICABLE) via a workspace-scoped REST API, with supporting persistence, migrations, and regenerated clients/docs.
Changes:
- Added
FindingFeedbackdomain model (entity, repository, service) and workspace-scoped controller endpoints for submit/latest/engagement. - Added Liquibase migration creating
finding_feedbacktable, constraints, and indexes; extendedPracticeFindingRepositorywith workspace-scoped lookup. - Regenerated OpenAPI + webapp TypeScript SDK/TanStack Query helpers and updated ERD; added unit + integration test suites.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| webapp/src/api/types.gen.ts | Adds generated TS types for finding feedback DTOs and new endpoints. |
| webapp/src/api/transformers.gen.ts | Adds generated response transformer for submitFeedback (date parsing). |
| webapp/src/api/sdk.gen.ts | Adds generated SDK calls for engagement/latest/submit endpoints. |
| webapp/src/api/index.ts | Re-exports new SDK functions and types. |
| webapp/src/api/@tanstack/react-query.gen.ts | Adds generated query keys/options + mutation helper for feedback endpoints. |
| server/intelligence-service/src/shared/db/schema.ts | Adds introspected Drizzle table definition for finding_feedback. |
| server/application-server/src/main/java/.../finding/feedback/FindingFeedback.java | New immutable append-only JPA entity for feedback events. |
| server/application-server/src/main/java/.../finding/feedback/FindingFeedbackAction.java | New enum defining the 3 feedback actions. |
| server/application-server/src/main/java/.../finding/feedback/FindingFeedbackRepository.java | New repository with “latest per finding” native query + workspace-scoped engagement aggregation. |
| server/application-server/src/main/java/.../finding/feedback/FindingFeedbackService.java | New service enforcing contributor-only submission + disputed explanation rule + engagement stats. |
| server/application-server/src/main/java/.../finding/feedback/FindingFeedbackController.java | New workspace-scoped REST API endpoints for feedback submission, retrieval, and engagement. |
| server/application-server/src/main/java/.../dto/CreateFindingFeedbackDTO.java | New request DTO with validation + OpenAPI schema metadata. |
| server/application-server/src/main/java/.../dto/FindingFeedbackDTO.java | New response DTO for feedback events. |
| server/application-server/src/main/java/.../dto/FindingFeedbackEngagementDTO.java | New response DTO for engagement counts. |
| server/application-server/src/main/resources/db/master.xml | Registers the new Liquibase changelog file. |
| server/application-server/src/main/resources/db/changelog/1774335316041_changelog.xml | Creates finding_feedback table, FKs, CHECKs, and indexes. |
| server/application-server/src/main/java/.../PracticeFindingRepository.java | Adds findByIdAndWorkspaceId for workspace-scoped finding lookup. |
| server/application-server/src/test/java/.../FindingFeedbackServiceTest.java | Unit tests for submit/latest/engagement/latest-by-ids behaviors. |
| server/application-server/src/test/java/.../FindingFeedbackControllerIntegrationTest.java | Integration tests for REST behavior, auth, append-only semantics, and workspace isolation. |
| server/application-server/src/test/java/.../ActivityModuleBoundaryTest.java | Allows FindingFeedbackController as a practices-module REST entry point. |
| server/application-server/openapi.yaml | Adds OpenAPI paths/schemas for finding feedback endpoints. |
| docs/contributor/erd/schema.mmd | Updates ERD to include FindingFeedback entity and relations. |
Comment on lines
+321
to
+324
| export const submitFeedbackResponseTransformer = async (data: any): Promise<SubmitFeedbackResponse> => { | ||
| data = findingFeedbackSchemaResponseTransformer(data); | ||
| return data; | ||
| }; |
There was a problem hiding this comment.
FindingFeedback.createdAt is typed as Date, but only submitFeedbackResponseTransformer applies the date conversion. getLatestFeedback returns the same FindingFeedback schema and will currently leave createdAt as an ISO string unless callers manually provide a transformer, leading to runtime/type mismatch. Add a corresponding response transformer for getLatestFeedback (and ensure it’s used by the generated query/sdk helpers) so both endpoints deserialize consistently.
| export const findingFeedback = pgTable( | ||
| "finding_feedback", | ||
| { | ||
| id: uuid().defaultRandom().primaryKey().notNull(), |
There was a problem hiding this comment.
This Drizzle schema declares id with defaultRandom() (DB default gen_random_uuid()), but the Liquibase changeset creating finding_feedback.id does not set any default. This divergence can cause schema drift between the application-server migrations and the intelligence-service introspected schema. Either add the DB default in the Liquibase migration or regenerate schema.ts from a DB that was created solely from Liquibase migrations.
Suggested change
| id: uuid().defaultRandom().primaryKey().notNull(), | |
| id: uuid().primaryKey().notNull(), |
| table.contributorId.asc().nullsLast(), | ||
| table.createdAt.desc().nullsFirst(), | ||
| ), | ||
| index("idx_finding_feedback_finding").using("btree", table.findingId.asc().nullsLast()), |
There was a problem hiding this comment.
schema.ts defines an idx_finding_feedback_finding index, but the Liquibase migration for finding_feedback only creates idx_finding_feedback_contributor_created and idx_finding_feedback_finding_contributor. If this index is not actually present in the DB after migrations, the intelligence-service schema is out of sync. Either add the missing Liquibase changeset for this index or re-introspect after applying the final Liquibase migrations.
Suggested change
| index("idx_finding_feedback_finding").using("btree", table.findingId.asc().nullsLast()), |
| check( | ||
| "chk_finding_feedback_action", | ||
| sql`(action)::text = ANY ((ARRAY['APPLIED'::character varying, 'DISPUTED'::character varying, 'NOT_APPLICABLE'::character varying])::text[])`, | ||
| ), |
There was a problem hiding this comment.
Liquibase adds a second CHECK constraint (chk_finding_feedback_disputed_explanation) requiring a non-blank explanation when action = 'DISPUTED', but the introspected Drizzle schema only contains the action-enum check. This mismatch is a sign the introspection output isn’t reflecting the final DB schema; re-run introspection after applying migrations, or ensure the DB constraint exists so downstream tooling/tests don’t diverge from production.
Suggested change
| ), | |
| ), | |
| check( | |
| "chk_finding_feedback_disputed_explanation", | |
| sql`(action)::text <> 'DISPUTED'::text OR btrim(coalesce(explanation, '')) <> ''`, | |
| ), |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| } | ||
| ) | ||
| @Getter | ||
| @Builder |
![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: 6
🧹 Nitpick comments (5)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/CreateFindingFeedbackDTO.java (1)
15-21: Annotate the requiredactioncomponent with@NonNull.
@NotNullcovers validation, but this DTO still misses the repo'sorg.springframework.lang.NonNullmarker for required record components.explanationcan stay bare because it's optional.As per coding guidelines, "
server/application-server/src/main/java/**/*DTO.java: Annotate record components in DTOs withorg.springframework.lang.NonNullwhen the API requires a value; leave optional fields bare in the application server."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/CreateFindingFeedbackDTO.java` around lines 15 - 21, Add the org.springframework.lang.NonNull marker to the required record component in CreateFindingFeedbackDTO: annotate the action component (FindingFeedbackAction action) with `@NonNull` in addition to the existing `@NotNull` so the DTO follows the repo guideline for required record components; leave explanation unannotated since it is optional.server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/FindingFeedbackDTO.java (1)
13-21: Mark the required response fields as@NonNull.
id,findingId,action, andcreatedAtare all documented as required, so they should also carryorg.springframework.lang.NonNull.explanationcan stay unannotated.As per coding guidelines, "
server/application-server/src/main/java/**/*DTO.java: Annotate record components in DTOs withorg.springframework.lang.NonNullwhen the API requires a value; leave optional fields bare in the application server."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/FindingFeedbackDTO.java` around lines 13 - 21, Update the FindingFeedbackDTO record to annotate required components with org.springframework.lang.NonNull: add `@NonNull` to the components id, findingId, action, and createdAt (leave explanation unannotated). Locate the record declaration for FindingFeedbackDTO and apply the `@NonNull` annotation to those four components so the DTO contract matches the `@Schema` requiredMode.server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackServiceTest.java (1)
83-83: Rename the new test methods to the repo convention.The
@DisplayNames are clear, but method names likeappliedFeedbackSavesandreturnsLatestWhenPresentstill deviate fromshould[ExpectedBehavior]When[Condition], which makes the suite less consistent to navigate.As per coding guidelines, "
server/application-server/**/*Test.java: Useshould[ExpectedBehavior]When[Condition]naming convention for test methods."Also applies to: 107-107, 126-126, 147-147, 160-160, 173-173, 186-186, 204-204, 230-230, 245-245, 262-262, 301-301, 323-323, 331-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackServiceTest.java` at line 83, Rename the test methods to follow the repo convention should[ExpectedBehavior]When[Condition]; specifically rename appliedFeedbackSaves, returnsLatestWhenPresent, and the other listed test methods (lines showing methods at 107,126,147,160,173,186,204,230,245,262,301,323,331) to descriptive names like shouldSaveAppliedFeedbackWhen[Condition] and shouldReturnLatestWhenPresentWhen[Condition] (replace [ExpectedBehavior] and [Condition] with the concrete behavior/condition described by each `@DisplayName`). Update the method identifiers (e.g., appliedFeedbackSaves, returnsLatestWhenPresent, etc.) in FindingFeedbackServiceTest to match the should...When... pattern while keeping their `@DisplayName` annotations unchanged and ensuring any references (imports, test-suite runners) still compile.server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackRepository.java (1)
19-21: Don't exempt the whole repository from workspace-scope checks.Applying
@WorkspaceAgnosticat the type level also suppresses architecture validation forcountByContributorAndWorkspaceGroupByAction(...), even though that method is workspace-scoped. Move the annotation to the two intentionally unscoped lookup methods instead.♻️ Suggested annotation scope
`@Repository` -@WorkspaceAgnostic("Feedback scoped through PracticeFinding -> Practice.workspace relationship") public interface FindingFeedbackRepository extends JpaRepository<FindingFeedback, UUID> { + `@WorkspaceAgnostic`("Caller already workspace-scopes the single finding id") Optional<FindingFeedback> findFirstByFindingIdAndContributorIdOrderByCreatedAtDesc( UUID findingId, Long contributorId ); + `@WorkspaceAgnostic`("Caller already workspace-scopes the provided finding ids") `@Query`( value = """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackRepository.java` around lines 19 - 21, The repository-level `@WorkspaceAgnostic` on FindingFeedbackRepository wrongly exempts all methods (including workspace-scoped countByContributorAndWorkspaceGroupByAction(...)); remove the `@WorkspaceAgnostic` from the interface declaration and instead annotate only the two specific lookup methods that are intended to be workspace-agnostic (leave countByContributorAndWorkspaceGroupByAction(...) and any other workspace-scoped methods without `@WorkspaceAgnostic`) so architecture validation still applies to scoped queries.server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackService.java (1)
19-21: Use Lombok@Slf4jfor service logging consistency.This service uses manual
LoggerFactoryinstead of@Slf4j, which diverges from the project logging convention.Suggested refactor
-import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; @@ `@Service` `@Transactional` `@RequiredArgsConstructor` +@Slf4j public class FindingFeedbackService { - - private static final Logger log = LoggerFactory.getLogger(FindingFeedbackService.class);As per coding guidelines, "Use
@Slf4jlogging with parameterized log messages instead ofSystem.out.println()" and "*Service.java: Use@Slf4jLombok annotation for logging and parameterized log messages with context".Also applies to: 41-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackService.java` around lines 19 - 21, The class FindingFeedbackService currently creates a manual Logger via LoggerFactory instead of using Lombok's `@Slf4j`; replace the manual logger import and field with the Lombok annotation by adding import lombok.extern.slf4j.Slf4j and annotating the FindingFeedbackService class with `@Slf4j`, then update all logging calls in FindingFeedbackService to use the generated log instance (and convert any string concatenation to parameterized messages) to match the project's logging convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackController.java`:
- Around line 52-66: The OpenAPI responses for submitFeedback and
getLatestFeedback currently hide error schemas causing clients to see unknown
types; update the `@ApiResponse` annotations on submitFeedback (the 400, 403 and
404 responses) and the 404 response on getLatestFeedback to use
`@Schema`(implementation = ProblemDetail.class) instead of `@Schema`(hidden = true)
so the generated contract reflects the RFC-7807 ProblemDetail produced by
GlobalControllerAdvice for EntityNotFoundException, AccessForbiddenException and
IllegalArgumentException.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackRepository.java`:
- Around line 57-71: The current repository query
countByContributorAndWorkspaceGroupByAction aggregates over all FindingFeedback
rows and will double-count when contributors change feedback; modify the query
in FindingFeedbackRepository so it only aggregates the latest feedback row per
(finding, contributor) before grouping by action. Implement this by restricting
ff to only rows whose id is the max id per finding for that contributor (e.g.,
add "AND ff.id IN (SELECT MAX(ff2.id) FROM FindingFeedback ff2 WHERE
ff2.contributorId = :contributorId GROUP BY ff2.finding)" or an equivalent
subquery using timestamp) while keeping the joins to ff.finding f and f.practice
p and retaining the method name countByContributorAndWorkspaceGroupByAction and
parameters contributorId/workspaceId.
- Around line 25-49: The current "latest feedback" selection only orders by
created_at which can tie across nodes; update both lookups to use a
deterministic, database-monotonic tie-breaker (the DB-generated primary key) in
the ORDER BY. For the native query in findLatestByFindingIdsAndContributor add
the id as a secondary sort (e.g. ORDER BY ff.finding_id, ff.created_at DESC,
ff.id DESC), and for the derived Spring Data method
findFirstByFindingIdAndContributorIdOrderByCreatedAtDesc either change it to an
explicit `@Query` with ORDER BY created_at DESC, id DESC or rename it to include
the id tie-breaker (e.g.
findFirstByFindingIdAndContributorIdOrderByCreatedAtDescIdDesc) so the
repository uses created_at then id to deterministically break ties.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackService.java`:
- Around line 102-111: getLatestFeedback currently skips the workspace ownership
check, allowing existence probing; update getLatestFeedback to first verify the
Finding belongs to the workspace just like submitFeedback does by calling
findingRepository.findByIdAndWorkspaceId(findingId, workspaceContext.id()) and
orElseThrow(new EntityNotFoundException(...)) before fetching feedback, or
extract that verification into the same helper used by submitFeedback so
getLatestFeedback uses the same ownership check (keep the throw type and message
consistent).
In
`@server/application-server/src/main/resources/db/changelog/1774335316041_changelog.xml`:
- Around line 33-35: The sort on FindingFeedback uses only created_at so
concurrent rows with identical TIMESTAMP(6) tie-break nondeterministically; add
a monotonic secondary column (e.g., insertion_order BIGINT GENERATED ALWAYS AS
IDENTITY or use a monotonic numeric id if available) to the FindingFeedback
table via the changelog XML (add column like insertion_order with NOT NULL and
DB-generated sequence), update the FindingFeedback entity indexes to include
this new column (update idx_finding_feedback_finding_contributor and related
index definitions to include insertion_order), and change repository ordering to
a deterministic composite order (for
findFirstByFindingIdAndContributorIdOrderByCreatedAtDesc and the native query in
findLatestByFindingIdsAndContributor) to ORDER BY created_at DESC,
insertion_order DESC (or created_at DESC, id DESC if you choose to rely on a
monotonic id) so the latest-feedback selection is stable.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackControllerIntegrationTest.java`:
- Around line 68-83: Tests rely on fixed slugs and a global
feedbackRepository.findAll().hasSize(2), which makes them brittle; change usages
of hardcoded identifiers like "feedback-ws", "test-practice", "other-ws" to
unique per-test values (e.g. append a UUID or test-specific suffix when calling
persistUser/createWorkspace and when setting Practice.slug), and replace
assertions against feedbackRepository.findAll() with assertions scoped to the
entities created in this test (e.g. query feedbacks by
workspace/practice/finding id or filter the repository result for the created
workspace/practice), updating the setup code that uses persistUser,
createWorkspace, ensureAdminMembership, Practice/practiceRepository.save and the
assertions at the referenced sections (including the other occurrences around
lines noted) accordingly.
---
Nitpick comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/CreateFindingFeedbackDTO.java`:
- Around line 15-21: Add the org.springframework.lang.NonNull marker to the
required record component in CreateFindingFeedbackDTO: annotate the action
component (FindingFeedbackAction action) with `@NonNull` in addition to the
existing `@NotNull` so the DTO follows the repo guideline for required record
components; leave explanation unannotated since it is optional.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/FindingFeedbackDTO.java`:
- Around line 13-21: Update the FindingFeedbackDTO record to annotate required
components with org.springframework.lang.NonNull: add `@NonNull` to the components
id, findingId, action, and createdAt (leave explanation unannotated). Locate the
record declaration for FindingFeedbackDTO and apply the `@NonNull` annotation to
those four components so the DTO contract matches the `@Schema` requiredMode.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackRepository.java`:
- Around line 19-21: The repository-level `@WorkspaceAgnostic` on
FindingFeedbackRepository wrongly exempts all methods (including
workspace-scoped countByContributorAndWorkspaceGroupByAction(...)); remove the
`@WorkspaceAgnostic` from the interface declaration and instead annotate only the
two specific lookup methods that are intended to be workspace-agnostic (leave
countByContributorAndWorkspaceGroupByAction(...) and any other workspace-scoped
methods without `@WorkspaceAgnostic`) so architecture validation still applies to
scoped queries.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackService.java`:
- Around line 19-21: The class FindingFeedbackService currently creates a manual
Logger via LoggerFactory instead of using Lombok's `@Slf4j`; replace the manual
logger import and field with the Lombok annotation by adding import
lombok.extern.slf4j.Slf4j and annotating the FindingFeedbackService class with
`@Slf4j`, then update all logging calls in FindingFeedbackService to use the
generated log instance (and convert any string concatenation to parameterized
messages) to match the project's logging convention.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackServiceTest.java`:
- Line 83: Rename the test methods to follow the repo convention
should[ExpectedBehavior]When[Condition]; specifically rename
appliedFeedbackSaves, returnsLatestWhenPresent, and the other listed test
methods (lines showing methods at
107,126,147,160,173,186,204,230,245,262,301,323,331) to descriptive names like
shouldSaveAppliedFeedbackWhen[Condition] and
shouldReturnLatestWhenPresentWhen[Condition] (replace [ExpectedBehavior] and
[Condition] with the concrete behavior/condition described by each
`@DisplayName`). Update the method identifiers (e.g., appliedFeedbackSaves,
returnsLatestWhenPresent, etc.) in FindingFeedbackServiceTest to match the
should...When... pattern while keeping their `@DisplayName` annotations unchanged
and ensuring any references (imports, test-suite runners) still compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3a7cd3d-bd06-490a-abd7-52f2c11867e0
📒 Files selected for processing (22)
docs/contributor/erd/schema.mmdserver/application-server/openapi.yamlserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedback.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackAction.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackController.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/CreateFindingFeedbackDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/FindingFeedbackDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/FindingFeedbackEngagementDTO.javaserver/application-server/src/main/resources/db/changelog/1774335316041_changelog.xmlserver/application-server/src/main/resources/db/master.xmlserver/application-server/src/test/java/de/tum/in/www1/hephaestus/architecture/ActivityModuleBoundaryTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackControllerIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackServiceTest.javaserver/intelligence-service/src/shared/db/schema.tswebapp/src/api/@tanstack/react-query.gen.tswebapp/src/api/index.tswebapp/src/api/sdk.gen.tswebapp/src/api/transformers.gen.tswebapp/src/api/types.gen.ts
Comment on lines
+52
to
+66
| @ApiResponse( | ||
| responseCode = "400", | ||
| description = "Invalid request (e.g., DISPUTED without explanation)", | ||
| content = @Content(schema = @Schema(hidden = true)) | ||
| ) | ||
| @ApiResponse( | ||
| responseCode = "403", | ||
| description = "Current user is not the finding's contributor", | ||
| content = @Content(schema = @Schema(hidden = true)) | ||
| ) | ||
| @ApiResponse( | ||
| responseCode = "404", | ||
| description = "Finding not found in this workspace", | ||
| content = @Content(schema = @Schema(hidden = true)) | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "FindingFeedbackController.java" -type fRepository: ls1intum/Hephaestus
Length of output: 188
🏁 Script executed:
head -100 server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackController.javaRepository: ls1intum/Hephaestus
Length of output: 4430
🏁 Script executed:
# Search for RestControllerAdvice in the codebase
rg "@RestControllerAdvice" --type javaRepository: ls1intum/Hephaestus
Length of output: 572
🏁 Script executed:
# Look for ProblemDetail usage
rg "ProblemDetail" --type javaRepository: ls1intum/Hephaestus
Length of output: 19932
🏁 Script executed:
# Check the error handling documentation
find . -name "api-error-handling.md" -o -name "*error*handling*" -type fRepository: ls1intum/Hephaestus
Length of output: 185
🏁 Script executed:
cat docs/contributor/api-error-handling.mdRepository: ls1intum/Hephaestus
Length of output: 5529
🏁 Script executed:
cat server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/PracticesControllerAdvice.javaRepository: ls1intum/Hephaestus
Length of output: 1450
🏁 Script executed:
# Check if there are other controllers in the practices domain and how they handle ApiResponse
rg "@ApiResponse" server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices --type java -A 2 | head -60Repository: ls1intum/Hephaestus
Length of output: 7425
🏁 Script executed:
# Check what exceptions the FindingFeedbackService throws
rg "throw|Exception" server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback --type java | head -30Repository: ls1intum/Hephaestus
Length of output: 1961
🏁 Script executed:
# Look for any custom exception classes in the finding/feedback package
find server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback -name "*Exception.java" -type fRepository: ls1intum/Hephaestus
Length of output: 45
🏁 Script executed:
# Check the complete FindingFeedbackController to see all error handling
sed -n '52,100p' server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackController.javaRepository: ls1intum/Hephaestus
Length of output: 2028
🏁 Script executed:
# Verify how other controllers document ProblemDetail in error responses
rg "@ApiResponse.*40[0-9]" -A 3 server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace --type java | head -40Repository: ls1intum/Hephaestus
Length of output: 2922
🏁 Script executed:
# Check how other practices controllers document error responses
cat server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/PracticesController.java | grep -A 10 "@ApiResponse" | head -50Repository: ls1intum/Hephaestus
Length of output: 2204
🏁 Script executed:
# Check GlobalControllerAdvice to see what it handles
cat server/application-server/src/main/java/de/tum/in/www1/hephaestus/core/exception/GlobalControllerAdvice.javaRepository: ls1intum/Hephaestus
Length of output: 7666
Expose ProblemDetail schema for error responses to match centralized exception handling.
The submitFeedback endpoint (lines 52-66) throws EntityNotFoundException, AccessForbiddenException, and IllegalArgumentException, all of which are handled by GlobalControllerAdvice and automatically converted to RFC-7807 ProblemDetail responses. However, @Schema(hidden = true) suppresses these schemas from the OpenAPI contract, causing client generators to type errors as unknown instead of the documented ProblemDetail structure.
Replace @Schema(hidden = true) with @Schema(implementation = ProblemDetail.class) for the 400, 403, and 404 responses to match the actual API behavior and the guidance in docs/contributor/api-error-handling.md.
Suggested change
`@ApiResponse`(
responseCode = "400",
description = "Invalid request (e.g., DISPUTED without explanation)",
- content = `@Content`(schema = `@Schema`(hidden = true))
+ content = `@Content`(schema = `@Schema`(implementation = ProblemDetail.class))
)
`@ApiResponse`(
responseCode = "403",
description = "Current user is not the finding's contributor",
- content = `@Content`(schema = `@Schema`(hidden = true))
+ content = `@Content`(schema = `@Schema`(implementation = ProblemDetail.class))
)
`@ApiResponse`(
responseCode = "404",
description = "Finding not found in this workspace",
- content = `@Content`(schema = `@Schema`(hidden = true))
+ content = `@Content`(schema = `@Schema`(implementation = ProblemDetail.class))
)Also applies to the getLatestFeedback endpoint at lines 92-96 (404 response only; 204 No Content may remain hidden since it has no body).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackController.java`
around lines 52 - 66, The OpenAPI responses for submitFeedback and
getLatestFeedback currently hide error schemas causing clients to see unknown
types; update the `@ApiResponse` annotations on submitFeedback (the 400, 403 and
404 responses) and the 404 response on getLatestFeedback to use
`@Schema`(implementation = ProblemDetail.class) instead of `@Schema`(hidden = true)
so the generated contract reflects the RFC-7807 ProblemDetail produced by
GlobalControllerAdvice for EntityNotFoundException, AccessForbiddenException and
IllegalArgumentException.
Comment on lines
+25
to
+49
| Optional<FindingFeedback> findFirstByFindingIdAndContributorIdOrderByCreatedAtDesc( | ||
| UUID findingId, | ||
| Long contributorId | ||
| ); | ||
|
|
||
| /** | ||
| * Returns the latest feedback per finding for a given contributor, using PostgreSQL's | ||
| * {@code DISTINCT ON} for efficient "latest row per group" retrieval. | ||
| * | ||
| * <p>Used to enrich finding lists with the contributor's current feedback state. | ||
| */ | ||
| @Query( | ||
| value = """ | ||
| SELECT DISTINCT ON (ff.finding_id) ff.* | ||
| FROM finding_feedback ff | ||
| WHERE ff.finding_id IN (:findingIds) | ||
| AND ff.contributor_id = :contributorId | ||
| ORDER BY ff.finding_id, ff.created_at DESC | ||
| """, | ||
| nativeQuery = true | ||
| ) | ||
| List<FindingFeedback> findLatestByFindingIdsAndContributor( | ||
| @Param("findingIds") Collection<UUID> findingIds, | ||
| @Param("contributorId") Long contributorId | ||
| ); |
Contributor
There was a problem hiding this comment.
Make the "latest feedback" ordering deterministic.
Both latest lookups sort only by created_at. Because that timestamp is populated in FindingFeedback.onCreate() on the application node, rapid updates can tie or be reordered across nodes, so the chosen "current" feedback can be wrong. Use a database-sourced or otherwise monotonic ordering key, and include it in both ORDER BY clauses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackRepository.java`
around lines 25 - 49, The current "latest feedback" selection only orders by
created_at which can tie across nodes; update both lookups to use a
deterministic, database-monotonic tie-breaker (the DB-generated primary key) in
the ORDER BY. For the native query in findLatestByFindingIdsAndContributor add
the id as a secondary sort (e.g. ORDER BY ff.finding_id, ff.created_at DESC,
ff.id DESC), and for the derived Spring Data method
findFirstByFindingIdAndContributorIdOrderByCreatedAtDesc either change it to an
explicit `@Query` with ORDER BY created_at DESC, id DESC or rename it to include
the id tie-breaker (e.g.
findFirstByFindingIdAndContributorIdOrderByCreatedAtDescIdDesc) so the
repository uses created_at then id to deterministically break ties.
Comment on lines
+57
to
+71
| @Query( | ||
| """ | ||
| SELECT ff.action AS action, COUNT(ff) AS count | ||
| FROM FindingFeedback ff | ||
| JOIN ff.finding f | ||
| JOIN f.practice p | ||
| WHERE ff.contributorId = :contributorId | ||
| AND p.workspace.id = :workspaceId | ||
| GROUP BY ff.action | ||
| """ | ||
| ) | ||
| List<ActionCountProjection> countByContributorAndWorkspaceGroupByAction( | ||
| @Param("contributorId") Long contributorId, | ||
| @Param("workspaceId") Long workspaceId | ||
| ); |
Contributor
There was a problem hiding this comment.
This engagement query will overcount after feedback changes.
It groups over every FindingFeedback row in the workspace. If a contributor changes a finding from APPLIED to DISPUTED, both buckets increase, even though the append-only design says the latest row is the current state. Aggregate from the latest row per (finding, contributor) first, then group by action.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackRepository.java`
around lines 57 - 71, The current repository query
countByContributorAndWorkspaceGroupByAction aggregates over all FindingFeedback
rows and will double-count when contributors change feedback; modify the query
in FindingFeedbackRepository so it only aggregates the latest feedback row per
(finding, contributor) before grouping by action. Implement this by restricting
ff to only rows whose id is the max id per finding for that contributor (e.g.,
add "AND ff.id IN (SELECT MAX(ff2.id) FROM FindingFeedback ff2 WHERE
ff2.contributorId = :contributorId GROUP BY ff2.finding)" or an equivalent
subquery using timestamp) while keeping the joins to ff.finding f and f.practice
p and retaining the method name countByContributorAndWorkspaceGroupByAction and
parameters contributorId/workspaceId.
Comment on lines
+33
to
+35
| <column name="created_at" type="TIMESTAMP(6) WITH TIME ZONE"> | ||
| <constraints nullable="false"/> | ||
| </column> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'OrderBy.*CreatedAtDesc|createdAt|created_at|DISTINCT ON|@CreationTimestamp|Uuid|UUID' \
server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedbackRepository: ls1intum/Hephaestus
Length of output: 26707
Add a stable tie-breaker to the latest-feedback sort order.
The repository queries findFirstByFindingIdAndContributorIdOrderByCreatedAtDesc and the native query ORDER BY ff.finding_id, ff.created_at DESC both use only created_at for ordering. With TIMESTAMP(6) precision, concurrent submissions within the same microsecond will make the latest row nondeterministic, since the UUID primary key is randomly generated, not insertion-ordered. Add a secondary monotonic column (e.g., a database-generated sequence or insertion-order ID) and include it in both the indexes and the sort order, or extend the sort to use id if a monotonic ID generation strategy is in place.
Applies to:
FindingFeedbackRepository.findFirstByFindingIdAndContributorIdOrderByCreatedAtDesc()(indexidx_finding_feedback_finding_contributor)FindingFeedbackRepository.findLatestByFindingIdsAndContributor()(native query ordering)FindingFeedbackentity indexes (lines 47, 50 of schema)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/main/resources/db/changelog/1774335316041_changelog.xml`
around lines 33 - 35, The sort on FindingFeedback uses only created_at so
concurrent rows with identical TIMESTAMP(6) tie-break nondeterministically; add
a monotonic secondary column (e.g., insertion_order BIGINT GENERATED ALWAYS AS
IDENTITY or use a monotonic numeric id if available) to the FindingFeedback
table via the changelog XML (add column like insertion_order with NOT NULL and
DB-generated sequence), update the FindingFeedback entity indexes to include
this new column (update idx_finding_feedback_finding_contributor and related
index definitions to include insertion_order), and change repository ordering to
a deterministic composite order (for
findFirstByFindingIdAndContributorIdOrderByCreatedAtDesc and the native query in
findLatestByFindingIdsAndContributor) to ORDER BY created_at DESC,
insertion_order DESC (or created_at DESC, id DESC if you choose to rely on a
monotonic id) so the latest-feedback selection is stable.
Comment on lines
+68
to
+83
| // Create workspace with an owner | ||
| User owner = persistUser("feedback-owner"); | ||
| workspace = createWorkspace("feedback-ws", "Feedback WS", "feedback-org", AccountType.ORG, owner); | ||
|
|
||
| // Ensure the "admin" user (from @WithAdminUser) exists and has workspace membership | ||
| adminUser = ensureAdminMembership(workspace).getUser(); | ||
|
|
||
| // Create practice | ||
| Practice practice = new Practice(); | ||
| practice.setWorkspace(workspace); | ||
| practice.setSlug("test-practice"); | ||
| practice.setName("Test Practice"); | ||
| practice.setCategory("test"); | ||
| practice.setDescription("Test description"); | ||
| practice.setTriggerEvents(OBJECT_MAPPER.valueToTree(List.of("PullRequestCreated"))); | ||
| practice = practiceRepository.save(practice); |
Contributor
There was a problem hiding this comment.
Make these tests independent of ambient DB state.
The fixed slugs (feedback-ws, test-practice, other-ws) and the table-wide feedbackRepository.findAll().hasSize(2) assertion both assume an empty database. That will flake as soon as another integration test leaves rows behind or the suite runs in parallel. Generate unique identifiers per test and scope the count assertion to this finding instead of the whole table.
🧪 Suggested hardening
User owner = persistUser("feedback-owner");
- workspace = createWorkspace("feedback-ws", "Feedback WS", "feedback-org", AccountType.ORG, owner);
+ String suffix = UUID.randomUUID().toString();
+ workspace = createWorkspace("feedback-ws-" + suffix, "Feedback WS", "feedback-org-" + suffix, AccountType.ORG, owner);
...
- practice.setSlug("test-practice");
+ practice.setSlug("test-practice-" + suffix);
...
- assertThat(feedbackRepository.findAll()).hasSize(2);
+ long feedbackCountForFinding = feedbackRepository.findAll()
+ .stream()
+ .filter(feedback -> feedback.getFinding().getId().equals(finding.getId()))
+ .count();
+ assertThat(feedbackCountForFinding).isEqualTo(2);
...
- Workspace otherWorkspace = createWorkspace("other-ws", "Other WS", "other-org", AccountType.ORG, owner2);
+ String otherSuffix = UUID.randomUUID().toString();
+ Workspace otherWorkspace = createWorkspace(
+ "other-ws-" + otherSuffix,
+ "Other WS",
+ "other-org-" + otherSuffix,
+ AccountType.ORG,
+ owner2
+ );As per coding guidelines "Test cases may run in parallel, so avoid required cleanup steps and assume that there might be data from previous tests in the database".
Also applies to: 213-214, 374-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackControllerIntegrationTest.java`
around lines 68 - 83, Tests rely on fixed slugs and a global
feedbackRepository.findAll().hasSize(2), which makes them brittle; change usages
of hardcoded identifiers like "feedback-ws", "test-practice", "other-ws" to
unique per-test values (e.g. append a UUID or test-specific suffix when calling
persistUser/createWorkspace and when setting Practice.slug), and replace
assertions against feedbackRepository.findAll() with assertions scoped to the
entities created in this test (e.g. query feedbacks by
workspace/practice/finding id or filter the repository result for the created
workspace/practice), updating the setup code that uses persistUser,
createWorkspace, ensureAdminMembership, Practice/practiceRepository.save and the
assertions at the referenced sections (including the other occurrences around
lines noted) accordingly.
Add append-only FindingFeedback entity allowing contributors to react to AI-generated practice findings with APPLIED, DISPUTED, or NOT_APPLICABLE actions. Includes workspace-scoped REST API, typed engagement DTO, and comprehensive unit + integration tests with authorization and cross-workspace isolation coverage. Closes #898 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update Drizzle schema to reflect removed defaultRandom() on UUID PK, removed redundant idx_finding_feedback_finding index, and added chk_finding_feedback_disputed_explanation CHECK constraint. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ibase schema drift The `contributorId` was a raw Long column without @manytoone, so Hibernate didn't model the FK constraint. Liquibase diff detected the DB-only FK as drift and generated a DROP changeset, failing CI. Fix: add @manytoone(fetch=LAZY) with explicit @foreignkey name matching the Liquibase migration, and make contributorId a read-only column (same pattern as findingId). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ma drift Dual-mapped columns (via @joincolumn + @column) need matching nullable settings. Without nullable=false on the read-only @column, Hibernate generates a schema without NOT NULL, causing Liquibase diff to detect drift (dropNotNullConstraint). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
FelixTJDietrich
force-pushed
the
feat/898-finding-feedback
branch
from
March 25, 2026 06:50
d493216 to
10879b7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackController.java (1)
52-66:⚠️ Potential issue | 🟡 MinorExpose
ProblemDetailschema for error responses.The controller throws exceptions handled centrally as RFC-7807
ProblemDetail, but the OpenAPI currently hides these response schemas (400/403/404on submit and404on latest-feedback), degrading generated client error typing.As per coding guidelines "Return consistent `ProblemDetail` payloads for errors in the application server and follow centralized exception-handling rules in `docs/contributor/api-error-handling.md`."Suggested fix
+import org.springframework.http.ProblemDetail; import org.springframework.http.ResponseEntity; @@ `@ApiResponse`( responseCode = "400", description = "Invalid request (e.g., DISPUTED without explanation)", - content = `@Content`(schema = `@Schema`(hidden = true)) + content = `@Content`(schema = `@Schema`(implementation = ProblemDetail.class)) ) @@ `@ApiResponse`( responseCode = "403", description = "Current user is not the finding's contributor", - content = `@Content`(schema = `@Schema`(hidden = true)) + content = `@Content`(schema = `@Schema`(implementation = ProblemDetail.class)) ) @@ `@ApiResponse`( responseCode = "404", description = "Finding not found in this workspace", - content = `@Content`(schema = `@Schema`(hidden = true)) + content = `@Content`(schema = `@Schema`(implementation = ProblemDetail.class)) ) @@ `@ApiResponse`( responseCode = "404", description = "Finding not found in this workspace", - content = `@Content`(schema = `@Schema`(hidden = true)) + content = `@Content`(schema = `@Schema`(implementation = ProblemDetail.class)) )Also applies to: 92-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackController.java` around lines 52 - 66, The OpenAPI annotations in FindingFeedbackController currently hide the 400/403/404 response schemas which prevents clients from seeing the centralized RFC-7807 ProblemDetail payload; update the ApiResponse annotations for the relevant endpoints in FindingFeedbackController (e.g., the submit/submitFeedback and latest-feedback endpoints) to expose the ProblemDetail schema by replacing content = `@Content`(schema = `@Schema`(hidden = true)) with content = `@Content`(schema = `@Schema`(implementation = ProblemDetail.class)) so generated clients receive proper error typing and conform to the centralized exception-handling contract.server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackService.java (1)
102-111:⚠️ Potential issue | 🟠 MajorEnforce contributor ownership in latest-feedback reads.
getLatestFeedbackvalidates workspace scope but does not enforce “finding belongs to current contributor” likesubmitFeedbackdoes, which allows existence probing within a workspace.Suggested fix
public Optional<FindingFeedbackDTO> getLatestFeedback(WorkspaceContext workspaceContext, UUID findingId) { - // Verify finding exists in this workspace - findingRepository + PracticeFinding finding = findingRepository .findByIdAndWorkspaceId(findingId, workspaceContext.id()) .orElseThrow(() -> new EntityNotFoundException("PracticeFinding", findingId.toString())); var currentUser = userRepository.getCurrentUserElseThrow(); + if (!finding.getContributor().getId().equals(currentUser.getId())) { + throw new AccessForbiddenException("Only the finding's contributor can access feedback"); + } return feedbackRepository .findFirstByFindingIdAndContributorIdOrderByCreatedAtDesc(findingId, currentUser.getId()) .map(FindingFeedbackDTO::from); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackService.java` around lines 102 - 111, The method getLatestFeedback verifies workspace scope but fails to ensure the finding belongs to the current contributor; change the initial existence check to enforce contributor ownership like submitFeedback does: obtain the current user via userRepository.getCurrentUserElseThrow(), then replace the findingRepository.findByIdAndWorkspaceId(...) call with a repository call that includes the contributor/creator id (e.g., findByIdAndWorkspaceIdAndContributorId or findByIdAndWorkspaceIdAndCreatorId) using currentUser.getId(), and throw EntityNotFoundException if not found; keep the later feedbackRepository.findFirstByFindingIdAndContributorIdOrderByCreatedAtDesc(findingId, currentUser.getId()) unchanged.
🧹 Nitpick comments (1)
server/application-server/openapi.yaml (1)
3549-3566: Encode theDISPUTEDexplanation rule directly in schema.Line 3562 says explanation is required for
DISPUTED, but the schema currently can’t enforce that condition. Modeling this in OpenAPI improves generated client validation and avoids avoidable 400s.♻️ Proposed schema refinement
CreateFindingFeedback: - type: object - description: Submit feedback on an AI-generated practice finding - properties: - action: - type: string - description: The feedback action to record - enum: - - APPLIED - - DISPUTED - - NOT_APPLICABLE - explanation: - type: string - description: Explanation for the feedback. Required when action is DISPUTED. - maxLength: 2000 - minLength: 0 - required: - - action + description: Submit feedback on an AI-generated practice finding + oneOf: + - type: object + properties: + action: + type: string + enum: [DISPUTED] + explanation: + type: string + minLength: 1 + maxLength: 2000 + required: [action, explanation] + - type: object + properties: + action: + type: string + enum: [APPLIED, NOT_APPLICABLE] + explanation: + type: string + maxLength: 2000 + required: [action]As per coding guidelines Never hand-edit generated artifacts in
server/application-server/openapi.yaml; regenerate instead using the appropriate generation commands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/application-server/openapi.yaml` around lines 3549 - 3566, The CreateFindingFeedback schema must enforce that explanation is required when action == "DISPUTED" (currently only documented). Update the source OpenAPI/JSON Schema model (not the generated server/application-server/openapi.yaml) for the CreateFindingFeedback object to add a conditional rule (e.g., JSON Schema if: {properties:{action:{const:"DISPUTED"}}} then: {required:["explanation"]}) so explanation is validated when action is DISPUTED while preserving existing maxLength/minLength; then regenerate the OpenAPI artifact using the project's API/schema generation command so server/application-server/openapi.yaml is produced with the conditional rule for action, explanation, and the DISPUTED enum.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webapp/src/api/transformers.gen.ts`:
- Around line 360-368: The generated transformers lack Date conversion for
FindingFeedback.createdAt in the getLatestFeedback response path; do not
hand-edit the file—re-run the API generator so that
findingFeedbackSchemaResponseTransformer (which converts createdAt to Date) is
applied to the getLatestFeedback response transformer (and any other response
functions like submitFeedbackResponseTransformer) so all paths consistently
convert createdAt to a Date object.
---
Duplicate comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackController.java`:
- Around line 52-66: The OpenAPI annotations in FindingFeedbackController
currently hide the 400/403/404 response schemas which prevents clients from
seeing the centralized RFC-7807 ProblemDetail payload; update the ApiResponse
annotations for the relevant endpoints in FindingFeedbackController (e.g., the
submit/submitFeedback and latest-feedback endpoints) to expose the ProblemDetail
schema by replacing content = `@Content`(schema = `@Schema`(hidden = true)) with
content = `@Content`(schema = `@Schema`(implementation = ProblemDetail.class)) so
generated clients receive proper error typing and conform to the centralized
exception-handling contract.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackService.java`:
- Around line 102-111: The method getLatestFeedback verifies workspace scope but
fails to ensure the finding belongs to the current contributor; change the
initial existence check to enforce contributor ownership like submitFeedback
does: obtain the current user via userRepository.getCurrentUserElseThrow(), then
replace the findingRepository.findByIdAndWorkspaceId(...) call with a repository
call that includes the contributor/creator id (e.g.,
findByIdAndWorkspaceIdAndContributorId or findByIdAndWorkspaceIdAndCreatorId)
using currentUser.getId(), and throw EntityNotFoundException if not found; keep
the later
feedbackRepository.findFirstByFindingIdAndContributorIdOrderByCreatedAtDesc(findingId,
currentUser.getId()) unchanged.
---
Nitpick comments:
In `@server/application-server/openapi.yaml`:
- Around line 3549-3566: The CreateFindingFeedback schema must enforce that
explanation is required when action == "DISPUTED" (currently only documented).
Update the source OpenAPI/JSON Schema model (not the generated
server/application-server/openapi.yaml) for the CreateFindingFeedback object to
add a conditional rule (e.g., JSON Schema if:
{properties:{action:{const:"DISPUTED"}}} then: {required:["explanation"]}) so
explanation is validated when action is DISPUTED while preserving existing
maxLength/minLength; then regenerate the OpenAPI artifact using the project's
API/schema generation command so server/application-server/openapi.yaml is
produced with the conditional rule for action, explanation, and the DISPUTED
enum.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a2d32d0-7625-4e98-88db-3ebf9b426027
📒 Files selected for processing (22)
docs/contributor/erd/schema.mmdserver/application-server/openapi.yamlserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedback.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackAction.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackController.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/CreateFindingFeedbackDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/FindingFeedbackDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/FindingFeedbackEngagementDTO.javaserver/application-server/src/main/resources/db/changelog/1774335316041_changelog.xmlserver/application-server/src/main/resources/db/master.xmlserver/application-server/src/test/java/de/tum/in/www1/hephaestus/architecture/ActivityModuleBoundaryTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackControllerIntegrationTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackServiceTest.javaserver/intelligence-service/src/shared/db/schema.tswebapp/src/api/@tanstack/react-query.gen.tswebapp/src/api/index.tswebapp/src/api/sdk.gen.tswebapp/src/api/transformers.gen.tswebapp/src/api/types.gen.ts
✅ Files skipped from review due to trivial changes (5)
- server/application-server/src/main/resources/db/master.xml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackAction.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/FindingFeedbackEngagementDTO.java
- server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackControllerIntegrationTest.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackRepository.java
🚧 Files skipped from review as they are similar to previous changes (8)
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/PracticeFindingRepository.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/CreateFindingFeedbackDTO.java
- server/intelligence-service/src/shared/db/schema.ts
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedback.java
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/practices/finding/feedback/dto/FindingFeedbackDTO.java
- server/application-server/src/test/java/de/tum/in/www1/hephaestus/practices/finding/feedback/FindingFeedbackServiceTest.java
- server/application-server/src/main/resources/db/changelog/1774335316041_changelog.xml
- webapp/src/api/sdk.gen.ts
Comment on lines
+360
to
+368
| const findingFeedbackSchemaResponseTransformer = (data: any) => { | ||
| data.createdAt = new Date(data.createdAt); | ||
| return data; | ||
| }; | ||
|
|
||
| export const submitFeedbackResponseTransformer = async (data: any): Promise<SubmitFeedbackResponse> => { | ||
| data = findingFeedbackSchemaResponseTransformer(data); | ||
| return data; | ||
| }; |
Contributor
There was a problem hiding this comment.
getLatestFeedback is missing Date transformation for createdAt.
submitFeedback now converts createdAt to Date, but the corresponding getLatestFeedback response path is not transformed, creating a runtime/type mismatch for FindingFeedback.createdAt.
Suggested fix (via generator output)
-import type { ..., SubmitFeedbackResponse, ... } from './types.gen';
+import type { ..., GetLatestFeedbackResponse, SubmitFeedbackResponse, ... } from './types.gen';
@@
export const submitFeedbackResponseTransformer = async (data: any): Promise<SubmitFeedbackResponse> => {
data = findingFeedbackSchemaResponseTransformer(data);
return data;
};
+
+export const getLatestFeedbackResponseTransformer = async (data: any): Promise<GetLatestFeedbackResponse> => {
+ if (data) {
+ data = findingFeedbackSchemaResponseTransformer(data);
+ }
+ return data;
+};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/api/transformers.gen.ts` around lines 360 - 368, The generated
transformers lack Date conversion for FindingFeedback.createdAt in the
getLatestFeedback response path; do not hand-edit the file—re-run the API
generator so that findingFeedbackSchemaResponseTransformer (which converts
createdAt to Date) is applied to the getLatestFeedback response transformer (and
any other response functions like submitFeedbackResponseTransformer) so all
paths consistently convert createdAt to a Date object.
Collaborator
Author
|
🎉 This PR is included in version 0.50.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
5 tasks
17 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
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.
Description
Adds an append-only
FindingFeedbackentity and workspace-scoped REST API enabling contributors to react to AI-generated practice findings with three actions: APPLIED ("I fixed this"), DISPUTED ("The AI is wrong"), and NOT_APPLICABLE ("Not relevant to my context"). This provides the data foundation for research questions RQ1/RQ2/RQ4 and the engagement metrics needed by the dashboard (#897 v2).Closes #898
Key design decisions
@Immutable(append-only)@ManyToOnetoPracticeFinding(not raw UUID)ON DELETE CASCADEfor cleanup.practiceSlug/findingVerdictproposed in issue would be stale on an immutable table. Join at query time instead.FindingFeedbackEngagementDTO(notMap<Enum,Long>){applied, disputed, notApplicable}with proper required markers.Files changed
practices/finding/feedback/FindingFeedback.java@Immutableentity with@PrePersistUUID, dualfindingIdcolumn mappingpractices/finding/feedback/FindingFeedbackAction.javapractices/finding/feedback/FindingFeedbackRepository.java@WorkspaceAgnosticrepo withDISTINCT ONnative query and workspace-scoped JPQL aggregatepractices/finding/feedback/FindingFeedbackService.javapractices/finding/feedback/FindingFeedbackController.java@WorkspaceScopedControllerwith POST 201, GET 200/204, GET engagementpractices/finding/feedback/dto/CreateFindingFeedbackDTO.java@NotNull action,@Size(max=2000) explanationpractices/finding/feedback/dto/FindingFeedbackDTO.java@Schema(requiredMode=REQUIRED)on non-nullable fieldspractices/finding/feedback/dto/FindingFeedbackEngagementDTO.java1774335316041_changelog.xmlPracticeFindingRepository.javafindByIdAndWorkspaceIdworkspace-scoped queryActivityModuleBoundaryTest.javaFindingFeedbackControllerto practices controller allowlistAPI endpoints
POST/workspaces/{slug}/practices/findings/{findingId}/feedbackGET/workspaces/{slug}/practices/findings/{findingId}/feedbackGET/workspaces/{slug}/practices/findings/engagementSchema
How to test
Unit tests (14 cases):
Integration tests (13 cases):
Test coverage includes:
Summary by CodeRabbit