feat(workspace): auto-discover and monitor GitLab repositories#963
Merged
FelixTJDietrich merged 22 commits intomainfrom Apr 7, 2026
Merged
feat(workspace): auto-discover and monitor GitLab repositories#963FelixTJDietrich merged 22 commits intomainfrom
FelixTJDietrich merged 22 commits intomainfrom
Conversation
When creating a GitLab PAT workspace, all repositories in the selected group are now automatically discovered and added to monitoring — matching the existing GitHub App installation behavior. Key changes: - Extract GitLabWorkspaceInitializationService from WorkspaceActivationService to encapsulate the full GitLab initialization lifecycle (webhook setup, project discovery, org linking, monitor creation, full data sync) - Add GitLabCommitBackfillService for JGit-based commit history with full diff stats (additions, deletions, changedFiles) and file change tracking - Fix subgroup repo inclusion: replace org-login-based query with findAllByWorkspaceMonitors (joins through RepositoryToMonitor) - Fix LazyInitializationException: eagerly fetch provider in repo query - Add rate limit awareness and cooldown for slowly-changing entities - Wire initialization into workspace creation flow (async, fire-and-forget) Fixes #961 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
application-server
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GitLab PAT workspace initialization with repository discovery and NATS consumer startup, a JGit-based GitLab commit backfill service, per-workspace member "hidden" visibility (DB, API, UI), team/collaborator enrichment, avatar initials utility, and related DTO/UI/migration changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Controller/User
participant Init as GitLabWorkspaceInitializationService
participant Webhook as GitLabWebhookService
participant GroupSync as GitLabGroupSyncService
participant DB as Database
participant Monitor as RepositoryToMonitorRepo
participant NATS as NATS
User->>Init: initializeAsync(workspaceId)
activate Init
Init->>Init: load workspace, set WorkspaceContext
Init->>Webhook: setupWebhook(workspace) / rotate token
Webhook-->>Init: success / non-fatal error
Init->>GroupSync: discoverGroupProjects(workspace)
GroupSync-->>Init: list of repositories / error
Init->>DB: linkWorkspaceToOrganization(workspace)
DB-->>Init: linked / no-op
Init->>Monitor: ensureRepositoryMonitors(workspace, repos)
Monitor-->>DB: create missing RepositoryToMonitor rows
DB-->>Monitor: persisted
Monitor-->>Init: created count
Init->>NATS: startNatsConsumer(workspace) (if enabled)
NATS-->>Init: consumer started / error
Init->>Init: syncFullData(workspace)
Init-->>User: initialization complete
deactivate Init
sequenceDiagram
participant Scheduler as GitLabDataSyncScheduler
participant Backfill as GitLabCommitBackfillService
participant GitMgr as GitRepositoryManager
participant JGit as Local JGit
participant CommitDB as CommitRepository
participant Event as DomainEventBus
Scheduler->>Scheduler: load repositories from workspace monitors
loop per repository
Scheduler->>Backfill: backfillCommits(scopeId, repository)
activate Backfill
Backfill->>GitMgr: resolve server/token + ensure clone/fetch
GitMgr->>JGit: clone/fetch operations
JGit-->>GitMgr: repo ready
GitMgr-->>Backfill: local repo handle
Backfill->>CommitDB: findLatestPersistedSha(repositoryId)
CommitDB-->>Backfill: latestSha / null
Backfill->>JGit: walk commits from latestSha..HEAD
JGit-->>Backfill: stream commits (bounded batches)
loop per commit
Backfill->>CommitDB: existsByShaAndRepositoryId?
CommitDB-->>Backfill: exists / not exists
alt not exists
Backfill->>CommitDB: upsert commit + file changes (Transaction)
Backfill->>Event: publish CommitCreated
end
end
Backfill-->>Scheduler: SyncResult.completed(processedCount)
deactivate Backfill
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class GitLab PAT workspace initialization to automatically discover all projects in a selected GitLab group (including subgroups), register monitoring, and run a full data sync—bringing GitLab behavior closer to the existing GitHub App installation flow.
Changes:
- Introduces
GitLabWorkspaceInitializationServiceto own GitLab workspace init (webhook, discovery, org linking, monitor creation) and full sync orchestration, reused by both creation-time async init and startup activation. - Adds
GitLabCommitBackfillService(JGit-based) and wires commit backfill preference into GitLab sync paths. - Adjusts workspace creation/activation and GitLab scheduler/repository queries to support “monitor all repos” discovery and subgroup inclusion.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/GitLabWorkspaceInitializationServiceTest.java | Adds unit tests covering GitLab init guards, webhook error isolation, monitor creation/idempotency, async path, and org linking. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/architecture/CodeQualityTest.java | Allows GitLabWorkspaceInitializationService to use ObjectProvider for optional GitLab beans. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceService.java | Adds createWorkspaceWithInitialization() wrapper to kick off async GitLab init and sets GitLab PAT repo selection to ALL. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceRepository.java | Adds org-link existence helpers used for unique-constraint guarding during org linking. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceRegistryController.java | Switches workspace creation endpoint to call createWorkspaceWithInitialization(). |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningService.java | Sets repositorySelection=ALL for bootstrapped GitLab PAT workspaces. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceActivationService.java | Replaces large inline GitLab init/sync logic with gitLabInitService.initialize() + syncFullData(). |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/GitLabWorkspaceInitializationService.java | New service encapsulating GitLab init lifecycle and full sync, including rate-limit-aware per-repo syncing and commit backfill preference. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/sync/GitLabDataSyncScheduler.java | Uses workspace monitors to resolve repos (including subgroups) and prefers commit backfill over REST commit sync. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/repository/RepositoryRepository.java | Adds findAllByWorkspaceMonitors(workspaceId) query to fetch repos via monitored names (with provider fetch). |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/common/gitlab/GitLabSyncServiceHolder.java | Adds optional GitLabCommitBackfillService to the holder. |
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/commit/gitlab/GitLabCommitBackfillService.java | New JGit-based commit backfill implementation for GitLab with diff stats + per-file changes. |
Comment on lines
+195
to
+213
| /** | ||
| * Creates a workspace and triggers async GitLab initialization if applicable. | ||
| * | ||
| * <p>This method is intentionally NOT {@code @Transactional} so that the inner | ||
| * {@link #createWorkspace(CreateWorkspaceRequestDTO)} transaction commits before | ||
| * the async initialization reads the workspace from the database. | ||
| * | ||
| * @param request the workspace creation request | ||
| * @return the created workspace | ||
| */ | ||
| public Workspace createWorkspaceWithInitialization(CreateWorkspaceRequestDTO request) { | ||
| Workspace workspace = createWorkspace(request); | ||
|
|
||
| // Trigger async repository discovery for GitLab PAT workspaces. | ||
| // The @Transactional createWorkspace() has already committed at this point, | ||
| // so the async thread will find the workspace in the database. | ||
| if (workspace.getGitProviderMode() == Workspace.GitProviderMode.GITLAB_PAT) { | ||
| gitLabWorkspaceInitializationService.initializeAsync(workspace.getId()); | ||
| } |
There was a problem hiding this comment.
createWorkspaceWithInitialization() calls createWorkspace(request) via self-invocation, so the @Transactional on createWorkspace(CreateWorkspaceRequestDTO) (and the 5-arg createWorkspace) will NOT be applied. This means workspace creation + owner membership are no longer atomic, and the comment claiming the inner transaction has already committed is inaccurate. Consider keeping workspace creation transactional and triggering async init in an after-commit hook (e.g., TransactionSynchronizationManager.registerSynchronization / @TransactionalEventListener), or calling the transactional method through the Spring proxy (inject self) and only scheduling initializeAsync after commit.
Comment on lines
+198
to
+206
| List<Repository> syncedRepos = discoverGroupProjects(workspace); | ||
|
|
||
| // Phase 2: Link organization + create monitors | ||
| if (!syncedRepos.isEmpty()) { | ||
| linkWorkspaceToOrganization(workspace); | ||
| int created = ensureRepositoryMonitors(workspace, syncedRepos); | ||
| // Update NATS consumer subscriptions to include newly discovered repos | ||
| if (created > 0 && natsProperties.enabled()) { | ||
| natsConsumerService.updateScopeConsumer(workspace.getId()); |
There was a problem hiding this comment.
initialize() calls linkWorkspaceToOrganization() within the same bean instance. With Spring proxy-based transactions, this self-invocation bypasses the @Transactional on linkWorkspaceToOrganization, so the uniqueness-guard + re-read/save sequence will not run in a single transaction as intended. If you need a transactional boundary here, use TransactionTemplate inside initialize(), move the method to a separate @service, or invoke it via the proxied bean (self-injection).
Comment on lines
+132
to
+142
| // Set workspace context for entire lifecycle (init + sync + NATS). | ||
| // initialize() checks contextOwner and won't double-set/clear. | ||
| WorkspaceContext context = WorkspaceContext.fromWorkspace(workspace, Set.of()); | ||
| WorkspaceContextHolder.setContext(context); | ||
| try { | ||
| initialize(workspace); | ||
| syncFullData(workspace); | ||
| startNatsConsumer(workspace); | ||
| } finally { | ||
| WorkspaceContextHolder.clearContext(); | ||
| } |
There was a problem hiding this comment.
initializeAsync() always calls startNatsConsumer(workspace) even if initialize() returned early due to missing token/accountLogin (or non-GITLAB_PAT mode). That can start a scope consumer for an uninitialized/misconfigured workspace. Consider gating startNatsConsumer (and syncFullData) behind the same eligibility checks as initialize(), or have initialize() return a boolean indicating whether initialization actually ran.
Comment on lines
+102
to
+110
| @Query( | ||
| """ | ||
| SELECT r FROM Repository r LEFT JOIN FETCH r.provider | ||
| WHERE r.nameWithOwner IN ( | ||
| SELECT m.nameWithOwner FROM RepositoryToMonitor m WHERE m.workspace.id = :workspaceId | ||
| ) | ||
| """ | ||
| ) | ||
| List<Repository> findAllByWorkspaceMonitors(@Param("workspaceId") Long workspaceId); |
There was a problem hiding this comment.
findAllByWorkspaceMonitors() selects repositories solely by nameWithOwner. Since Repository has a unique constraint on (provider_id, name_with_owner), the same nameWithOwner can exist for different providers/instances; this query can therefore return repositories from the wrong provider (e.g., GitHub) if names collide. Consider scoping the query by provider (or join via workspace→organization→provider) so it only returns repositories for the workspace's GitProvider instance.
| : List.of(); | ||
| // Find all repositories monitored by this workspace (via RepositoryToMonitor join, | ||
| // which correctly includes subgroup repos — not just top-level group repos) | ||
| List<Repository> repos = repositoryRepository.findAllByWorkspaceMonitors(session.scopeId()); |
There was a problem hiding this comment.
Switching repo resolution to findAllByWorkspaceMonitors(scopeId) relies on nameWithOwner matching and currently drops the providerId scoping that previously prevented cross-provider collisions. If two providers contain the same nameWithOwner, the scheduler may sync the wrong repository. Once RepositoryRepository.findAllByWorkspaceMonitors is scoped to the workspace's provider, this callsite should use that safer variant (or pass providerId explicitly).
Suggested change
| List<Repository> repos = repositoryRepository.findAllByWorkspaceMonitors(session.scopeId()); | |
| List<Repository> repos = repositoryRepository | |
| .findAllByWorkspaceMonitors(session.scopeId()) | |
| .stream() | |
| .filter(repo -> repo.getProvider().getId().equals(session.providerId())) | |
| .collect(Collectors.toList()); |
![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: 2
🧹 Nitpick comments (3)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/commit/gitlab/GitLabCommitBackfillService.java (2)
262-281: Redundant database query after commit persistence.
publishCommitCreatedqueries the database for the commit (line 263) immediately afterprocessCommitInfojust saved it (line 252). Consider returning the commit from the transaction or passing it directly to avoid the extra query:♻️ Suggested optimization
- private void publishCommitCreated(String sha, Repository repository, Long scopeId) { - Commit commit = commitRepository.findByShaAndRepositoryId(sha, repository.getId()).orElse(null); - if (commit == null) { - return; - } + private void publishCommitCreated(Commit commit, Repository repository, Long scopeId) { + if (commit == null) { + return; + }And update the caller to pass the commit returned from the transaction.
🤖 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/gitprovider/commit/gitlab/GitLabCommitBackfillService.java` around lines 262 - 281, publishCommitCreated currently re-queries the DB via commitRepository.findByShaAndRepositoryId(sha, repository.getId()) even though processCommitInfo already persisted the Commit; change publishCommitCreated to accept a Commit parameter (instead of sha) and use EventPayload.CommitData.from(commit) directly, then update the caller (the method that calls processCommitInfo and then publishCommitCreated) to return or pass the Commit returned from processCommitInfo/the transaction into publishCommitCreated; keep existing EventContext construction and eventPublisher.publishEvent(new DomainEvent.CommitCreated(...)) unchanged.
274-274:DataSource.GRAPHQL_SYNCis misleading for JGit-based operations.This service (and
GitHubCommitBackfillService) uses local JGit clones, not GraphQL. TheDataSourceenum is misnamed —GRAPHQL_SYNCsemantically suggests GraphQL-based data, but it's actually used for all scheduled synchronization operations regardless of implementation. Event consumers seeingGRAPHQL_SYNCmay incorrectly infer that data came from GraphQL.Consider renaming the enum value to
SCHEDULED_SYNCorBACKFILLto reflect its actual purpose, or adding provider-specific values (JGIT_BACKFILL,LOCAL_GIT) for better observability.🤖 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/gitprovider/commit/gitlab/GitLabCommitBackfillService.java` at line 274, The DataSource enum value GRAPHQL_SYNC is misleading for JGit-based backfill services; update the enum (DataSource) to use a clearer value such as SCHEDULED_SYNC or JGIT_BACKFILL (pick one consistent name) and replace usages in GitLabCommitBackfillService and GitHubCommitBackfillService (and any other callers) to the new enum constant so event consumers correctly reflect scheduled/local JGit syncs rather than GraphQL; ensure you update the enum declaration, all references to DataSource.GRAPHQL_SYNC, and any persistence/serialization mappings or tests that rely on the old name.server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/repository/RepositoryRepository.java (1)
97-110: Provider filtering adds defensive robustness but is not required in practice.The query joins repositories with monitored entries via
nameWithOwnerwithout explicitly filtering by provider type. However, because each workspace is bound to a single provider type (set at workspace creation), all repositories associated with that workspace will naturally be of the correct provider. The callers (GitLabWorkspaceInitializationServiceandGitLabDataSyncScheduler) are GitLab-specific, and the workspace constraint ensures only GitLab repositories are returned in practice.For defensive programming, consider adding an explicit provider filter to guard against potential data inconsistencies:
Optional refactoring example
`@Query`( """ SELECT r FROM Repository r LEFT JOIN FETCH r.provider WHERE r.nameWithOwner IN ( SELECT m.nameWithOwner FROM RepositoryToMonitor m WHERE m.workspace.id = :workspaceId ) + AND r.provider.type = de.tum.in.www1.hephaestus.gitprovider.common.GitProviderType.GITLAB """ ) -List<Repository> findAllByWorkspaceMonitors(`@Param`("workspaceId") Long workspaceId); +List<Repository> findAllByWorkspaceMonitorsAndProviderType( + `@Param`("workspaceId") Long workspaceId, + `@Param`("providerType") GitProviderType providerType +);🤖 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/gitprovider/repository/RepositoryRepository.java` around lines 97 - 110, The query in RepositoryRepository.findAllByWorkspaceMonitors currently matches repositories by nameWithOwner but does not explicitly filter by provider, which could allow mismatched provider types if data is inconsistent; update the JPQL to also restrict repositories to the workspace's provider (e.g., join or compare r.provider.type or r.provider.id to the workspace's provider) so only repositories whose provider matches the workspace's provider are returned—locate the method findAllByWorkspaceMonitors in class RepositoryRepository and modify the `@Query` to include an explicit provider predicate referencing Repository (r), RepositoryToMonitor (m), and the workspaceId parameter.
🤖 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/workspace/GitLabWorkspaceInitializationService.java`:
- Around line 351-366: The loop in GitLabWorkspaceInitializationService builds a
one-time snapshot "existing" from repositoryToMonitorRepository, so if
syncedRepos contains duplicate getNameWithOwner values you may save the same
RepositoryToMonitor twice; after creating and
repositoryToMonitorRepository.save(monitor) inside the for-loop, add the newly
saved name (nwo) to the existing Set (or otherwise check/record created names)
so subsequent iterations see it and skip duplicates, preserving idempotency and
avoiding unique-constraint violations.
- Around line 597-625: The code advances
repositoryRepository.updateLastSyncAt(...) when issues/MRs are done but before
verifying commits completed, which can skip failed commit windows because
updatedAfter is derived from repo.getLastSyncAt(); modify the logic in
GitLabWorkspaceInitializationService so the watermark is only bumped if commit
processing succeeded: track whether commits finished successfully (e.g. set a
commitsDone flag when backfillCommits or syncCommitsForRepository returns
without exception and produced/attempted the expected window) and include that
flag in the allDone condition (or require successful commit path when
commitBackfillService or commitSyncService are non-null) before calling
repositoryRepository.updateLastSyncAt(...); ensure you reference
commitBackfillService.backfillCommits,
commitSyncService.syncCommitsForRepository, totalCommits, issuesDone, mrsDone,
and repositoryRepository.updateLastSyncAt when making the change.
---
Nitpick comments:
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/commit/gitlab/GitLabCommitBackfillService.java`:
- Around line 262-281: publishCommitCreated currently re-queries the DB via
commitRepository.findByShaAndRepositoryId(sha, repository.getId()) even though
processCommitInfo already persisted the Commit; change publishCommitCreated to
accept a Commit parameter (instead of sha) and use
EventPayload.CommitData.from(commit) directly, then update the caller (the
method that calls processCommitInfo and then publishCommitCreated) to return or
pass the Commit returned from processCommitInfo/the transaction into
publishCommitCreated; keep existing EventContext construction and
eventPublisher.publishEvent(new DomainEvent.CommitCreated(...)) unchanged.
- Line 274: The DataSource enum value GRAPHQL_SYNC is misleading for JGit-based
backfill services; update the enum (DataSource) to use a clearer value such as
SCHEDULED_SYNC or JGIT_BACKFILL (pick one consistent name) and replace usages in
GitLabCommitBackfillService and GitHubCommitBackfillService (and any other
callers) to the new enum constant so event consumers correctly reflect
scheduled/local JGit syncs rather than GraphQL; ensure you update the enum
declaration, all references to DataSource.GRAPHQL_SYNC, and any
persistence/serialization mappings or tests that rely on the old name.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/repository/RepositoryRepository.java`:
- Around line 97-110: The query in
RepositoryRepository.findAllByWorkspaceMonitors currently matches repositories
by nameWithOwner but does not explicitly filter by provider, which could allow
mismatched provider types if data is inconsistent; update the JPQL to also
restrict repositories to the workspace's provider (e.g., join or compare
r.provider.type or r.provider.id to the workspace's provider) so only
repositories whose provider matches the workspace's provider are returned—locate
the method findAllByWorkspaceMonitors in class RepositoryRepository and modify
the `@Query` to include an explicit provider predicate referencing Repository (r),
RepositoryToMonitor (m), and the workspaceId parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b956fde4-a77e-4d9d-a6cc-107f634f8241
📒 Files selected for processing (12)
server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/commit/gitlab/GitLabCommitBackfillService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/common/gitlab/GitLabSyncServiceHolder.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/repository/RepositoryRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/sync/GitLabDataSyncScheduler.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/GitLabWorkspaceInitializationService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceActivationService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceRegistryController.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceService.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/architecture/CodeQualityTest.javaserver/application-server/src/test/java/de/tum/in/www1/hephaestus/workspace/GitLabWorkspaceInitializationServiceTest.java
Comment on lines
+351
to
+366
| Set<String> existing = repositoryToMonitorRepository | ||
| .findByWorkspaceId(workspace.getId()) | ||
| .stream() | ||
| .map(RepositoryToMonitor::getNameWithOwner) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| int created = 0; | ||
| for (Repository repo : syncedRepos) { | ||
| String nwo = repo.getNameWithOwner(); | ||
| if (nwo == null || existing.contains(nwo)) { | ||
| continue; | ||
| } | ||
| RepositoryToMonitor monitor = new RepositoryToMonitor(); | ||
| monitor.setNameWithOwner(nwo); | ||
| monitor.setWorkspace(workspace); | ||
| repositoryToMonitorRepository.save(monitor); |
Contributor
There was a problem hiding this comment.
Update the existing set after each monitor save.
This only snapshots DB state once. If syncedRepos contains the same nameWithOwner twice, the second entry is saved again in the same invocation, which breaks the method's advertised idempotency and can trip the unique constraint.
♻️ Minimal fix
for (Repository repo : syncedRepos) {
String nwo = repo.getNameWithOwner();
if (nwo == null || existing.contains(nwo)) {
continue;
}
RepositoryToMonitor monitor = new RepositoryToMonitor();
monitor.setNameWithOwner(nwo);
monitor.setWorkspace(workspace);
repositoryToMonitorRepository.save(monitor);
+ existing.add(nwo);
created++;
}📝 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.
Suggested change
| Set<String> existing = repositoryToMonitorRepository | |
| .findByWorkspaceId(workspace.getId()) | |
| .stream() | |
| .map(RepositoryToMonitor::getNameWithOwner) | |
| .collect(Collectors.toSet()); | |
| int created = 0; | |
| for (Repository repo : syncedRepos) { | |
| String nwo = repo.getNameWithOwner(); | |
| if (nwo == null || existing.contains(nwo)) { | |
| continue; | |
| } | |
| RepositoryToMonitor monitor = new RepositoryToMonitor(); | |
| monitor.setNameWithOwner(nwo); | |
| monitor.setWorkspace(workspace); | |
| repositoryToMonitorRepository.save(monitor); | |
| Set<String> existing = repositoryToMonitorRepository | |
| .findByWorkspaceId(workspace.getId()) | |
| .stream() | |
| .map(RepositoryToMonitor::getNameWithOwner) | |
| .collect(Collectors.toSet()); | |
| int created = 0; | |
| for (Repository repo : syncedRepos) { | |
| String nwo = repo.getNameWithOwner(); | |
| if (nwo == null || existing.contains(nwo)) { | |
| continue; | |
| } | |
| RepositoryToMonitor monitor = new RepositoryToMonitor(); | |
| monitor.setNameWithOwner(nwo); | |
| monitor.setWorkspace(workspace); | |
| repositoryToMonitorRepository.save(monitor); | |
| existing.add(nwo); | |
| created++; | |
| } |
🤖 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/workspace/GitLabWorkspaceInitializationService.java`
around lines 351 - 366, The loop in GitLabWorkspaceInitializationService builds
a one-time snapshot "existing" from repositoryToMonitorRepository, so if
syncedRepos contains duplicate getNameWithOwner values you may save the same
RepositoryToMonitor twice; after creating and
repositoryToMonitorRepository.save(monitor) inside the for-loop, add the newly
saved name (nwo) to the existing Set (or otherwise check/record created names)
so subsequent iterations see it and skip duplicates, preserving idempotency and
avoiding unique-constraint violations.
Comment on lines
+597
to
+625
| if (commitBackfillService != null) { | ||
| try { | ||
| SyncResult r = commitBackfillService.backfillCommits(workspace.getId(), repo); | ||
| totalCommits += r.count(); | ||
| } catch (Exception e) { | ||
| log.warn( | ||
| "Failed commit backfill: workspaceId={}, repo={}", | ||
| workspace.getId(), | ||
| repo.getNameWithOwner(), | ||
| e | ||
| ); | ||
| } | ||
| } else if (commitSyncService != null) { | ||
| try { | ||
| SyncResult r = commitSyncService.syncCommitsForRepository(workspace.getId(), repo, updatedAfter); | ||
| totalCommits += r.count(); | ||
| } catch (Exception e) { | ||
| log.warn( | ||
| "Failed commit sync: workspaceId={}, repo={}", | ||
| workspace.getId(), | ||
| repo.getNameWithOwner(), | ||
| e | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| boolean allDone = (issueSyncService == null || issuesDone) && (mrSyncService == null || mrsDone); | ||
| if (allDone) { | ||
| repositoryRepository.updateLastSyncAt(repo.getId(), Instant.now()); |
Contributor
There was a problem hiding this comment.
Don't advance lastSyncAt after a failed commit sync.
updatedAfter is derived from repo.getLastSyncAt(), but Line 625 bumps that watermark as soon as issues/MRs finish. If the REST fallback at Line 611 throws, the next run starts after the new watermark and permanently skips the failed commit window.
🛡️ Gate the watermark on commit completion too
boolean issuesDone = false;
boolean mrsDone = false;
+ boolean commitsDone = commitBackfillService == null && commitSyncService == null;
if (issueSyncService != null) {
try {
SyncResult r = issueSyncService.syncIssues(workspace.getId(), repo, updatedAfter);
totalIssues += r.count();
@@
if (commitBackfillService != null) {
try {
SyncResult r = commitBackfillService.backfillCommits(workspace.getId(), repo);
totalCommits += r.count();
+ commitsDone = r.isCompleted();
} catch (Exception e) {
log.warn(
"Failed commit backfill: workspaceId={}, repo={}",
workspace.getId(),
@@
} else if (commitSyncService != null) {
try {
SyncResult r = commitSyncService.syncCommitsForRepository(workspace.getId(), repo, updatedAfter);
totalCommits += r.count();
+ commitsDone = r.isCompleted();
} catch (Exception e) {
log.warn(
"Failed commit sync: workspaceId={}, repo={}",
workspace.getId(),
@@
- boolean allDone = (issueSyncService == null || issuesDone) && (mrSyncService == null || mrsDone);
+ boolean allDone =
+ (issueSyncService == null || issuesDone) &&
+ (mrSyncService == null || mrsDone) &&
+ commitsDone;
if (allDone) {
repositoryRepository.updateLastSyncAt(repo.getId(), Instant.now());
}🤖 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/workspace/GitLabWorkspaceInitializationService.java`
around lines 597 - 625, The code advances
repositoryRepository.updateLastSyncAt(...) when issues/MRs are done but before
verifying commits completed, which can skip failed commit windows because
updatedAfter is derived from repo.getLastSyncAt(); modify the logic in
GitLabWorkspaceInitializationService so the watermark is only bumped if commit
processing succeeded: track whether commits finished successfully (e.g. set a
commitsDone flag when backfillCommits or syncCommitsForRepository returns
without exception and produced/attempted the expected window) and include that
flag in the allDone condition (or require successful commit path when
commitBackfillService or commitSyncService are non-null) before calling
repositoryRepository.updateLastSyncAt(...); ensure you reference
commitBackfillService.backfillCommits,
commitSyncService.syncCommitsForRepository, totalCommits, issuesDone, mrsDone,
and repositoryRepository.updateLastSyncAt when making the change.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Avatar consistency:
- Add shared getInitials() utility for consistent initial generation
across Header, ContributorCard, LeaderboardTable, ProfileHeader
- Fix relative GitLab avatar URLs by prepending server URL in
GitLabUserService.resolveAvatarUrl() and WorkspaceProvisioningService
- Remove hardcoded github.com avatar fallback from Header
Profile link fixes:
- TeamsPage: use member.htmlUrl instead of hardcoded github.com URL
- ProfileHeader: extract domain from htmlUrl for display text
Hide member feature:
- Add hidden boolean column to workspace_membership (Liquibase migration)
- Add PATCH /{userId}/hidden endpoint for admin toggle
- Filter hidden members from individual leaderboard
- Filter hidden members from TeamInfoDTO (team views)
- Add visibility toggle (eye icon) in admin members table
- Enrich UserTeamsDTO with hidden flag for admin page
Team sync Phase F fix:
- Use TeamMembership constructor (sets composite ID + isNew flag)
instead of no-arg constructor which caused merge failures
- Students with WRITE/TRIAGE permissions now correctly added as
MEMBER to their parent subgroup team
github-actions
bot
added
the
webapp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
server/application-server/openapi.yaml (1)
910-946: Consider adding 404 response documentation.The endpoint is correctly defined with proper path/query parameters. However, it's missing the 404 response documentation for when the membership doesn't exist. While the global exception handler will produce the correct RFC-7807 response, explicit documentation improves API discoverability.
📝 Suggested addition for 404 response
responses: "200": content: application/json: schema: $ref: "#/components/schemas/WorkspaceMembership" description: Updated membership + "404": + description: Workspace membership not found summary: Toggle the hidden flag for a workspace member.🤖 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 910 - 946, Add an explicit 404 response to the PATCH operation with operationId updateMemberVisibility (path /workspaces/{workspaceSlug}/members/{userId}/hidden) to document the case where the membership does not exist; update the OpenAPI fragment to include a "404" response entry that references the RFC-7807/problem+json content type (or the existing ProblemDetails schema used elsewhere) and a brief description like "Membership not found" so clients can discover the missing-membership error condition.webapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/members.tsx (1)
56-60: Also invalidate leaderboard-related queries after visibility changes.Toggling
hiddenaffects leaderboard results, but onlygetUsersWithTeamsis invalidated. Consider invalidating leaderboard queries too to avoid stale rankings in the same session.♻️ Suggested follow-up
onSuccess: () => { queryClient.invalidateQueries({ queryKey: getUsersWithTeamsQueryKey({ path: { workspaceSlug: workspaceSlug ?? "" } }), }); + queryClient.invalidateQueries({ + predicate: (query) => + typeof query.queryKey[0] === "object" && + query.queryKey[0] !== null && + "_id" in query.queryKey[0] && + (query.queryKey[0] as { _id?: string })._id === "getLeaderboard", + }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/routes/_authenticated/w/`$workspaceSlug/admin/_admin/members.tsx around lines 56 - 60, Add leaderboard query invalidation inside the same onSuccess block that currently calls queryClient.invalidateQueries for getUsersWithTeamsQueryKey; call queryClient.invalidateQueries with the leaderboard query key (e.g., getLeaderboardQueryKey({ path: { workspaceSlug: workspaceSlug ?? "" } }) or the app's leaderboard query key pattern) so leaderboard data is refreshed when visibility (hidden) is toggled, placing this alongside the existing invalidate for getUsersWithTeamsQueryKey.server/application-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/LeaderboardService.java (1)
168-170: Avoid repeated hidden-member lookups in team mode.
createTeamLeaderboardcallscreateIndividualLeaderboardper team, and each call re-fetches hidden member IDs. Consider passing a precomputedhiddenMemberIdsset intocreateIndividualLeaderboardto avoid N+1 reads.♻️ Refactor sketch
- private List<LeaderboardEntryDTO> createIndividualLeaderboard(..., Map<Long, List<Team>> teamHierarchy) { + private List<LeaderboardEntryDTO> createIndividualLeaderboard(..., Map<Long, List<Team>> teamHierarchy, Set<Long> hiddenMemberIds) { - Set<Long> hiddenMemberIds = workspaceMembershipService.getHiddenMemberIds(workspaceId); activityData.keySet().removeAll(hiddenMemberIds); } + Set<Long> hiddenMemberIds = workspaceMembershipService.getHiddenMemberIds(workspaceId); List<LeaderboardEntryDTO> entries = createIndividualLeaderboard( workspace, after, before, Optional.of(teamEntity), LeaderboardSortType.SCORE, - teamHierarchy + teamHierarchy, hiddenMemberIds );🤖 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/leaderboard/LeaderboardService.java` around lines 168 - 170, The code repeatedly calls workspaceMembershipService.getHiddenMemberIds inside createIndividualLeaderboard for each team causing N+1 reads; refactor createIndividualLeaderboard to accept a Set<Long> hiddenMemberIds (or an Optional/nullable parameter) and update createTeamLeaderboard to compute hiddenMemberIds once per workspace via workspaceMembershipService.getHiddenMemberIds(workspaceId) and pass that set into each createIndividualLeaderboard call, then remove the per-call fetch and use the passed hiddenMemberIds when filtering activityData.keySet().webapp/src/components/admin/UsersTable.tsx (1)
52-57: Export theUsersTablePropsinterface.Per coding guidelines, component prop interfaces should be exported so other modules can reuse them.
♻️ Proposed fix
-interface UsersTableProps { +export interface UsersTableProps { users: ExtendedUserTeams[]; teams: TeamInfo[]; isLoading?: boolean; onToggleHidden?: (userId: number, hidden: boolean) => void; }As per coding guidelines: "Export prop interfaces from components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/admin/UsersTable.tsx` around lines 52 - 57, The UsersTableProps interface is currently not exported; add the export modifier to the interface declaration (interface UsersTableProps) so other modules can import it—i.e., change the declaration for UsersTableProps to an exported interface and verify any consumers import { UsersTableProps } from this module; no other logic changes needed.server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/team/gitlab/GitLabTeamSyncService.java (1)
82-108: Prefer@RequiredArgsConstructorfor this new dependency.This change adds another required field/parameter to a hand-maintained constructor. The class already uses
finalcollaborators, so Lombok can generate the same injection point and keep future additions out of boilerplate.As per coding guidelines,
server/application-server/**/*.java:Use constructor injection via@requiredargsconstructorannotation.🤖 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/gitprovider/team/gitlab/GitLabTeamSyncService.java` around lines 82 - 108, The class GitLabTeamSyncService now has a hand-written constructor that must be replaced with Lombok-generated injection; remove the explicit constructor and annotate the class with `@RequiredArgsConstructor` (import lombok.RequiredArgsConstructor), keeping all collaborator fields (teamRepository, teamMembershipRepository, repositoryRepository, graphQlClientProvider, responseHandler, teamProcessor, gitLabUserService, gitProviderRepository, gitLabProperties, collaboratorRepository, transactionTemplate) as final so Lombok generates the same constructor for dependency injection.
🤖 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/gitprovider/repository/collaborator/RepositoryCollaboratorRepository.java`:
- Around line 26-37: The query in
RepositoryCollaboratorRepository.findByOrgLoginAndPermissions is too broad
because it only filters by c.repository.organization.login; add a
provider/server scope by including c.repository.provider.id (or the appropriate
provider identifier) in the WHERE clause, add a `@Param` for that provider id to
the method signature (e.g., Long providerId or UUID providerId), and update all
callers — notably GitLabTeamSyncService — to pass the provider id when invoking
findByOrgLoginAndPermissions so collaborators are only selected for the intended
provider/server.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/team/gitlab/GitLabTeamSyncService.java`:
- Around line 202-221: Phase C's stale-cleanup in syncTeamMembers() currently
deletes all TeamMembership rows not present in group.groupMembers, which removes
collaborator-derived memberships that Phase F
(addProjectCollaboratorsAsTeamMembers) later re-adds; change the logic so
collaborator-created memberships are preserved: add a source marker (e.g.,
TeamMembership.source or TeamMembership.isCollaborator) when creating entries in
addProjectCollaboratorsAsTeamMembers, then update the stale-removal in
syncTeamMembers() to skip/defer deletion of memberships with that marker (or
merge collaborator IDs into group.groupMembers before cleanup), ensuring Phase C
does not delete collaborator rows that Phase F owns.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/gitlab/GitLabUserService.java`:
- Around line 41-47: The resolveAvatarUrl method currently prefixes relative
avatar paths with gitLabProperties.defaultServerUrl(), which fails for
self-hosted/multi-provider setups; update resolveAvatarUrl (and all call sites)
to obtain the provider-scoped base URL (from the providerId or the
workspace/server URL passed into the service) and use that base instead of
gitLabProperties.defaultServerUrl() when avatarUrl.startsWith("/"); ensure the
method signature (resolveAvatarUrl) accepts or can access the provider-specific
server URL (e.g., add a parameter providerServerUrl or lookup by providerId) and
fallback to gitLabProperties.defaultServerUrl() only if no provider-specific URL
is available.
In `@webapp/src/components/profile/ProfileHeader.tsx`:
- Line 104: Replace the direct construction new URL(user.htmlUrl).host (which
can throw) with a safe parsing path: add or use a small helper (e.g.,
safeGetHostFromUrl or parseHost) that takes user.htmlUrl, returns the host
string or an empty string on failure, and call that helper where the component
renders {safeGetHostFromUrl(user.htmlUrl)}/{user.login}; ensure the helper
catches errors (try-catch) or validates the URL before calling new URL and
returns a fallback to avoid throwing during render.
In `@webapp/src/components/teams/TeamsPage.tsx`:
- Line 117: The code passes member.htmlUrl (optional) into ContributorCard which
expects a required htmlUrl string, risking undefined anchors; update the call
site in TeamsPage (where htmlUrl: member.htmlUrl is set) to either filter out
members missing htmlUrl or provide a safe fallback (e.g., htmlUrl:
member.htmlUrl ?? "" or only render ContributorCard when member.htmlUrl is
truthy), or alternatively make the Contributor interface in ContributorCard.tsx
accept htmlUrl?: string and handle missing links inside ContributorCard by
disabling the anchor or rendering plain text; choose one approach and apply it
consistently.
In `@webapp/src/lib/avatar.ts`:
- Around line 7-12: The initials-generation code in avatar.ts can crash for
whitespace-only names because parts becomes [""] and parts[0][0] is undefined;
update the logic in the function that computes initials (the block using parts =
name.trim().split(/\s+/)) to first normalize and early-return for empty trimmed
names (e.g., if name.trim() === ""), or filter out empty strings (e.g., parts =
name.trim().split(/\s+/).filter(Boolean)), then compute initials safely: if
parts.length >= 2 use first char of parts[0] and last char of
parts[parts.length-1], otherwise take up to two characters from the single part
and uppercase the result; ensure you handle single-character names without
indexing beyond length.
---
Nitpick comments:
In `@server/application-server/openapi.yaml`:
- Around line 910-946: Add an explicit 404 response to the PATCH operation with
operationId updateMemberVisibility (path
/workspaces/{workspaceSlug}/members/{userId}/hidden) to document the case where
the membership does not exist; update the OpenAPI fragment to include a "404"
response entry that references the RFC-7807/problem+json content type (or the
existing ProblemDetails schema used elsewhere) and a brief description like
"Membership not found" so clients can discover the missing-membership error
condition.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/team/gitlab/GitLabTeamSyncService.java`:
- Around line 82-108: The class GitLabTeamSyncService now has a hand-written
constructor that must be replaced with Lombok-generated injection; remove the
explicit constructor and annotate the class with `@RequiredArgsConstructor`
(import lombok.RequiredArgsConstructor), keeping all collaborator fields
(teamRepository, teamMembershipRepository, repositoryRepository,
graphQlClientProvider, responseHandler, teamProcessor, gitLabUserService,
gitProviderRepository, gitLabProperties, collaboratorRepository,
transactionTemplate) as final so Lombok generates the same constructor for
dependency injection.
In
`@server/application-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/LeaderboardService.java`:
- Around line 168-170: The code repeatedly calls
workspaceMembershipService.getHiddenMemberIds inside createIndividualLeaderboard
for each team causing N+1 reads; refactor createIndividualLeaderboard to accept
a Set<Long> hiddenMemberIds (or an Optional/nullable parameter) and update
createTeamLeaderboard to compute hiddenMemberIds once per workspace via
workspaceMembershipService.getHiddenMemberIds(workspaceId) and pass that set
into each createIndividualLeaderboard call, then remove the per-call fetch and
use the passed hiddenMemberIds when filtering activityData.keySet().
In `@webapp/src/components/admin/UsersTable.tsx`:
- Around line 52-57: The UsersTableProps interface is currently not exported;
add the export modifier to the interface declaration (interface UsersTableProps)
so other modules can import it—i.e., change the declaration for UsersTableProps
to an exported interface and verify any consumers import { UsersTableProps }
from this module; no other logic changes needed.
In `@webapp/src/routes/_authenticated/w/`$workspaceSlug/admin/_admin/members.tsx:
- Around line 56-60: Add leaderboard query invalidation inside the same
onSuccess block that currently calls queryClient.invalidateQueries for
getUsersWithTeamsQueryKey; call queryClient.invalidateQueries with the
leaderboard query key (e.g., getLeaderboardQueryKey({ path: { workspaceSlug:
workspaceSlug ?? "" } }) or the app's leaderboard query key pattern) so
leaderboard data is refreshed when visibility (hidden) is toggled, placing this
alongside the existing invalidate for getUsersWithTeamsQueryKey.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4ac8c5d-de01-4884-8c7a-99c2315ef1e9
📒 Files selected for processing (35)
server/application-server/openapi.yamlserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/repository/collaborator/RepositoryCollaboratorRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/team/TeamInfoDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/team/gitlab/GitLabTeamSyncService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserTeamsDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/gitlab/GitLabUserService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/leaderboard/LeaderboardService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceMembership.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceMembershipController.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceMembershipRepository.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceMembershipService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceProvisioningService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceTeamLabelService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/dto/WorkspaceMembershipDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/team/TeamService.javaserver/application-server/src/main/resources/db/changelog/1774900000000_changelog.xmlserver/application-server/src/main/resources/db/master.xmlwebapp/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.tswebapp/src/components/admin/AdminMembersPage.stories.tsxwebapp/src/components/admin/AdminMembersPage.tsxwebapp/src/components/admin/UsersTable.stories.tsxwebapp/src/components/admin/UsersTable.tsxwebapp/src/components/admin/types.tswebapp/src/components/core/Header.tsxwebapp/src/components/leaderboard/LeaderboardOverview.tsxwebapp/src/components/leaderboard/LeaderboardTable.tsxwebapp/src/components/profile/ProfileHeader.tsxwebapp/src/components/shared/ContributorCard.tsxwebapp/src/components/teams/TeamsPage.tsxwebapp/src/lib/avatar.tswebapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/members.tsx
✅ 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/workspace/WorkspaceMembership.java
- webapp/src/components/admin/AdminMembersPage.stories.tsx
- webapp/src/components/admin/UsersTable.stories.tsx
- server/application-server/src/main/resources/db/changelog/1774900000000_changelog.xml
Comment on lines
+26
to
+37
| @Query( | ||
| """ | ||
| SELECT DISTINCT c FROM RepositoryCollaborator c | ||
| JOIN FETCH c.user | ||
| WHERE c.repository.organization.login = :orgLogin | ||
| AND c.permission IN :permissions | ||
| """ | ||
| ) | ||
| List<RepositoryCollaborator> findByOrgLoginAndPermissions( | ||
| @Param("orgLogin") String orgLogin, | ||
| @Param("permissions") List<RepositoryCollaborator.Permission> permissions | ||
| ); |
Contributor
There was a problem hiding this comment.
Scope this collaborator lookup by provider/server.
Filtering only by c.repository.organization.login is too broad here. If the same subgroup path exists on another GitLab provider, Phase F can pull collaborators from the wrong server and create memberships on the wrong team. Please add c.repository.provider.id (or equivalent provider/server scoping) to this query and pass that value from GitLabTeamSyncService.
Suggested shape of the fix
`@Query`(
"""
SELECT DISTINCT c FROM RepositoryCollaborator c
JOIN FETCH c.user
WHERE c.repository.organization.login = :orgLogin
+ AND c.repository.provider.id = :providerId
AND c.permission IN :permissions
"""
)
List<RepositoryCollaborator> findByOrgLoginAndPermissions(
`@Param`("orgLogin") String orgLogin,
+ `@Param`("providerId") Long providerId,
`@Param`("permissions") List<RepositoryCollaborator.Permission> permissions
);🤖 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/gitprovider/repository/collaborator/RepositoryCollaboratorRepository.java`
around lines 26 - 37, The query in
RepositoryCollaboratorRepository.findByOrgLoginAndPermissions is too broad
because it only filters by c.repository.organization.login; add a
provider/server scope by including c.repository.provider.id (or the appropriate
provider identifier) in the WHERE clause, add a `@Param` for that provider id to
the method signature (e.g., Long providerId or UUID providerId), and update all
callers — notably GitLabTeamSyncService — to pass the provider id when invoking
findByOrgLoginAndPermissions so collaborators are only selected for the intended
provider/server.
Comment on lines
+202
to
+221
| // Phase F: Add project collaborators as team members | ||
| // Students who are direct project members (WRITE/TRIAGE) under a subgroup | ||
| // should appear as team members of that subgroup. | ||
| int totalCollaboratorMembers = 0; | ||
| for (Map.Entry<Long, Team> entry : syncedTeamsByNativeId.entrySet()) { | ||
| String fullPath = teamFullPathsByNativeId.get(entry.getKey()); | ||
| if (fullPath != null) { | ||
| try { | ||
| int added = addProjectCollaboratorsAsTeamMembers(entry.getValue().getId(), fullPath); | ||
| totalCollaboratorMembers += added; | ||
| } catch (Exception e) { | ||
| log.warn( | ||
| "Failed to sync collaborator-to-team for: teamSlug={}, error={}", | ||
| entry.getValue().getSlug(), | ||
| e.getMessage() | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| log.info("Phase F complete: groupPath={}, collaboratorMembers={}", groupFullPath, totalCollaboratorMembers); |
Contributor
There was a problem hiding this comment.
Don't let Phase C delete the memberships that Phase F adds.
syncTeamMembers() still removes every TeamMembership that is not present in group.groupMembers before this new phase runs. Because collaborator-derived rows live in the same table, every sync deletes them first and only restores them if Phase F succeeds; if this helper fails for one team, its student memberships disappear until the next successful run. Please preserve collaborator IDs during stale cleanup or track the membership source so Phase C can skip Phase F rows.
Also applies to: 681-729
🤖 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/gitprovider/team/gitlab/GitLabTeamSyncService.java`
around lines 202 - 221, Phase C's stale-cleanup in syncTeamMembers() currently
deletes all TeamMembership rows not present in group.groupMembers, which removes
collaborator-derived memberships that Phase F
(addProjectCollaboratorsAsTeamMembers) later re-adds; change the logic so
collaborator-created memberships are preserved: add a source marker (e.g.,
TeamMembership.source or TeamMembership.isCollaborator) when creating entries in
addProjectCollaboratorsAsTeamMembers, then update the stale-removal in
syncTeamMembers() to skip/defer deletion of memberships with that marker (or
merge collaborator IDs into group.groupMembers before cleanup), ensuring Phase C
does not delete collaborator rows that Phase F owns.
Comment on lines
+41
to
+47
| private String resolveAvatarUrl(@Nullable String avatarUrl) { | ||
| if (avatarUrl == null || avatarUrl.isEmpty()) { | ||
| return ""; | ||
| } | ||
| if (avatarUrl.startsWith("/")) { | ||
| return gitLabProperties.defaultServerUrl() + avatarUrl; | ||
| } |
Contributor
There was a problem hiding this comment.
Use provider-scoped server URL for relative avatar paths.
Relative avatars are currently expanded with gitLabProperties.defaultServerUrl(). For self-hosted/multi-provider setups, this can point to the wrong host. Resolve the base URL from providerId (or explicit workspace server URL) before prefixing "/..." paths.
💡 Proposed fix sketch
+import de.tum.in.www1.hephaestus.gitprovider.common.GitProviderRepository;
...
- private final GitLabProperties gitLabProperties;
+ private final GitLabProperties gitLabProperties;
+ private final GitProviderRepository gitProviderRepository;
...
- public GitLabUserService(UserRepository userRepository, GitLabProperties gitLabProperties) {
+ public GitLabUserService(
+ UserRepository userRepository,
+ GitLabProperties gitLabProperties,
+ GitProviderRepository gitProviderRepository
+ ) {
this.userRepository = userRepository;
this.gitLabProperties = gitLabProperties;
+ this.gitProviderRepository = gitProviderRepository;
}
- private String resolveAvatarUrl(`@Nullable` String avatarUrl) {
+ private String resolveAvatarUrl(`@Nullable` String avatarUrl, Long providerId) {
if (avatarUrl == null || avatarUrl.isEmpty()) {
return "";
}
if (avatarUrl.startsWith("/")) {
- return gitLabProperties.defaultServerUrl() + avatarUrl;
+ String serverUrl = gitProviderRepository
+ .findById(providerId)
+ .map(provider -> provider.getServerUrl())
+ .orElse(gitLabProperties.defaultServerUrl());
+ return serverUrl + avatarUrl;
}
return avatarUrl;
}
...
- String avatarUrl = resolveAvatarUrl(dto.avatarUrl());
+ String avatarUrl = resolveAvatarUrl(dto.avatarUrl(), providerId);
...
- String resolvedAvatarUrl = resolveAvatarUrl(avatarUrl);
+ String resolvedAvatarUrl = resolveAvatarUrl(avatarUrl, providerId);🤖 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/gitprovider/user/gitlab/GitLabUserService.java`
around lines 41 - 47, The resolveAvatarUrl method currently prefixes relative
avatar paths with gitLabProperties.defaultServerUrl(), which fails for
self-hosted/multi-provider setups; update resolveAvatarUrl (and all call sites)
to obtain the provider-scoped base URL (from the providerId or the
workspace/server URL passed into the service) and use that base instead of
gitLabProperties.defaultServerUrl() when avatarUrl.startsWith("/"); ensure the
method signature (resolveAvatarUrl) accepts or can access the provider-specific
server URL (e.g., add a parameter providerServerUrl or lookup by providerId) and
fallback to gitLabProperties.defaultServerUrl() only if no provider-specific URL
is available.
| rel="noopener noreferrer" | ||
| > | ||
| github.com/{user.login} | ||
| {user.htmlUrl ? new URL(user.htmlUrl).host : ""}/{user.login} |
Contributor
There was a problem hiding this comment.
new URL() can throw on malformed htmlUrl.
If user.htmlUrl contains a malformed URL string (e.g., missing protocol or invalid format), new URL(user.htmlUrl) will throw a TypeError, crashing the component during render. Consider wrapping in a try-catch or using a safe URL parser.
Proposed fix using a safe helper
+const getSafeHost = (url: string | undefined): string => {
+ if (!url) return "";
+ try {
+ return new URL(url).host;
+ } catch {
+ return "";
+ }
+};
+
// In the JSX:
-{user.htmlUrl ? new URL(user.htmlUrl).host : ""}/{user.login}
+{getSafeHost(user.htmlUrl)}/{user.login}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/profile/ProfileHeader.tsx` at line 104, Replace the
direct construction new URL(user.htmlUrl).host (which can throw) with a safe
parsing path: add or use a small helper (e.g., safeGetHostFromUrl or parseHost)
that takes user.htmlUrl, returns the host string or an empty string on failure,
and call that helper where the component renders
{safeGetHostFromUrl(user.htmlUrl)}/{user.login}; ensure the helper catches
errors (try-catch) or validates the URL before calling new URL and returns a
fallback to avoid throwing during render.
| name: member.name, | ||
| avatarUrl: member.avatarUrl, | ||
| htmlUrl: `https://github.com/${member.login}`, | ||
| htmlUrl: member.htmlUrl, |
Contributor
There was a problem hiding this comment.
Potential undefined htmlUrl passed to ContributorCard.
The member.htmlUrl field is optional (htmlUrl?: string per the type definition in types.gen.ts), but the Contributor interface in ContributorCard.tsx expects htmlUrl: string (required). If a member lacks an htmlUrl, this will pass undefined, potentially rendering an invalid anchor link.
Consider providing a fallback or filtering out members without htmlUrl:
Proposed fix
return filtered.map((member) => ({
id: member.id,
login: member.login,
name: member.name,
avatarUrl: member.avatarUrl,
- htmlUrl: member.htmlUrl,
+ htmlUrl: member.htmlUrl ?? "",
}));📝 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.
Suggested change
| htmlUrl: member.htmlUrl, | |
| htmlUrl: member.htmlUrl ?? "", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/components/teams/TeamsPage.tsx` at line 117, The code passes
member.htmlUrl (optional) into ContributorCard which expects a required htmlUrl
string, risking undefined anchors; update the call site in TeamsPage (where
htmlUrl: member.htmlUrl is set) to either filter out members missing htmlUrl or
provide a safe fallback (e.g., htmlUrl: member.htmlUrl ?? "" or only render
ContributorCard when member.htmlUrl is truthy), or alternatively make the
Contributor interface in ContributorCard.tsx accept htmlUrl?: string and handle
missing links inside ContributorCard by disabling the anchor or rendering plain
text; choose one approach and apply it consistently.
Comment on lines
+7
to
+12
| if (name) { | ||
| const parts = name.trim().split(/\s+/); | ||
| if (parts.length >= 2) { | ||
| return (parts[0][0] + parts[parts.length - 1][0]).toUpperCase(); | ||
| } | ||
| return name.slice(0, 2).toUpperCase(); |
Contributor
There was a problem hiding this comment.
Edge case: whitespace-only name can cause runtime error.
If name is whitespace-only (e.g., " "), the truthy check passes but name.trim().split(/\s+/) yields [""]. Accessing parts[0][0] on an empty string returns undefined, and string concatenation with undefined values produces unexpected results.
Proposed fix
export function getInitials(name?: string | null, login?: string | null): string {
- if (name) {
- const parts = name.trim().split(/\s+/);
+ const trimmedName = name?.trim();
+ if (trimmedName) {
+ const parts = trimmedName.split(/\s+/);
if (parts.length >= 2) {
return (parts[0][0] + parts[parts.length - 1][0]).toUpperCase();
}
- return name.slice(0, 2).toUpperCase();
+ return trimmedName.slice(0, 2).toUpperCase();
}📝 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.
Suggested change
| if (name) { | |
| const parts = name.trim().split(/\s+/); | |
| if (parts.length >= 2) { | |
| return (parts[0][0] + parts[parts.length - 1][0]).toUpperCase(); | |
| } | |
| return name.slice(0, 2).toUpperCase(); | |
| if (trimmedName) { | |
| const parts = trimmedName.split(/\s+/); | |
| if (parts.length >= 2) { | |
| return (parts[0][0] + parts[parts.length - 1][0]).toUpperCase(); | |
| } | |
| return trimmedName.slice(0, 2).toUpperCase(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/lib/avatar.ts` around lines 7 - 12, The initials-generation code
in avatar.ts can crash for whitespace-only names because parts becomes [""] and
parts[0][0] is undefined; update the logic in the function that computes
initials (the block using parts = name.trim().split(/\s+/)) to first normalize
and early-return for empty trimmed names (e.g., if name.trim() === ""), or
filter out empty strings (e.g., parts =
name.trim().split(/\s+/).filter(Boolean)), then compute initials safely: if
parts.length >= 2 use first char of parts[0] and last char of
parts[parts.length-1], otherwise take up to two characters from the single part
and uppercase the result; ensure you handle single-character names without
indexing beyond length.
FelixTJDietrich
force-pushed
the
feat/961-gitlab-workspace-auto-discover
branch
from
d0f80c6 to
bab708c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webapp/src/components/admin/AdminFeaturesSettings.stories.tsx (1)
53-59:⚠️ Potential issue | 🟡 MinorVariant stories now misrepresent leagues state.
Because
meta.argssetsleaguesEnabled: false(Line 40), these variants inheritfalseunless overridden.AllEnabled,GamificationOnly, andSavingshould explicitly setleaguesEnabled: truewhere intended.💡 Suggested fix
export const AllEnabled: Story = { args: { practicesEnabled: true, achievementsEnabled: true, leaderboardEnabled: true, progressionEnabled: true, + leaguesEnabled: true, }, }; @@ export const GamificationOnly: Story = { args: { practicesEnabled: false, achievementsEnabled: true, leaderboardEnabled: true, progressionEnabled: true, + leaguesEnabled: true, }, }; @@ export const Saving: Story = { args: { practicesEnabled: true, achievementsEnabled: true, leaderboardEnabled: true, progressionEnabled: true, + leaguesEnabled: true, isSaving: true, }, };As per coding guidelines, "Stories should be colocated with components and cover default state, all variants, loading state, error state, and edge cases."
Also applies to: 73-79, 83-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/admin/AdminFeaturesSettings.stories.tsx` around lines 53 - 59, The variant stories (AllEnabled, GamificationOnly, Saving) are inheriting leaguesEnabled:false from meta.args and thus misrepresent the intended state; update each story's args (e.g., the AllEnabled Story.args, GamificationOnly Story.args, and Saving Story.args) to explicitly set leaguesEnabled: true where leagues should be enabled so they do not inherit the false default from meta.args and correctly reflect the intended variants.webapp/src/components/leaderboard/LeaderboardTable.tsx (1)
47-49:⚠️ Potential issue | 🟡 MinorLoading skeleton ignores
leaguesEnabledand shows a stale column layout.When leagues are disabled, the table hides the League column, but the loading skeleton still renders it. This creates a temporary layout shift/inconsistency in Line 47’s loading path.
💡 Suggested fix
export function LeaderboardTable({ leaderboard = [], isLoading, variant, currentUser, onUserClick, onTeamClick, teamLabelsById, providerType = "GITHUB", leaguesEnabled = true, }: LeaderboardTableProps) { if (isLoading) { - return <LeaderboardTableSkeleton />; + return <LeaderboardTableSkeleton leaguesEnabled={leaguesEnabled} />; } @@ -function LeaderboardTableSkeleton() { +function LeaderboardTableSkeleton({ leaguesEnabled = true }: { leaguesEnabled?: boolean }) { return ( <Table> <TableHeader> <TableRow> <TableHead className="text-center w-16">Rank</TableHead> - <TableHead className="text-center w-20">League</TableHead> + {leaguesEnabled && <TableHead className="text-center w-20">League</TableHead>} <TableHead>Contributor</TableHead> <TableHead className="text-center">Score</TableHead> <TableHead>Activity</TableHead> </TableRow> </TableHeader> <TableBody> {Array.from({ length: 10 }, (_, idx) => `skeleton-${idx}`).map((key, idx) => ( <TableRow key={key}> <TableCell> <Skeleton className="h-5 w-7" style={{ width: `${20 + 1 * idx}px` }} /> </TableCell> - <TableCell> - <Skeleton className="h-8 w-8 mx-auto" /> - </TableCell> + {leaguesEnabled && ( + <TableCell> + <Skeleton className="h-8 w-8 mx-auto" /> + </TableCell> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/leaderboard/LeaderboardTable.tsx` around lines 47 - 49, The loading path in LeaderboardTable renders LeaderboardTableSkeleton regardless of leaguesEnabled, causing a stale League column; update the isLoading branch in LeaderboardTable so it renders a skeleton that respects the same leaguesEnabled condition (e.g., pass leaguesEnabled into LeaderboardTableSkeleton or render an alternate skeleton variant) so the skeleton's column layout matches the final table; locate the isLoading check in LeaderboardTable and adjust the call to LeaderboardTableSkeleton (or create/use a prop like leaguesEnabled) accordingly.
♻️ Duplicate comments (1)
webapp/src/components/profile/ProfileHeader.tsx (1)
110-110:⚠️ Potential issue | 🟠 MajorGuard
new URL()to avoid render-time crashes.Line 110 calls
new URL(user.htmlUrl)directly during render. If the backend returns a malformed/non-absolute URL, this throws and breaks the profile header.💡 Suggested fix
+const getSafeHostFromUrl = (value?: string): string => { + if (!value) return ""; + try { + return new URL(value).host; + } catch { + return ""; + } +}; + // ... - {user.htmlUrl ? new URL(user.htmlUrl).host : ""}/{user.login} + {getSafeHostFromUrl(user.htmlUrl)}/{user.login}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/profile/ProfileHeader.tsx` at line 110, The render currently calls new URL(user.htmlUrl) directly which can throw for malformed or non-absolute strings and crash ProfileHeader; wrap the URL parsing in a safe guard such as a small helper (e.g., getHostFromUrl or safeParseHost) that checks for a non-empty string and either tests for an absolute URL scheme (startsWith 'http') or performs the new URL(...) inside a try/catch and returns an empty string on error, then use that helper (or a memoized value computed above render) in place of new URL(user.htmlUrl).host so rendering never throws when user.htmlUrl is invalid.
🧹 Nitpick comments (4)
webapp/src/components/core/sidebar/NavAdmin.tsx (1)
11-17: Consider extracting the props type to an exported interface.The inline type works but the coding guidelines for
webapp/src/components/**/*.{ts,tsx}recommend exporting prop interfaces from components for reusability.Suggested refactor
+export interface NavAdminProps { + workspaceSlug: string; + achievementsEnabled: boolean; +} + -export function NavAdmin({ - workspaceSlug, - achievementsEnabled, -}: { - workspaceSlug: string; - achievementsEnabled: boolean; -}) { +export function NavAdmin({ workspaceSlug, achievementsEnabled }: NavAdminProps) {As per coding guidelines: "
webapp/src/components/**/*.{ts,tsx}: Export prop interfaces from components"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/core/sidebar/NavAdmin.tsx` around lines 11 - 17, Extract the inline prop type into an exported interface (e.g., export interface NavAdminProps) that declares workspaceSlug: string and achievementsEnabled: boolean, then update the NavAdmin function signature to accept props: NavAdminProps (or use function NavAdmin(props: NavAdminProps) / NavAdmin({ ... }: NavAdminProps)); export the interface so other components can import and reuse it; ensure no behavior changes to NavAdmin, only the type declaration is moved to the exported interface.webapp/src/components/admin/AdminSettingsPage.stories.tsx (1)
99-99: Add a dedicatedleaguesEnabled: truestory variant.With the new default arg at Line 99, it would help to add at least one explicit enabled-state story so the new leagues UI path is exercised in Storybook previews.
As per coding guidelines, "Stories should be colocated with components and cover default state, all variants, loading state, error state, and edge cases."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/admin/AdminSettingsPage.stories.tsx` at line 99, The story file AdminSettingsPage.stories.tsx now defaults leaguesEnabled to false; add an explicit story variant that sets leaguesEnabled: true so Storybook renders the leagues UI path — create a new exported story (e.g., export const LeaguesEnabled or WithLeagues) that spreads the base/default args and overrides leaguesEnabled: true, ensure it uses the same component/story setup (the existing default export and story renderer) so the new variant appears alongside the default in Storybook.webapp/src/components/leaderboard/LeaderboardPage.tsx (1)
11-39: Export the page props interface for reuse.
LeaderboardPagePropsshould be exported to align with the component contract-sharing guideline.As per coding guidelines,
webapp/src/components/**/*.{ts,tsx}: "Export prop interfaces from components".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/leaderboard/LeaderboardPage.tsx` around lines 11 - 39, The interface LeaderboardPageProps is currently declared but not exported; update its declaration to export it (i.e., export interface LeaderboardPageProps) so other modules can reuse the component props, and update any local imports/usages if needed to import { LeaderboardPageProps } where the type is referenced; ensure the symbol name LeaderboardPageProps remains unchanged for compatibility with LeaderboardPage and any consumers.webapp/src/components/profile/ProfilePage.tsx (1)
9-25: Export the component props interface.
ProfilePropsis used as a component prop contract but is not exported. Please export it for reuse and consistency with project conventions.As per coding guidelines,
webapp/src/components/**/*.{ts,tsx}: "Export prop interfaces from components".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/profile/ProfilePage.tsx` around lines 9 - 25, The ProfileProps interface is currently unexported; export it so other modules and the ProfilePage component consumers can reuse the prop contract—update the declaration of ProfileProps (used by the ProfilePage component) to be exported (e.g., add export before the interface or export it as a type) and ensure any imports elsewhere reference the exported symbol.
🤖 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/openapi.yaml`:
- Around line 5028-5029: Update the UserTeams OpenAPI schema to mark the hidden
property as required: in the UserTeams schema's "required" array add "hidden" so
the schema matches the service layer behavior (fromUserWithScopeSettings()
always populates hidden); locate the UserTeams schema and its required array and
include "hidden" alongside the other required fields to ensure generated clients
and validators treat it as mandatory.
- Around line 5247-5249: The WorkspaceMembership OpenAPI schema currently lists
a boolean property "hidden" but doesn't mark it as required; update the
WorkspaceMembership schema in openapi.yaml to include "hidden" in the required
array so the schema reflects the non-nullable DB constraint and DTO (i.e., add
"hidden" to the schema's required fields for WorkspaceMembership); verify the
"hidden" property remains type: boolean (non-nullable) and the schema validation
passes.
In `@webapp/src/hooks/use-workspace-features.ts`:
- Line 49: The hook useWorkspaceFeatures currently sets leaguesEnabled to
workspace?.leaguesEnabled ?? false which contradicts the hook's contract that
feature flags should default to true while loading; update the initialization
for leaguesEnabled in use-workspace-features (the leaguesEnabled binding) to
default to true (e.g., use workspace?.leaguesEnabled ?? true) so it matches the
other flags and the comments, ensuring no UI flicker during loading.
---
Outside diff comments:
In `@webapp/src/components/admin/AdminFeaturesSettings.stories.tsx`:
- Around line 53-59: The variant stories (AllEnabled, GamificationOnly, Saving)
are inheriting leaguesEnabled:false from meta.args and thus misrepresent the
intended state; update each story's args (e.g., the AllEnabled Story.args,
GamificationOnly Story.args, and Saving Story.args) to explicitly set
leaguesEnabled: true where leagues should be enabled so they do not inherit the
false default from meta.args and correctly reflect the intended variants.
In `@webapp/src/components/leaderboard/LeaderboardTable.tsx`:
- Around line 47-49: The loading path in LeaderboardTable renders
LeaderboardTableSkeleton regardless of leaguesEnabled, causing a stale League
column; update the isLoading branch in LeaderboardTable so it renders a skeleton
that respects the same leaguesEnabled condition (e.g., pass leaguesEnabled into
LeaderboardTableSkeleton or render an alternate skeleton variant) so the
skeleton's column layout matches the final table; locate the isLoading check in
LeaderboardTable and adjust the call to LeaderboardTableSkeleton (or create/use
a prop like leaguesEnabled) accordingly.
---
Duplicate comments:
In `@webapp/src/components/profile/ProfileHeader.tsx`:
- Line 110: The render currently calls new URL(user.htmlUrl) directly which can
throw for malformed or non-absolute strings and crash ProfileHeader; wrap the
URL parsing in a safe guard such as a small helper (e.g., getHostFromUrl or
safeParseHost) that checks for a non-empty string and either tests for an
absolute URL scheme (startsWith 'http') or performs the new URL(...) inside a
try/catch and returns an empty string on error, then use that helper (or a
memoized value computed above render) in place of new URL(user.htmlUrl).host so
rendering never throws when user.htmlUrl is invalid.
---
Nitpick comments:
In `@webapp/src/components/admin/AdminSettingsPage.stories.tsx`:
- Line 99: The story file AdminSettingsPage.stories.tsx now defaults
leaguesEnabled to false; add an explicit story variant that sets leaguesEnabled:
true so Storybook renders the leagues UI path — create a new exported story
(e.g., export const LeaguesEnabled or WithLeagues) that spreads the base/default
args and overrides leaguesEnabled: true, ensure it uses the same component/story
setup (the existing default export and story renderer) so the new variant
appears alongside the default in Storybook.
In `@webapp/src/components/core/sidebar/NavAdmin.tsx`:
- Around line 11-17: Extract the inline prop type into an exported interface
(e.g., export interface NavAdminProps) that declares workspaceSlug: string and
achievementsEnabled: boolean, then update the NavAdmin function signature to
accept props: NavAdminProps (or use function NavAdmin(props: NavAdminProps) /
NavAdmin({ ... }: NavAdminProps)); export the interface so other components can
import and reuse it; ensure no behavior changes to NavAdmin, only the type
declaration is moved to the exported interface.
In `@webapp/src/components/leaderboard/LeaderboardPage.tsx`:
- Around line 11-39: The interface LeaderboardPageProps is currently declared
but not exported; update its declaration to export it (i.e., export interface
LeaderboardPageProps) so other modules can reuse the component props, and update
any local imports/usages if needed to import { LeaderboardPageProps } where the
type is referenced; ensure the symbol name LeaderboardPageProps remains
unchanged for compatibility with LeaderboardPage and any consumers.
In `@webapp/src/components/profile/ProfilePage.tsx`:
- Around line 9-25: The ProfileProps interface is currently unexported; export
it so other modules and the ProfilePage component consumers can reuse the prop
contract—update the declaration of ProfileProps (used by the ProfilePage
component) to be exported (e.g., add export before the interface or export it as
a type) and ensure any imports elsewhere reference the exported symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2548a467-c246-495d-a094-03e8eccf52fd
📒 Files selected for processing (29)
server/application-server/openapi.yamlserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceFeatures.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSettingsService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/dto/UpdateWorkspaceFeaturesRequestDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/dto/WorkspaceDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/dto/WorkspaceListItemDTO.javaserver/application-server/src/main/resources/db/changelog/1775000000000_changelog.xmlserver/application-server/src/main/resources/db/master.xmlwebapp/src/api/types.gen.tswebapp/src/components/admin/AdminFeaturesSettings.stories.tsxwebapp/src/components/admin/AdminFeaturesSettings.tsxwebapp/src/components/admin/AdminSettingsPage.stories.tsxwebapp/src/components/admin/AdminSettingsPage.tsxwebapp/src/components/core/sidebar/AppSidebar.stories.tsxwebapp/src/components/core/sidebar/AppSidebar.tsxwebapp/src/components/core/sidebar/NavAdmin.stories.tsxwebapp/src/components/core/sidebar/NavAdmin.tsxwebapp/src/components/core/sidebar/WorkspaceSwitcher.stories.tsxwebapp/src/components/leaderboard/LeaderboardFilter.tsxwebapp/src/components/leaderboard/LeaderboardOverview.tsxwebapp/src/components/leaderboard/LeaderboardPage.tsxwebapp/src/components/leaderboard/LeaderboardTable.tsxwebapp/src/components/leaderboard/SortFilter.tsxwebapp/src/components/profile/ProfileHeader.tsxwebapp/src/components/profile/ProfilePage.tsxwebapp/src/hooks/use-workspace-features.tswebapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsxwebapp/src/routes/_authenticated/w/$workspaceSlug/index.tsxwebapp/src/routes/_authenticated/w/$workspaceSlug/user/$username/index.tsx
✅ Files skipped from review due to trivial changes (5)
- webapp/src/components/core/sidebar/NavAdmin.stories.tsx
- server/application-server/src/main/resources/db/changelog/1775000000000_changelog.xml
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSettingsService.java
- server/application-server/src/main/resources/db/master.xml
- webapp/src/components/core/sidebar/WorkspaceSwitcher.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- webapp/src/components/leaderboard/LeaderboardOverview.tsx
- webapp/src/api/types.gen.ts
Comment on lines
+5028
to
+5029
| hidden: | ||
| type: boolean |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the Java DTO or service that populates UserTeams to see if hidden is always set
rg -n "UserTeams" --type java -A 10 -B 2 | head -100Repository: ls1intum/Hephaestus
Length of output: 15644
🏁 Script executed:
# Find the UserTeamsDTO class definition
fd -t f "UserTeamsDTO" -o "*.java" | head -5Repository: ls1intum/Hephaestus
Length of output: 194
🏁 Script executed:
# Search for the UserTeamsDTO class definition
rg -n "class UserTeamsDTO|record UserTeamsDTO" --type java -A 20Repository: ls1intum/Hephaestus
Length of output: 3047
🏁 Script executed:
# Check the OpenAPI spec around lines 5028-5048 to see the full UserTeams definition and required fields
sed -n '5020,5060p' server/application-server/openapi.yamlRepository: ls1intum/Hephaestus
Length of output: 861
🏁 Script executed:
# Look for the complete factory method in UserTeamsDTO
cat -n server/application-server/src/main/java/de/tum/in/www1/hephaestus/gitprovider/user/UserTeamsDTO.javaRepository: ls1intum/Hephaestus
Length of output: 2579
🏁 Script executed:
# Search for any direct instantiations of UserTeamsDTO (new UserTeamsDTO or constructor calls)
rg -n "new UserTeamsDTO|UserTeamsDTO\(" --type java -B 2 -A 2Repository: ls1intum/Hephaestus
Length of output: 1428
Mark hidden as required in the UserTeams OpenAPI schema.
The hidden field is always populated by the service layer (via fromUserWithScopeSettings() factory method), so it should be added to the required array at lines 5044-5048 for accurate API contract definition and stricter type safety in generated clients.
🤖 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 5028 - 5029, Update the
UserTeams OpenAPI schema to mark the hidden property as required: in the
UserTeams schema's "required" array add "hidden" so the schema matches the
service layer behavior (fromUserWithScopeSettings() always populates hidden);
locate the UserTeams schema and its required array and include "hidden"
alongside the other required fields to ensure generated clients and validators
treat it as mandatory.
Comment on lines
+5247
to
+5249
| hidden: | ||
| type: boolean | ||
| description: Whether the member is hidden from the leaderboard |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the Java DTO to see if hidden has a default or is always set
rg -n "WorkspaceMembershipDTO|WorkspaceMembership" --type java -A 15 | head -150Repository: ls1intum/Hephaestus
Length of output: 23193
🏁 Script executed:
#!/bin/bash
# Find the WorkspaceMembershipDTO class definition
find . -name "*WorkspaceMembershipDTO*" -o -name "*WorkspaceMembership.java" | head -20Repository: ls1intum/Hephaestus
Length of output: 275
🏁 Script executed:
#!/bin/bash
# Search for WorkspaceMembershipDTO class definition
rg -n "class WorkspaceMembershipDTO" --type java -A 20Repository: ls1intum/Hephaestus
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look at the OpenAPI schema around the specified lines
sed -n '5240,5260p' server/application-server/openapi.yamlRepository: ls1intum/Hephaestus
Length of output: 740
🏁 Script executed:
#!/bin/bash
# Read the WorkspaceMembershipDTO class
cat -n server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/dto/WorkspaceMembershipDTO.javaRepository: ls1intum/Hephaestus
Length of output: 1969
🏁 Script executed:
#!/bin/bash
# Read the WorkspaceMembership entity class
cat -n server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceMembership.javaRepository: ls1intum/Hephaestus
Length of output: 6863
🏁 Script executed:
#!/bin/bash
# Look for the required fields in the OpenAPI schema for WorkspaceMembership
sed -n '5230,5280p' server/application-server/openapi.yamlRepository: ls1intum/Hephaestus
Length of output: 1637
🏁 Script executed:
#!/bin/bash
# Check for the complete WorkspaceMembership schema definition with required fields
sed -n '5230,5310p' server/application-server/openapi.yamlRepository: ls1intum/Hephaestus
Length of output: 2689
🏁 Script executed:
#!/bin/bash
# Search for UserTeams schema to understand the comparison
rg -n "UserTeams:" server/application-server/openapi.yaml -A 30Repository: ls1intum/Hephaestus
Length of output: 813
🏁 Script executed:
#!/bin/bash
# Search more thoroughly for WorkspaceMembership required section
rg -n "WorkspaceMembership:" server/application-server/openapi.yaml -A 50 | grep -E "(required:|WorkspaceMembership|hidden|userName)" | head -30Repository: ls1intum/Hephaestus
Length of output: 339
🏁 Script executed:
#!/bin/bash
# Look at a wider range to see the complete WorkspaceMembership schema
sed -n '5237,5290p' server/application-server/openapi.yamlRepository: ls1intum/Hephaestus
Length of output: 1924
🏁 Script executed:
#!/bin/bash
# Get the exact line numbers for WorkspaceMembership schema and look for required
sed -n '5239,5285p' server/application-server/openapi.yaml | cat -nRepository: ls1intum/Hephaestus
Length of output: 2022
Add hidden to the required fields list in the WorkspaceMembership OpenAPI schema.
The hidden field is always populated by the server—the entity has a nullable: false database constraint and a default value of false, and the DTO uses a primitive boolean rather than Optional. Since the field is never null, it should be marked as required in the OpenAPI schema to accurately reflect the API contract and improve client-side type safety.
🤖 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 5247 - 5249, The
WorkspaceMembership OpenAPI schema currently lists a boolean property "hidden"
but doesn't mark it as required; update the WorkspaceMembership schema in
openapi.yaml to include "hidden" in the required array so the schema reflects
the non-nullable DB constraint and DTO (i.e., add "hidden" to the schema's
required fields for WorkspaceMembership); verify the "hidden" property remains
type: boolean (non-nullable) and the schema validation passes.
| achievementsEnabled: workspace?.achievementsEnabled ?? true, | ||
| leaderboardEnabled: workspace?.leaderboardEnabled ?? true, | ||
| progressionEnabled: workspace?.progressionEnabled ?? true, | ||
| leaguesEnabled: workspace?.leaguesEnabled ?? false, |
Contributor
There was a problem hiding this comment.
leaguesEnabled default contradicts the hook contract.
Line 49 defaults to false, while the comments (Lines 18 and 41) say flags default to true during loading to avoid flicker. Please align code and contract.
💡 Suggested fix
- leaguesEnabled: workspace?.leaguesEnabled ?? false,
+ leaguesEnabled: workspace?.leaguesEnabled ?? true,📝 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.
Suggested change
| leaguesEnabled: workspace?.leaguesEnabled ?? false, | |
| leaguesEnabled: workspace?.leaguesEnabled ?? true, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/src/hooks/use-workspace-features.ts` at line 49, The hook
useWorkspaceFeatures currently sets leaguesEnabled to workspace?.leaguesEnabled
?? false which contradicts the hook's contract that feature flags should default
to true while loading; update the initialization for leaguesEnabled in
use-workspace-features (the leaguesEnabled binding) to default to true (e.g.,
use workspace?.leaguesEnabled ?? true) so it matches the other flags and the
comments, ensuring no UI flicker during loading.
…oast When leaderboard or achievements are disabled, navigating to their routes now silently redirects to the profile page instead of showing an error toast. Users shouldn't see error messages for expected behavior when features are simply turned off.
github-actions
bot
added
ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/routes/_authenticated/w/$workspaceSlug/achievements.tsx (1)
25-35:⚠️ Potential issue | 🟡 MinorUse
userProfile?.usernameas fallback in the redirect guard for disabled achievements.The redirect guard at line 25 requires
usernameto be truthy before redirecting when achievements are disabled. However,usernameis typed asstring | undefinedin AuthContext, and if it's temporarily undefined, the guard skips and the loading spinner persists indefinitely. SinceuserProfileis populated concurrently withusernamefrom the Keycloak service andUserProfile.usernameis a guaranteed non-optional string, use it as a fallback source:if (!isLoading && !achievementsEnabled && workspaceSlug && (username || userProfile?.username)) { // redirect logic }This aligns the guard with the render logic below (line 46), which already uses
username || ""as a fallback for the component props.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/routes/_authenticated/w/`$workspaceSlug/achievements.tsx around lines 25 - 35, The redirect guard currently requires username to be truthy which can block the silent redirect while username is briefly undefined; update the condition in the useEffect that checks isLoading/achievementsEnabled to use a fallback to userProfile?.username (i.e. require workspaceSlug && (username || userProfile?.username)), and when calling navigate set the params.username value to the same fallback (username || userProfile?.username) so the redirect occurs even if AuthContext.username is temporarily undefined; update references to isLoading, achievementsEnabled, workspaceSlug, username, userProfile, and navigate accordingly.
🧹 Nitpick comments (2)
webapp/src/components/core/sidebar/NavAdmin.tsx (1)
11-17: Extract and export the props interface.The inline props type should be extracted to an exported interface per coding guidelines.
♻️ Suggested refactor
+export interface NavAdminProps { + workspaceSlug: string; + achievementsEnabled: boolean; +} + -export function NavAdmin({ - workspaceSlug, - achievementsEnabled, -}: { - workspaceSlug: string; - achievementsEnabled: boolean; -}) { +export function NavAdmin({ workspaceSlug, achievementsEnabled }: NavAdminProps) {As per coding guidelines:
webapp/src/components/**/*.{ts,tsx}: "Export prop interfaces from components".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/core/sidebar/NavAdmin.tsx` around lines 11 - 17, Extract the inline props type for NavAdmin into a new exported interface (e.g., export interface NavAdminProps { workspaceSlug: string; achievementsEnabled: boolean; }) and update the function signature to accept NavAdminProps (export function NavAdmin(props: NavAdminProps) or export function NavAdmin({ workspaceSlug, achievementsEnabled }: NavAdminProps)). Ensure the interface is exported so other files can import it and update any local references/usages accordingly.webapp/src/components/leaderboard/LeaderboardTable.tsx (1)
178-221: Skeleton always renders the League column, ignoringleaguesEnabledandisTeam.
LeaderboardTableSkeletonunconditionally renders the "League" header and cell (lines 184, 196-198), causing a visual mismatch when leagues are disabled or in team mode. Consider passingleaguesEnabledandvariantto the skeleton and conditionally rendering the League column to match the actual table structure.♻️ Suggested fix
-function LeaderboardTableSkeleton() { +function LeaderboardTableSkeleton({ + leaguesEnabled = true, + isTeam = false, +}: { + leaguesEnabled?: boolean; + isTeam?: boolean; +}) { return ( <Table> <TableHeader> <TableRow> <TableHead className="text-center w-16">Rank</TableHead> - <TableHead className="text-center w-20">League</TableHead> - <TableHead>Contributor</TableHead> + {!isTeam && leaguesEnabled && <TableHead className="text-center w-20">League</TableHead>} + <TableHead>{isTeam ? "Team" : "Contributor"}</TableHead> <TableHead className="text-center">Score</TableHead> <TableHead>Activity</TableHead> </TableRow> </TableHeader> <TableBody> {Array.from({ length: 10 }, (_, idx) => `skeleton-${idx}`).map((key, idx) => ( <TableRow key={key}> <TableCell> <Skeleton className="h-5 w-7" style={{ width: `${20 + 1 * idx}px` }} /> </TableCell> - <TableCell> - <Skeleton className="h-8 w-8 mx-auto" /> - </TableCell> + {!isTeam && leaguesEnabled && ( + <TableCell> + <Skeleton className="h-8 w-8 mx-auto" /> + </TableCell> + )}Then update the call site:
if (isLoading) { - return <LeaderboardTableSkeleton />; + return <LeaderboardTableSkeleton leaguesEnabled={leaguesEnabled} isTeam={isTeam} />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/leaderboard/LeaderboardTable.tsx` around lines 178 - 221, LeaderboardTableSkeleton currently always renders the "League" column; change it to accept props (e.g., leaguesEnabled: boolean and isTeam: boolean) and conditionally render the League TableHead and the corresponding TableCell only when leaguesEnabled is true and isTeam is false so the skeleton matches the real table layout; update any call sites of LeaderboardTableSkeleton to pass the appropriate leaguesEnabled and isTeam values from the parent component (the same props used to render the real Leaderboard table).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/compose.core.yaml`:
- Around line 426-427: The allowedClockSkew value is inconsistent between the
gitlab-lrz OIDC provider entry (currently "allowedClockSkew": "300") and the
realm JSON (currently "allowedClockSkew": "21700"); choose and apply a single
canonical skew value across both configurations to avoid divergent token
validation. Update the docker/compose core OIDC block where "allowedClockSkew"
appears (gitlab-lrz provider) to match the canonical value used in
keycloak-hephaestus-realm-example-config.json (or vice versa), ensuring the same
numeric value and type (seconds as a number/string consistently) is used in both
the docker compose snippet and the realm JSON to unify behavior.
In `@server/application-server/keycloak-hephaestus-realm-example-config.json`:
- Around line 1559-1560: The allowedClockSkew JSON field is set to an unsafe
high value ("21700"); change the "allowedClockSkew" entry to be configurable via
an environment variable (e.g., KEYCLOAK_ALLOWED_CLOCK_SKEW) with a safe default
of 60 (seconds) and ensure it's stored as an integer/number, not an excessively
large literal string; update any template or loader code that emits
keycloak-hephaestus-realm-example-config.json to read
process.env.KEYCLOAK_ALLOWED_CLOCK_SKEW || 60 and use that numeric value for the
"allowedClockSkew" property (referencing the "allowedClockSkew" key and the
surrounding client config such as "clientAuthMethod" to locate the spot).
---
Outside diff comments:
In `@webapp/src/routes/_authenticated/w/`$workspaceSlug/achievements.tsx:
- Around line 25-35: The redirect guard currently requires username to be truthy
which can block the silent redirect while username is briefly undefined; update
the condition in the useEffect that checks isLoading/achievementsEnabled to use
a fallback to userProfile?.username (i.e. require workspaceSlug && (username ||
userProfile?.username)), and when calling navigate set the params.username value
to the same fallback (username || userProfile?.username) so the redirect occurs
even if AuthContext.username is temporarily undefined; update references to
isLoading, achievementsEnabled, workspaceSlug, username, userProfile, and
navigate accordingly.
---
Nitpick comments:
In `@webapp/src/components/core/sidebar/NavAdmin.tsx`:
- Around line 11-17: Extract the inline props type for NavAdmin into a new
exported interface (e.g., export interface NavAdminProps { workspaceSlug:
string; achievementsEnabled: boolean; }) and update the function signature to
accept NavAdminProps (export function NavAdmin(props: NavAdminProps) or export
function NavAdmin({ workspaceSlug, achievementsEnabled }: NavAdminProps)).
Ensure the interface is exported so other files can import it and update any
local references/usages accordingly.
In `@webapp/src/components/leaderboard/LeaderboardTable.tsx`:
- Around line 178-221: LeaderboardTableSkeleton currently always renders the
"League" column; change it to accept props (e.g., leaguesEnabled: boolean and
isTeam: boolean) and conditionally render the League TableHead and the
corresponding TableCell only when leaguesEnabled is true and isTeam is false so
the skeleton matches the real table layout; update any call sites of
LeaderboardTableSkeleton to pass the appropriate leaguesEnabled and isTeam
values from the parent component (the same props used to render the real
Leaderboard table).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46fe664c-35b0-4a1b-8e70-0949f3e6c388
📒 Files selected for processing (33)
docker/compose.core.yamlserver/application-server/keycloak-hephaestus-realm-example-config.jsonserver/application-server/openapi.yamlserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceFeatures.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceSettingsService.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/dto/UpdateWorkspaceFeaturesRequestDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/dto/WorkspaceDTO.javaserver/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/dto/WorkspaceListItemDTO.javaserver/application-server/src/main/resources/db/changelog/1775000000000_changelog.xmlserver/application-server/src/main/resources/db/master.xmlwebapp/src/api/types.gen.tswebapp/src/components/admin/AdminFeaturesSettings.stories.tsxwebapp/src/components/admin/AdminFeaturesSettings.tsxwebapp/src/components/admin/AdminSettingsPage.stories.tsxwebapp/src/components/admin/AdminSettingsPage.tsxwebapp/src/components/core/sidebar/AppSidebar.stories.tsxwebapp/src/components/core/sidebar/AppSidebar.tsxwebapp/src/components/core/sidebar/NavAdmin.stories.tsxwebapp/src/components/core/sidebar/NavAdmin.tsxwebapp/src/components/core/sidebar/WorkspaceSwitcher.stories.tsxwebapp/src/components/leaderboard/LeaderboardFilter.tsxwebapp/src/components/leaderboard/LeaderboardOverview.tsxwebapp/src/components/leaderboard/LeaderboardPage.tsxwebapp/src/components/leaderboard/LeaderboardTable.tsxwebapp/src/components/leaderboard/SortFilter.tsxwebapp/src/components/profile/ProfileHeader.tsxwebapp/src/components/profile/ProfilePage.tsxwebapp/src/hooks/use-workspace-features.tswebapp/src/routes/_authenticated/w/$workspaceSlug/achievements.tsxwebapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsxwebapp/src/routes/_authenticated/w/$workspaceSlug/index.tsxwebapp/src/routes/_authenticated/w/$workspaceSlug/user/$username/achievements.tsxwebapp/src/routes/_authenticated/w/$workspaceSlug/user/$username/index.tsx
✅ Files skipped from review due to trivial changes (7)
- webapp/src/routes/_authenticated/w/$workspaceSlug/admin/_admin/settings.tsx
- server/application-server/src/main/resources/db/changelog/1775000000000_changelog.xml
- webapp/src/components/core/sidebar/WorkspaceSwitcher.stories.tsx
- webapp/src/components/core/sidebar/NavAdmin.stories.tsx
- webapp/src/components/admin/AdminSettingsPage.stories.tsx
- server/application-server/src/main/resources/db/master.xml
- webapp/src/api/types.gen.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- webapp/src/components/core/sidebar/AppSidebar.stories.tsx
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/dto/UpdateWorkspaceFeaturesRequestDTO.java
- webapp/src/components/core/sidebar/AppSidebar.tsx
- webapp/src/components/admin/AdminFeaturesSettings.stories.tsx
- webapp/src/components/profile/ProfilePage.tsx
- server/application-server/src/main/java/de/tum/in/www1/hephaestus/workspace/WorkspaceFeatures.java
- webapp/src/components/leaderboard/LeaderboardFilter.tsx
- webapp/src/components/leaderboard/LeaderboardOverview.tsx
- webapp/src/components/admin/AdminSettingsPage.tsx
- webapp/src/hooks/use-workspace-features.ts
- webapp/src/components/leaderboard/SortFilter.tsx
- webapp/src/components/profile/ProfileHeader.tsx
- webapp/src/components/leaderboard/LeaderboardPage.tsx
- webapp/src/routes/_authenticated/w/$workspaceSlug/user/$username/index.tsx
Comment on lines
+1559
to
+1560
| "clientAuthMethod": "client_secret_post", | ||
| "allowedClockSkew": "21700" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Keycloak 26 OIDC identity providers, what does allowedClockSkew control, what unit is it in, and what value range is recommended for production deployments?
💡 Result:
- What it controls:
allowedClockSkewis the clock-skew tolerance Keycloak applies when validating time-based JWT claims in tokens coming from the OIDC identity provider (for example, to avoid rejecting otherwise-valid tokens when the IdP and Keycloak clocks differ slightly). (keycloak.org) - Unit: seconds (integer). Default is 0. (keycloak.org)
- Recommended production range: keep it as low as possible—ideally 0 with proper NTP/clock sync; if you must allow skew, use a small value (commonly ~30–60 seconds) and avoid large values (OIDC guidance is “usually no more than a few minutes”). (keycloak.org)
Citations:
- 1: https://www.keycloak.org/securing-apps/jwt-authorization-grant?utm_source=openai
- 2: https://www.keycloak.org/securing-apps/jwt-authorization-grant?utm_source=openai
- 3: https://www.keycloak.org/securing-apps/jwt-authorization-grant?utm_source=openai
🏁 Script executed:
# Search for all allowedClockSkew occurrences in the config file
rg "allowedClockSkew" server/application-server/keycloak-hephaestus-realm-example-config.json -B 2 -A 1Repository: ls1intum/Hephaestus
Length of output: 211
🏁 Script executed:
# Check if 21700 appears elsewhere (possible pattern/unit confusion)
rg "21700" server/application-server/keycloak-hephaestus-realm-example-config.json -B 2 -A 2Repository: ls1intum/Hephaestus
Length of output: 221
🏁 Script executed:
# Get context around lines 1559-1560 to see the OIDC provider configuration
sed -n '1540,1580p' server/application-server/keycloak-hephaestus-realm-example-config.jsonRepository: ls1intum/Hephaestus
Length of output: 1782
allowedClockSkew is set excessively high for OIDC token validation.
Line 1560 uses 21700 seconds (~6 hours), which far exceeds recommended production values (30–60 seconds or "a few minutes" per OIDC guidance). This weakens JWT expiry and not-before claim validation. Since other GitLab settings use environment variables (e.g., KEYCLOAK_GITLAB_CLIENT_SECRET), move this to a configurable env var with a safe default.
Suggested fix
- "allowedClockSkew": "21700"
+ "allowedClockSkew": "${KEYCLOAK_GITLAB_ALLOWED_CLOCK_SKEW:-300}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/application-server/keycloak-hephaestus-realm-example-config.json`
around lines 1559 - 1560, The allowedClockSkew JSON field is set to an unsafe
high value ("21700"); change the "allowedClockSkew" entry to be configurable via
an environment variable (e.g., KEYCLOAK_ALLOWED_CLOCK_SKEW) with a safe default
of 60 (seconds) and ensure it's stored as an integer/number, not an excessively
large literal string; update any template or loader code that emits
keycloak-hephaestus-realm-example-config.json to read
process.env.KEYCLOAK_ALLOWED_CLOCK_SKEW || 60 and use that numeric value for the
"allowedClockSkew" property (referencing the "allowedClockSkew" key and the
surrounding client config such as "clientAuthMethod" to locate the spot).
…nership Rename ensureAuthenticatedGitLabUser → ensureAuthenticatedUserExists. Now runs for ALL workspace creation (not just GitLab PAT), reads both gitlab_id and github_id from JWT, and creates the appropriate provider User entity. This handles: - GitLab user on fresh DB creating GitLab workspace - GitHub user on fresh DB creating GitLab workspace - Any provider creating any workspace type Fast path: skips if user already exists by login in any provider.
Instead of silently creating wrong-provider users as workspace owners, properly gate GitLab workspace creation on having a linked GitLab identity: Backend: - ensureAuthenticatedUserExists returns 409 Conflict with clear message when user has no gitlab_id in JWT - Only creates GitLab User entity when gitlab_id is present Frontend: - GitLab wizard shows "Link Your GitLab Account" page when no GitLab identity is detected in the Keycloak token - Offers "Go to Settings" and "Link GitLab Now" buttons - GitLab IdP alias resolved dynamically from identity providers API
userProfile.gitlabId was always undefined because loadUserProfile() returns standard Keycloak attributes, not custom token claims. The gitlab_id is in tokenParsed (set by the protocol mapper). Add keycloakService.hasGitLabIdentity() that checks tokenParsed directly, expose it via AuthContext.hasGitLabIdentity.
Replace the "Handle Existing Account" sub-flow (which shows Keycloak UI for manual confirmation + email verification) with idp-auto-link which silently merges accounts when the email matches. This means: if a user logs in via GitHub first, then later via GitLab with the same email, the GitLab identity is automatically linked to their existing Keycloak account. No Keycloak page shown, no duplicate accounts created. Safe because both GitHub and GitLab have trustEmail=true (they verify emails), so email-based matching is reliable.
…counts
When users log in with different IdPs that have different emails,
Keycloak creates separate accounts. The standard "Link" flow then
fails with "identity already linked to another user".
New POST /user/linked-accounts/{providerAlias}/claim endpoint:
- Finds the other Keycloak user who has the requested IdP identity
- Transfers the federated identity to the current user
- Deletes the orphan account if it has no remaining identities
Frontend GitLab wizard now shows:
- "Link GitLab Account" button (standard flow, works for fresh accounts)
- "Merge GitLab Identity" option (shown on expand, handles the duplicate
account scenario by calling the claim endpoint)
GitLab LRZ server has significant clock skew (~6 hours behind UTC). Without clock tolerance, Keycloak rejects the ID token with "Token is no longer valid" because the token appears expired from Keycloak's perspective. Local dev: 21700s (covers the LRZ clock offset) Production: 300s (standard 5-minute tolerance)
…kspaces When a user logs in via GitHub and creates a GitLab PAT workspace, the ensureAuthenticatedGitLabUser method now falls back to creating a GitHub User entity from the JWT's github_id claim. Previously it would skip user creation entirely, causing workspace creation to fail with "Cannot create workspace without an owner" on a fresh database. Also normalize allowedClockSkew to 300s for both local and production.
- Add missing leaguesEnabled argument to UpdateWorkspaceFeaturesRequestDTO constructor calls in WorkspaceControllerIntegrationTest - Regenerate ERD documentation for new hidden/leagues_enabled columns - Regenerate intelligence-service DB models
github-actions
bot
added
documentation
Contributor
📚 Documentation Preview
|
Merge the two separate changelogs (1774900000000 for hidden column, 1775000000000 for leagues_enabled) into a single changelog with a precise timestamp (1775507671119).
Remove auto-generated duplicate constraint names (uknlcwyn..., uk6cjmvj...) that exist in local PostgreSQL but not in CI's fresh H2 database. These are duplicates of the named constraints (uq_discussion_repo_number, uq_discussion_category_repo_slug).
Change /repositories/{owner}/{name} to /repositories/{*nameWithOwner}
to support GitLab paths with multiple segments (e.g. group/subgroup/repo).
The old two-segment pattern broke for any nested group structure.
GitLab repos may not be synced yet when manually adding them to monitor. Skip the DB existence check for GITLAB_PAT workspaces — the monitor entry is created with nameWithOwner and the sync will populate the repository entity later.
…b paths Path variables with encoded slashes (%2F) fail CORS preflight in Spring. Switch add/remove repository endpoints from path variables to query parameter: POST/DELETE /repositories?nameWithOwner=group/subgroup/repo
- Remove API key exposure from Docker container env vars; Azure OpenAI
keys now injected via shell export instead of container env map
- Add review cooldown (configurable, default 15min) to prevent event
spam from rapid MR updates or duplicate bot commands
- React to bot command comments with eyes emoji via GitLab GraphQL
awardEmojiAdd mutation
- Agent now produces delivery.mrNote — natural language MR summary
composed by the LLM, replacing server-side string templates
- Add AI disclaimer footer ("AI-generated feedback can be inaccurate")
with thumbs-up/down reaction guidance
- Escape LIKE wildcards in cooldown query prefix to prevent SQL
pattern injection
- Validate cooldownMinutes >= 0 with @min constraint
- Externalize all PracticeReviewProperties in application.yml with
env var overrides (deliver-to-merged, app-base-url, cooldown-minutes)
- Update orchestrator-protocol.md and CLAUDE.md to require
delivery.mrNote from all agents; add mrNote to Claude Code JSON schema
- Detect MR head commit changes for future PullRequestSynchronized
support in GitLab
- Disable review cooldown in test profile (cooldown-minutes: 0) to prevent AgentJobSubmissionIntegrationTest.allowsDifferentCommitSha from being blocked by the cooldown check - Update WorkspaceControllerIntegrationTest to use query param style (?nameWithOwner=) instead of path variables for add/remove repository endpoints, matching the controller's @RequestParam signature
Collaborator
Author
|
🎉 This PR is included in version 0.56.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
Implements auto-discovery and monitoring of all repositories in GitLab PAT workspaces. When a GitLab workspace is created (via PAT + group selection), all projects within the selected group (including subgroups) are now automatically enumerated and added to monitoring — matching the existing GitHub App installation behavior via
InstallationRepositoryEnumerationService.Also includes: hide-member feature, feature flag polish (leagues split), avatar/profile link fixes, team sync Phase F (students as team members), and identity provider account linking/merging.
Fixes #961
Production Deployment Checklist
1. Environment Variables (
.env)GitLab Identity Provider (Keycloak login)
Create the OAuth Application on GitLab:
https://gitlab.lrz.de/admin/applications(admin) orhttps://gitlab.lrz.de/-/user_settings/applications(personal)https://<hostname>/keycloak/realms/hephaestus/broker/gitlab-lrz/endpointopenid,profile,emailGitLab Sync & Webhooks
The webhook URL must be publicly reachable from GitLab. The app-server registers a group-level webhook on workspace creation that sends:
GitHub App URL (workspace creation UI)
AI Model (for practice review)
NATS (real-time webhook processing)
2. Application Configuration
Set
practice-review-for-all: trueto enable practice review for all contributors without requiring a Keycloak role:Or via environment variables:
3. Keycloak Admin Console (Manual Steps)
After deploying, configure in the Keycloak Admin Console at
https://<hostname>/keycloak/admin/:3a. Disable Profile Review on First Login
This prevents Keycloak from showing "Update Account Information" when users first log in via a new IdP.
3b. Enable Auto-Link for Same-Email Accounts
This auto-merges accounts when a user logs in with a second IdP that has the same email. For different emails, users use the "Merge GitLab Identity" button in the workspace wizard.
3c. Relax Name Validation (for GitLab names with parentheses)
person-name-prohibited-charactersGitLab LRZ returns names with parentheses (e.g., "Dietrich, Felix (Timotheus Johannes)") that fail Keycloak's default validator.
3d. Set Clock Skew Tolerance
300(seconds)4. Database Migrations (Automatic)
Run automatically on startup via Liquibase:
1774900000000: Addshiddencolumn toworkspace_membership1775000000000: Addsleagues_enabledcolumn toworkspace5. Keycloak Version
Upgraded from 26.0 → 26.5 (fixes
kc_action=idp_linkNPE). Docker images pull automatically.6. Per-Workspace Feature Flags (Admin UI)
After deployment, configure in Admin → Settings → Features per workspace:
7. GitLab Access Token for Workspace
When creating a GitLab workspace via the UI:
api8. End-to-End Flow