Feature: Enable users add comments on data charts#476
Feature: Enable users add comments on data charts#476khalifan-kfan wants to merge 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @khalifan-kfan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant new feature that allows users to interact with data charts by adding comments. This functionality facilitates a more collaborative environment for data analysis and review, enabling users to highlight specific observations, inconsistencies, or points of interest directly on the charts. The changes encompass both frontend UI enhancements and backend data persistence, ensuring a seamless experience for managing chart-related feedback.
Highlights
- New Commenting Feature: Users can now add, view, edit, resolve, and delete comments directly on data charts, enhancing collaboration and review processes.
- Dedicated Comment Dialog: A new
ClimateCommentDialogComponenthas been introduced to handle the creation and modification of comments with form validation. - Chart Integration: The
ChartSummaryComponenthas been updated to display a 'Mark for Review' button, show a badge for unresolved comments, and feature a collapsible sidebar to list comments pertinent to the active chart. - Backend Service and Database Update: A new
ClimateCommentServicemanages comment persistence via Supabase, and theclimate_station_datatable now includes acommentsJSONB array column to store these user-generated comments.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
View your CI Pipeline Execution ↗ for commit 38faf13
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for adding comments to data charts. The implementation is comprehensive, covering creation, editing, deletion, and resolution of comments. However, there are several areas for improvement. The most critical issue is a potential race condition in ClimateCommentService due to its read-modify-write approach on a JSONB column, which could lead to data loss under concurrent use. Additionally, there are opportunities to enhance performance and maintainability in the Angular components by using computed signals instead of template method calls, and by updating local state more efficiently instead of re-fetching data. Lastly, improving user-facing error feedback would enhance the user experience.
| async submitComment(stationId: string, comment: ClimateComment): Promise<void> { | ||
| // Get existing comments | ||
| const existingComments = await this.getComments(stationId); | ||
| const updatedComments = [...existingComments, comment]; | ||
|
|
||
| // Update the station data with new comment | ||
| const { error } = await this.stationDataDB.update({ comments: updatedComments as any }).eq('station_id', stationId); | ||
|
|
||
| if (error) { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
The methods submitComment, updateComment, and deleteComment all use a read-modify-write pattern to update the comments JSONB array. This is susceptible to race conditions. If two users modify comments for the same station at the same time, one user's changes could be overwritten and lost. To prevent this, you should perform atomic updates on the server-side, for example, by using a Supabase Edge Function that leverages PostgreSQL's JSONB manipulation functions (jsonb_set, jsonb_insert, etc.) to append, update, or remove elements from the array atomically. This is a critical issue that can lead to data loss.
...pp/modules/climate/pages/station-details/components/chart-summary/chart-summary.component.ts
Outdated
Show resolved
Hide resolved
| @if (isSubmitting) { | ||
| <mat-spinner diameter="20"></mat-spinner> | ||
| } | ||
| @if (!isSubmitting) { | ||
| <span>{{data.isEditMode ? 'Update Comment' : 'Submit Review'}}</span> | ||
| } | ||
| @if (isSubmitting) { | ||
| <span>Submitting...</span> | ||
| } |
There was a problem hiding this comment.
The logic for displaying the submit button's content can be simplified. You are using three separate @if blocks, two of which have the same condition isSubmitting. This can be refactored into a single @if/@else block for better readability and maintainability.
| @if (isSubmitting) { | |
| <mat-spinner diameter="20"></mat-spinner> | |
| } | |
| @if (!isSubmitting) { | |
| <span>{{data.isEditMode ? 'Update Comment' : 'Submit Review'}}</span> | |
| } | |
| @if (isSubmitting) { | |
| <span>Submitting...</span> | |
| } | |
| @if (isSubmitting) { | |
| <mat-spinner diameter="20"></mat-spinner> | |
| <span>Submitting...</span> | |
| } @else { | |
| <span>{{data.isEditMode ? 'Update Comment' : 'Submit Review'}}</span> | |
| } |
| export interface CommentDialogData { | ||
| stationId: string; | ||
| stationName: string; | ||
| chartName: string; | ||
| editComment?: ClimateComment; | ||
| isEditMode?: boolean; | ||
| } | ||
|
|
||
| export interface ClimateComment { | ||
| id: string; | ||
| station_id: string; | ||
| chart_name: string; | ||
| date_created: string; | ||
| created_by: string; | ||
| created_by_name: string; | ||
| comment: string; | ||
| resolved?: boolean; | ||
| } |
There was a problem hiding this comment.
The CommentDialogData and ClimateComment interfaces are defined within this component file but are used across multiple files (chart-summary.component.ts, climate-comment.service.ts). For better code organization and reusability, these shared interfaces should be moved to a dedicated models or types file within the climate module, for example, apps/picsa-apps/dashboard/src/app/modules/climate/types/comment.types.ts.
| } catch (error) { | ||
| console.error('Error submitting comment:', error); |
There was a problem hiding this comment.
In the onSubmit method, errors during comment submission are only logged to the console. This provides no feedback to the user that the operation failed. The dialog will just remain open without explanation. You should implement user-facing error handling, for instance, by displaying a snackbar message or an error message within the dialog to inform the user about the failure.
| constructor( | ||
| private dialog: MatDialog, | ||
| private commentService: ClimateCommentService, | ||
| private supabaseService: SupabaseService, | ||
| ) { |
There was a problem hiding this comment.
The expression station.id || station.station_id is used repeatedly throughout this component to get the station identifier. This suggests an inconsistency in the data model and makes the code harder to maintain. To improve this, you could introduce a computed signal that provides a single, reliable way to access the station ID.
For example, you could add this to your component:
private stationIdentifier = computed(() => {
const station = this.station();
return station.id || station.station_id;
});Then, you can use this.stationIdentifier() wherever you need the station ID, which centralizes the logic and makes the code cleaner.
| dialogRef.afterClosed().subscribe(async (result) => { | ||
| if (result) { | ||
| // Refresh comments after new comment is added | ||
| try { | ||
| const comments = await this.commentService.getComments(station.id || station.station_id); | ||
| this.comments.set(comments); | ||
| } catch (error) { | ||
| console.error('Failed to refresh comments:', error); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
After adding or editing a comment, you are re-fetching the entire list of comments from the server. This is inefficient. The dialog already returns the created or updated comment object. You can use this object to update the local comments signal directly, avoiding an unnecessary network request. This same issue is present in the openCommentModal method's afterClosed subscription (lines 181-191).
For adding a new comment (in showCommentDialog):
// ...
dialogRef.afterClosed().subscribe((result: ClimateComment) => {
if (result) {
this.comments.update(comments => [...comments, result]);
}
});For editing a comment (in openCommentModal):
// ...
dialogRef.afterClosed().subscribe((result: ClimateComment) => {
if (result) {
this.comments.update(comments =>
comments.map(c => c.id === result.id ? result : c)
);
}
});| public async toggleCommentResolved(comment: ClimateComment): Promise<void> { | ||
| const station = this.station(); | ||
| if (!station) return; | ||
|
|
||
| try { | ||
| await this.commentService.toggleCommentResolved(station.id || station.station_id, comment.id); | ||
| // Refresh comments | ||
| const comments = await this.commentService.getComments(station.id || station.station_id); | ||
| this.comments.set(comments); | ||
| } catch (error) { | ||
| console.error('Failed to toggle comment resolved status:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
The toggleCommentResolved and deleteComment methods re-fetch the entire list of comments after performing an action, which is inefficient. It's better to update the local state optimistically or upon a successful API response. This applies to both toggleCommentResolved and deleteComment.
For toggleCommentResolved:
public async toggleCommentResolved(commentToToggle: ClimateComment): Promise<void> {
// ... guard clauses
try {
await this.commentService.toggleCommentResolved(this.stationIdentifier(), commentToToggle.id);
this.comments.update(comments =>
comments.map(c => c.id === commentToToggle.id ? { ...c, resolved: !c.resolved } : c)
);
} // ... catch block
}For deleteComment:
public async deleteComment(commentToDelete: ClimateComment): Promise<void> {
// ... guard clauses
try {
await this.commentService.deleteComment(this.stationIdentifier(), commentToDelete.id);
this.comments.update(comments => comments.filter(c => c.id !== commentToDelete.id));
} // ... catch block
}|
Recommendation (rough notes - use own judgement and AI to create)
|
Description
This feature enables a user to add comments to data charts for review
Discussion
Closes #377
Preview
Screenshots / Videos