Conversation
Implement Firebase Firestore adapter for data persistence with unit tests. Refactor tests for improved type safety and add comprehensive JSDoc comments to both production and test files. Includes necessary dependency updates.
- Added integration tests for FirebaseFirestoreClient to validate real connections and operations. - Implemented unit tests to mock Firestore interactions and ensure correct functionality. - Updated package dependencies and configurations for improved compatibility.
- Implemented pagination options in queryByName and list methods across various database clients. - Updated data route to handle pagination parameters and return paginated results. - Enhanced documentation for pagination options in relevant types and schemas.
- Implemented comprehensive unit tests for FirebaseFirestoreClient to validate CRUD operations and error handling. - Added tests for InMemoryBlob and InMemoryDataDBClient to ensure correct functionality of blob storage and in-memory database operations. - Created tests for S3CompatibleBlob to verify upload, retrieval, and deletion of blobs. - Developed integration tests for blob and data routes to ensure proper API behavior and authorization checks.
- Changed import paths for lucide-svelte to @lucide/svelte across multiple components. - Reformatted code for better readability and consistency in style. - Updated type definitions and interfaces in character-related files for clarity. - Enhanced the Vite configuration with improved test settings and proxy configurations.
concertypin
added
c:frontend
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| sessionStorage.setItem( | ||
| STORAGE_KEYS.SESSION_MASTER_PASSWORD, | ||
| masterPassword | ||
| masterPassword, |
Check failure
Code scanning / CodeQL
Clear text storage of sensitive information High
|
/gemini review |
Summary of ChangesHello @concertypin, 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 comprehensive CRUD (Create, Read, Update, Delete) functionality for phonebook entries, enhancing the backend with Firebase Firestore support and integrating Biome for improved code quality. It also refines data handling with pagination in storage clients and adds dedicated blob storage routes. The frontend is updated to reflect these new capabilities, including a new UI for managing phonebook entries and general code quality improvements. Highlights
Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality by adding CRUD operations for the phonebook, complete with backend APIs, multiple database adapters, and a corresponding frontend UI. The migration to Biome for code formatting and the addition of Vitest for testing are excellent steps towards improving code quality and maintainability. My review highlights a few key areas for improvement, particularly concerning database performance and scalability in the new adapters, and seeks clarification on some authorization changes. Addressing these points will help ensure the new features are robust and scalable.
I am having trouble creating individual review comments. Click here to see my feedback.
backend/src/adapters/vendor/AzureCosmosDB.ts (143-151)
The logic for constructing the orderBy clause is incorrect and will lead to invalid SQL queries.
- When
orderisDataListOrder.DownloadsFirst,orderBybecomes"downloadCount DESC". The resulting query will beSELECT * FROM c downloadCount DESC ..., which is invalid. It should beORDER BY c.downloadCount DESC. - When
orderisundefined,orderBybecomes"c.id". The resulting query will beSELECT * FROM c c.id ..., which is also invalid. It should beORDER BY c.id.
This needs to be corrected to ensure the list method functions correctly.
let orderByClause: string;
if (order === DataListOrder.NewestFirst) {
orderByClause = "ORDER BY c.uploadedAt DESC";
} else if (order === DataListOrder.DownloadsFirst) {
orderByClause = "ORDER BY c.downloadCount DESC";
} else {
// Default order by id to ensure consistent pagination
orderByClause = "ORDER BY c.id";
}
backend/src/adapters/vendor/FirebaseFirestore.ts (104-138)
The current pagination implementation for queryByName (and list) fetches all matching documents from Firestore and then slices the result in memory. As noted in the code comments, this is not scalable and will lead to significant performance issues and high costs as the number of documents grows.
Firestore is optimized for cursor-based pagination. You should use limit() and startAfter() with a document snapshot to fetch pages efficiently without loading the entire dataset. This is a critical change for production readiness.
backend/src/routes/data.ts (65-71)
This change removes the authorization middleware that restricted the creation of data entries (POST /data) to administrators. Now, any user with a 'known' role can create entries. While the pull request is about enabling CRUD for users, this is a significant change in the security model. Please confirm if this is the intended behavior.
backend/src/adapters/vendor/FirebaseFirestore.ts (62-71)
The normalizeData function uses any and manual type casting (Number()), which is not type-safe. For instance, if data.downloadCount is a non-numeric string, Number(data.downloadCount) could result in NaN, which might not be the intended behavior. A safer approach is to use a Zod schema (like DataSchema) with safeParse to validate and parse the data from Firestore. This ensures type safety and gracefully handles unexpected data structures.
private normalizeData(id: string, data: unknown): DataType | null {
const parsed = DataSchema.safeParse({ ...data, id });
if (!parsed.success) {
console.error(`Invalid data from Firestore for id ${id}:`, parsed.error);
return null;
}
return parsed.data;
}backend/src/adapters/vendor/InMemoryDB.ts (165-169)
The use of as any here to merge the existing item with the update is not type-safe. You can achieve the same result in a type-safe manner by using a non-null assertion (!), which is safe here because you've already checked for the item's existence on line 164.
this.store.set(item.id, { ...this.store.get(item.id)!, ...item });backend/src/routes/data.ts (303)
The OpenAPI specification for this endpoint indicates that only admins (ClerkAdmin) can perform this action. However, the implementation logic on lines 319-322 correctly allows both admins and the author of the entry to delete it. The OpenAPI spec should be updated to reflect the actual, more permissive authorization logic to avoid confusion for API consumers. You could change this to ClerkUser and add a description of the specific authorization rules.
security: [{ ClerkUser: [] }],
There was a problem hiding this comment.
Code Review
This is a substantial pull request that introduces CRUD operations for the phonebook, adds a Firebase Firestore database adapter, and migrates the project to use Biome for formatting and Vitest for testing. The implementation of pagination on the backend is a great improvement, and the addition of comprehensive unit and integration tests is excellent. I've identified a few areas for improvement, primarily concerning the efficiency of the new Firestore adapter's pagination and a minor logging issue.
| // Get all documents (not ideal for large datasets) | ||
| const allQuerySnapshot = await this.db | ||
| .collection(this.collectionName) | ||
| .where("name", "==", name) | ||
| .get(); |
There was a problem hiding this comment.
This pagination implementation fetches all documents from the collection and then performs slicing in memory. As the comment at line 108 correctly notes, this approach is not scalable and will lead to significant performance degradation and increased costs as the number of documents grows. For efficient pagination with Firestore, you should use query cursors with startAfter() and limit(). This typically involves using the ID of the last document from the previous page as the pageToken to fetch the next set of documents directly from the database.
| } | ||
|
|
||
| const query = `SELECT * FROM c ${orderBy} OFFSET @offset LIMIT @limit`; | ||
| console.log("CosmosDB list query:", query); |
There was a problem hiding this comment.
This console.log statement appears to be for debugging. It should be removed to avoid cluttering logs in production environments and potentially leaking query information.
| async queryByName( | ||
| name: string, | ||
| options?: PaginationOptions, | ||
| ): Promise<PaginationResult<DataType>> { |
There was a problem hiding this comment.
The queryByName implementation here performs an exact match on the name field. This is inconsistent with the AzureCosmosDB adapter, which uses a case-insensitive CONTAINS for a fuzzy search. This difference in behavior could lead to unexpected results depending on the deployed database. The search behavior should be standardized across all BaseDataDBClient implementations. If fuzzy search is the intended behavior, you might need to consider a third-party search service like Algolia for Firestore, as it lacks native support for this. If an exact match is sufficient, the comment in the CosmosDB adapter should be updated to reflect this for consistency.
✅ Deploy Preview for arisutalk-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
concertypin
force-pushed
the
dev/phonebook-crud
branch
2 times, most recently
from
b056424 to
208f1e4
Compare
|
Blocked by Project Kei. #84 |
2 participants
Pull Request Template
Description
It implements CRUD for phonebook to users.
Checklist
If these questions do not apply to your pull request, just check them.
Additional Notes
Add any other context or screenshots here.