Conversation
danoswaltCL
requested review from
bcb37,
Copilot and
zackcl
and removed request for
bcb37
There was a problem hiding this comment.
Pull request overview
This PR adds a new "Logs" tab to the experiment details page, allowing users to view audit logs specific to a particular experiment. The implementation includes both backend and frontend changes to filter audit logs by experiment ID and display them in a timeline format with search and filtering capabilities.
Changes:
- Backend adds optional
experimentIdparameter to the audit logs API endpoint - Frontend implements experiment-specific log storage in NgRx store using a map indexed by experimentId
- New standalone components for displaying experiment logs with timeline view, search, and pagination
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/packages/Upgrade/src/api/controllers/LogController.ts | Added experimentId parameter to audit logs endpoint documentation and implementation |
| backend/packages/Upgrade/src/api/controllers/validators/AuditLogParamsValidators.ts | Added validation for optional experimentId UUID parameter |
| backend/packages/Upgrade/src/api/services/AuditService.ts | Updated service methods to accept and pass experimentId filter |
| backend/packages/Upgrade/src/api/repositories/ExperimentAuditLogRepository.ts | Implemented experimentId filtering in database queries using JSON field extraction |
| frontend/.../logs.actions.ts | Added new actions for experiment-specific log fetching and filtering |
| frontend/.../logs.reducer.ts | Implemented state management for experiment logs stored by experimentId |
| frontend/.../logs.selectors.ts | Added selectors for accessing experiment-specific log state |
| frontend/.../logs.model.ts | Added ExperimentLogsMetadata interface and updated AuditLogParams |
| frontend/.../logs.effects.ts | Implemented effect handlers for fetching experiment-specific logs |
| frontend/.../logs.service.ts | Added service methods for experiment log operations |
| frontend/.../common-section-card-search-header/* | Enhanced search header to support grouped filter options |
| frontend/.../experiment-overview-details-footer/* | Added "Logs" tab to experiment details navigation |
| frontend/.../experiment-log-section-card/* | New component for displaying experiment logs with search and pagination |
| frontend/.../experiment-logs-timeline/* | New timeline component for rendering log entries |
| frontend/.../experiment-log-diff-display/* | New component for displaying log diffs in expandable panels |
| frontend/.../pipes/* | New standalone pipes for formatting log dates and messages |
| frontend/.../experiment-details-page-content.* | Integrated logs tab into experiment details page |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Component({ | ||
| selector: 'app-experiment-log-section-card', | ||
| imports: [ | ||
| CommonModule, | ||
| CommonSectionCardComponent, | ||
| CommonSectionCardActionButtonsComponent, | ||
| CommonSectionCardSearchHeaderComponent, | ||
| TranslateModule, | ||
| SharedModule, | ||
| ExperimentLogsTimelineComponent, | ||
| LogDateFormatPipe, | ||
| MatProgressSpinnerModule, | ||
| ], | ||
| templateUrl: './experiment-log-section-card.component.html', | ||
| styleUrl: './experiment-log-section-card.component.scss', | ||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||
| }) |
There was a problem hiding this comment.
Missing standalone property: The component uses the 'imports' array (line 36) but doesn't explicitly declare 'standalone: true'. While this might work in newer Angular versions with implicit standalone detection, it's best practice to explicitly declare 'standalone: true' for clarity and consistency with other standalone components in the codebase.
...experiment-log-section-card/experiment-logs-timeline/experiment-logs-timeline.component.html
Show resolved
Hide resolved
| imports: [ | ||
| CommonModule, | ||
| RouterModule, | ||
| MatExpansionModule, | ||
| MatTooltipModule, | ||
| TranslateModule, | ||
| ExperimentLogMessagePipe, | ||
| ListOperationMessagePipe, | ||
| ExperimentLogDiffDisplayComponent, | ||
| ], |
There was a problem hiding this comment.
Missing module import: The template uses non-standalone components and pipes (app-shared-icons on lines 6 and 19, truncate pipe on line 40) that are exported from SharedModule, but SharedModule is not imported in this component's imports array. Add SharedModule to the imports array to make these components and pipes available.
| <mat-expansion-panel-header class="expansion-header" expandedHeight="*" collapsedHeight="*"> | ||
| <mat-panel-title> | ||
| <span class="ft-16-600 action-msg"> | ||
| {{ log.type | experimentActionMessage | translate }} |
There was a problem hiding this comment.
Missing pipe import: The template uses the pipe 'experimentActionMessage' on line 133, but this pipe is not imported in the component's imports array. The existing 'experimentActionMessage' pipe in the codebase is not standalone (marked as standalone: false), so it cannot be used in this standalone component. Consider creating a standalone version or using the existing 'ExperimentLogMessagePipe' instead.
| {{ log.type | experimentActionMessage | translate }} | |
| {{ log.type | experimentLogMessage | translate }} |
| - | ||
| {{ | ||
| log.data.list.operation | ||
| | listOperationsMessage |
There was a problem hiding this comment.
Missing pipe import: The template uses the pipe 'listOperationsMessage' on line 143, but this pipe is not imported in the component's imports array. The existing 'listOperationsMessage' pipe in the codebase is not standalone (marked as standalone: false), so it cannot be used in this standalone component. Consider using the existing 'ListOperationMessagePipe' that is already imported in this component instead.
| | listOperationsMessage | |
| | listOperationMessage |
| <pre><div | ||
| class="ft-14-400 diff-container" | ||
| [id]="log.id" | ||
| >{{ getHtmlFormedLogData(log.id, log.data.list.diff) }}</div |
There was a problem hiding this comment.
Side effect in template binding: The template calls 'getHtmlFormedLogData()' inside a template binding on line 162. This method has side effects (DOM manipulation via getElementById and innerHTML). Methods with side effects should not be called from template bindings as they may be called multiple times during change detection. Consider using ngAfterViewInit or a directive to handle this DOM manipulation properly.
| *ngIf="log.data.isExperimentExist; else experimentNotExist" | ||
| [routerLink]="['/home', 'detail', log.data.experimentId]" | ||
| >{{ log.data.experimentName }}</a | ||
| > |
There was a problem hiding this comment.
Undefined template reference: The template references 'experimentNotExist' on line 136, but this ng-template is not defined. This will cause a template error when an experiment doesn't exist.
| > | |
| > | |
| <ng-template #experimentNotExist> | |
| <span>{{ log.data.experimentName }}</span> | |
| </ng-template> |
SO...
The main new backend functionality here is fetching audit logs by adding the optional
experimentIdto the existing /audit endpoint.The main new frontend functionality (other than adding the new tab) is that when the experiment-specific logs are received, we will save these experiment-specific logs + metadata (skip, filter, take, total) in a map on the
logsstore by experimentId:*** FOLLOW-UPS ***
This is an initial component to get something in there, it's intentionally left ugly, we can sculpt it as we go and make some decisions:
NOTE: because we have a mix of standalone and non-standalone components, it was awkward to try and re-use the "pipes" that are used here, so they are basically duplicated as standalones. these too are a little messy and will need the same attention, but they will do the trick for now.