Feature: Generic data model and Supabase data service #124
Conversation
A generic data template model has been implemented. This model can be extended to other classes, such as User. It includes the TABLE_NAME and DATABASE_MAPPING properties for a more efficient database access. The SupabaseService has been implemented to support general data fetching. It currently supports the fetching of a single object and an array of objects. There is no need to implement the data fetching logic repeatedly for each model. - Added SupabaseService - Added GenericModel - Added User model - Added data fetching tests during Homepage initialization
WalkthroughThe changes introduce a new abstract class Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SupabaseService
participant Database
User->>SupabaseService: fetchSingle(User, targetID)
SupabaseService->>Database: Query for targetID
Database-->>SupabaseService: Return record
SupabaseService->>User: parseJson(record)
User-->>SupabaseService: Return User instance
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (7)
src/models/GenericModel.ts (2)
1-2: LGTM! Consider documenting the contract.The abstract class declaration is well-structured for its purpose as a base model. However, since this is a foundational piece, consider adding JSDoc comments to document:
- The purpose and responsibility of this class
- Expected usage patterns for extending classes
- Requirements for implementing the abstract members
Example documentation:
/** * Abstract base class for database models providing a consistent interface * for table operations and JSON parsing. * * @abstract * @example * class User extends GenericModel { * static TABLE_NAME = 'users'; * static DATABASE_MAP = { ... }; * * static parseJson(json: unknown): User { * // Implementation * } * } */
1-12: Consider enhancing the base model with common database operations.Since this is a foundational piece for database models, consider adding:
- Common CRUD operation methods
- Validation utilities
- Type-safe query building helpers
- Error handling utilities
This would provide a more complete and consistent interface for all model implementations.
Example additions:
abstract class GenericModel { // ... existing code ... static async findById(id: string): Promise<ThisType> { // Implementation } static async findMany(criteria: QueryCriteria): Promise<ThisType[]> { // Implementation } abstract validate(): ValidationResult; protected static buildQuery(): QueryBuilder { // Implementation } }🧰 Tools
🪛 Biome (1.9.4)
[error] 2-12: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 6-6: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
🪛 eslint (1.23.1)
[error] 6-6: Don't use
Objectas a type. TheObjecttype actually means "any non-nullish value", so it is marginally better thanunknown.
- If you want a type meaning "any object", you probably want
objectinstead.- If you want a type meaning "any value", you probably want
unknowninstead.- If you really want a type meaning "any non-nullish value", you probably want
NonNullable<unknown>instead.(@typescript-eslint/ban-types)
[error] 9-9: 'json' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 9-9: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/services/SupabaseService.ts (3)
39-47: Enhance error handling and response typingThe error handling could be more robust:
- const data = await query; + const { data, error } = await query; + + if (error) { + throw new Error(`Failed to fetch ${modelClass.name}: ${error.message}`); + } if (!data.data || data.data.length == 0) { return null } const record = data.data[0] - return model.parseJson(record) + return modelClass.prototype.constructor.parseJson(record)
1-84: Add unit tests for the serviceThe service lacks unit tests which are crucial for maintaining reliability.
Would you like me to help create a comprehensive test suite for this service? The tests would cover:
- Single record fetching with various criteria
- Multiple record fetching with pagination and filters
- Error handling scenarios
- Edge cases
🧰 Tools
🪛 Biome (1.9.4)
[error] 5-84: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 25-25: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
🪛 eslint (1.23.1)
[error] 25-25: Don't use
Stringas a type. Use string instead(@typescript-eslint/ban-types)
[error] 26-26: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 28-28: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 31-33: Unexpected var, use let or const instead.
(no-var)
[error] 67-67: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
1-84: Consider implementing a proper error handling strategyThe current implementation would benefit from a more robust error handling strategy:
- Define custom error types for different failure scenarios
- Implement proper error propagation
- Add logging for debugging purposes
// Example error types export class SupabaseServiceError extends Error { constructor(message: string, public cause?: Error) { super(message); this.name = 'SupabaseServiceError'; } } export class RecordNotFoundError extends SupabaseServiceError { constructor(modelName: string, id: string) { super(`${modelName} with id ${id} not found`); this.name = 'RecordNotFoundError'; } }🧰 Tools
🪛 Biome (1.9.4)
[error] 5-84: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 25-25: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
🪛 eslint (1.23.1)
[error] 25-25: Don't use
Stringas a type. Use string instead(@typescript-eslint/ban-types)
[error] 26-26: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 28-28: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 31-33: Unexpected var, use let or const instead.
(no-var)
[error] 67-67: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/routes/home/index.tsx (2)
74-87: Clean up or implement commented UI components.There are multiple commented-out UI components that appear to be part of the intended implementation. Either remove them if they're not needed or implement them properly.
41-89: Add proper TypeScript types and accessibility attributes.The component could benefit from:
- Type definitions for the route parameters and component props
- ARIA labels and roles for better accessibility
- Semantic HTML structure
Consider wrapping the profile section in a more semantic structure:
- <section className='h-[200px] mb-[40px] bg-yellow-50 relative border-b-[5px] border-gray-600'> + <section + aria-label="User Profile" + className='h-[200px] mb-[40px] bg-yellow-50 relative border-b-[5px] border-gray-600' + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/models/GenericModel.ts(1 hunks)src/models/User.ts(1 hunks)src/routes/home/index.tsx(1 hunks)src/services/SupabaseService.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/models/GenericModel.ts
[error] 2-12: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 6-6: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
src/models/User.ts
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 6-6: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
[error] 14-14: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 15-15: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 16-16: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 17-17: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 18-18: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 19-19: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 21-21: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 22-22: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 23-23: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 24-24: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
src/services/SupabaseService.ts
[error] 5-84: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 25-25: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
🪛 eslint (1.23.1)
src/models/GenericModel.ts
[error] 6-6: Don't use Object as a type. The Object type actually means "any non-nullish value", so it is marginally better than unknown.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead. - If you really want a type meaning "any non-nullish value", you probably want
NonNullable<unknown>instead.
(@typescript-eslint/ban-types)
[error] 9-9: 'json' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 9-9: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/models/User.ts
[error] 6-6: Don't use Object as a type. The Object type actually means "any non-nullish value", so it is marginally better than unknown.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead. - If you really want a type meaning "any non-nullish value", you probably want
NonNullable<unknown>instead.
(@typescript-eslint/ban-types)
[error] 14-14: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 15-15: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 16-16: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 17-17: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 18-18: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 19-19: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 21-21: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 22-22: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 23-23: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 24-24: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 27-27: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 32-32: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
src/routes/home/index.tsx
[error] 4-4: 'homeItems' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
src/services/SupabaseService.ts
[error] 25-25: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 26-26: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 28-28: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 31-33: Unexpected var, use let or const instead.
(no-var)
[error] 67-67: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🔇 Additional comments (1)
src/models/User.ts (1)
1-3: LGTM! Clean class structure and inheritance.
The class properly extends GenericModel and follows good inheritance practices.
| // Abstract method for parsing JSON | ||
| static parseJson(json: any): GenericModel { | ||
| throw new Error("parseJson method not implemented."); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling of parseJson method.
Several improvements can be made to enhance type safety and developer experience:
- Use
unknowninstead ofanyfor better type safety - Make the return type match the implementing class using generics
- Improve the error message to be more descriptive
- static parseJson(json: any): GenericModel {
- throw new Error("parseJson method not implemented.");
+ abstract static parseJson(json: unknown): ThisType {
+ throw new Error(
+ `${this.name}.parseJson() must be implemented to parse database records.`
+ );
+ }Consider also adding runtime type checking:
protected static validateJson(json: unknown): void {
if (!json || typeof json !== 'object') {
throw new Error(`Invalid JSON data provided to ${this.name}.parseJson()`);
}
}🧰 Tools
🪛 eslint (1.23.1)
[error] 9-9: 'json' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 9-9: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
| // Abstract property for table name | ||
| static TABLE_NAME: string; | ||
|
|
||
| static DATABASE_MAP: Object; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety of DATABASE_MAP.
The Object type is too broad and provides little type safety. Consider:
- Defining a specific interface for the mapping structure
- Using a generic type parameter for flexibility
- static DATABASE_MAP: Object;
+ /** Mapping of model properties to database columns */
+ static DATABASE_MAP: Record<string, string>;
+ // Or for more flexibility:
+ static DATABASE_MAP: {
+ [K in keyof ThisType]: string;
+ };Also, consider making these properties abstract to enforce implementation:
- static TABLE_NAME: string;
+ abstract static TABLE_NAME: string;
- static DATABASE_MAP: Object;
+ abstract static DATABASE_MAP: Record<string, string>;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
🪛 eslint (1.23.1)
[error] 6-6: Don't use Object as a type. The Object type actually means "any non-nullish value", so it is marginally better than unknown.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead. - If you really want a type meaning "any non-nullish value", you probably want
NonNullable<unknown>instead.
(@typescript-eslint/ban-types)
| static TABLE_NAME = "members" | ||
| static DATABASE_MAP: Object = { | ||
| id: "uuid", | ||
| username: "username", | ||
| email: "fk_email", | ||
| identity: "fk_identity", | ||
| avatar: "avatar", | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and documentation of database mappings.
- Replace the broad
Objecttype with a specific interface:
- static DATABASE_MAP: Object = {
+ static DATABASE_MAP: Record<keyof User, string> = {
id: "uuid",
username: "username",
email: "fk_email",
identity: "fk_identity",
avatar: "avatar",
}- Document foreign key relationships:
+ /** Database field mappings
+ * @property fk_email - Foreign key reference to user's email
+ * @property fk_identity - Foreign key reference to user's identity
+ */
static DATABASE_MAP: Record<keyof User, string> = {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
🪛 eslint (1.23.1)
[error] 6-6: Don't use Object as a type. The Object type actually means "any non-nullish value", so it is marginally better than unknown.
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead. - If you really want a type meaning "any non-nullish value", you probably want
NonNullable<unknown>instead.
(@typescript-eslint/ban-types)
| public id: String = "" | ||
| public username: String = "" | ||
| public email: String = "" | ||
| public phone: String = "" | ||
| public avatar: String = "" | ||
| public profileBackground: String = "" | ||
| public joinedAt: Date = new Date() | ||
| public identity: String = "" | ||
| public department: String = "" | ||
| public grade: String = "" | ||
| public bio: String = "" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type definitions and add property documentation.
- Use correct TypeScript primitive types:
- public id: String = ""
- public username: String = ""
- public email: String = ""
- public phone: String = ""
- public avatar: String = ""
- public profileBackground: String = ""
- public joinedAt: Date = new Date()
- public identity: String = ""
- public department: String = ""
- public grade: String = ""
- public bio: String = ""
+ /** Unique identifier for the user */
+ public id: string = ""
+ /** User's display name */
+ public username: string = ""
+ /** User's email address */
+ public email: string = ""
+ /** User's phone number */
+ public phone: string = ""
+ /** URL to user's avatar image */
+ public avatar: string = ""
+ /** URL to user's profile background image */
+ public profileBackground: string = ""
+ /** Timestamp when user joined */
+ public joinedAt: Date = new Date()
+ /** User's role or identity type */
+ public identity: string = ""
+ /** User's academic department */
+ public department: string = ""
+ /** User's academic grade/year */
+ public grade: string = ""
+ /** User's biography or description */
+ public bio: string = ""📝 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.
| public id: String = "" | |
| public username: String = "" | |
| public email: String = "" | |
| public phone: String = "" | |
| public avatar: String = "" | |
| public profileBackground: String = "" | |
| public joinedAt: Date = new Date() | |
| public identity: String = "" | |
| public department: String = "" | |
| public grade: String = "" | |
| public bio: String = "" | |
| /** Unique identifier for the user */ | |
| public id: string = "" | |
| /** User's display name */ | |
| public username: string = "" | |
| /** User's email address */ | |
| public email: string = "" | |
| /** User's phone number */ | |
| public phone: string = "" | |
| /** URL to user's avatar image */ | |
| public avatar: string = "" | |
| /** URL to user's profile background image */ | |
| public profileBackground: string = "" | |
| /** Timestamp when user joined */ | |
| public joinedAt: Date = new Date() | |
| /** User's role or identity type */ | |
| public identity: string = "" | |
| /** User's academic department */ | |
| public department: string = "" | |
| /** User's academic grade/year */ | |
| public grade: string = "" | |
| /** User's biography or description */ | |
| public bio: string = "" |
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 15-15: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 16-16: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 17-17: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 18-18: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 19-19: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 21-21: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 22-22: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 23-23: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
[error] 24-24: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
🪛 eslint (1.23.1)
[error] 14-14: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 15-15: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 16-16: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 17-17: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 18-18: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 19-19: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 21-21: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 22-22: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 23-23: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
[error] 24-24: Don't use String as a type. Use string instead
(@typescript-eslint/ban-types)
| static parseJson(json: any) : User { | ||
| const user = new User() | ||
|
|
||
| for (const [userProperty, jsonField] of Object.entries(this.DATABASE_MAP)) | ||
| if (json[jsonField] !== undefined) | ||
| (user as any)[userProperty] = json[jsonField]; | ||
|
|
||
| return user | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling in parseJson method.
The current implementation has several issues that should be addressed:
- static parseJson(json: any) : User {
- const user = new User()
-
- for (const [userProperty, jsonField] of Object.entries(this.DATABASE_MAP))
- if (json[jsonField] !== undefined)
- (user as any)[userProperty] = json[jsonField];
-
- return user
- }
+ static parseJson(json: Record<string, unknown>): User {
+ const user = new User();
+
+ try {
+ for (const [userProperty, jsonField] of Object.entries(User.DATABASE_MAP)) {
+ if (json[jsonField] !== undefined) {
+ // Type-safe assignment with validation
+ const value = json[jsonField];
+ switch(userProperty) {
+ case 'joinedAt':
+ user[userProperty] = new Date(value as string);
+ break;
+ default:
+ if (typeof value === 'string') {
+ user[userProperty as keyof User] = value;
+ }
+ }
+ }
+ }
+ } catch (error) {
+ throw new Error(`Failed to parse user data: ${error.message}`);
+ }
+
+ return user;
+ }Changes made:
- Added proper type safety with
Record<string, unknown> - Replaced
thiswith explicit class name - Added error handling
- Added type validation for properties
- Special handling for Date type conversion
📝 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.
| static parseJson(json: any) : User { | |
| const user = new User() | |
| for (const [userProperty, jsonField] of Object.entries(this.DATABASE_MAP)) | |
| if (json[jsonField] !== undefined) | |
| (user as any)[userProperty] = json[jsonField]; | |
| return user | |
| } | |
| static parseJson(json: Record<string, unknown>): User { | |
| const user = new User(); | |
| try { | |
| for (const [userProperty, jsonField] of Object.entries(User.DATABASE_MAP)) { | |
| if (json[jsonField] !== undefined) { | |
| // Type-safe assignment with validation | |
| const value = json[jsonField]; | |
| switch(userProperty) { | |
| case 'joinedAt': | |
| user[userProperty] = new Date(value as string); | |
| break; | |
| default: | |
| if (typeof value === 'string') { | |
| user[userProperty as keyof User] = value; | |
| } | |
| } | |
| } | |
| } | |
| } catch (error) { | |
| throw new Error(`Failed to parse user data: ${error.message}`); | |
| } | |
| return user; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 eslint (1.23.1)
[error] 27-27: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 32-32: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
| public static async fetchMultiple<T extends GenericModel>( | ||
| modelClass: new() => T, | ||
| ): Promise<Array<T>> { | ||
| const model = (modelClass as any) | ||
| const tableName = model.TABLE_NAME | ||
|
|
||
| const data = await supabase.from(tableName) | ||
| .select('*') | ||
|
|
||
| const records = data.data | ||
|
|
||
| const results: Array<T> = [] | ||
|
|
||
| records?.forEach(record => { | ||
| const user = model.parseJson(record) | ||
| results.push(user) | ||
| }) | ||
|
|
||
| return results | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add pagination and filtering support to fetchMultiple
The current implementation has several limitations:
- No pagination support could lead to performance issues with large datasets
- No filtering capabilities limits usefulness
- Similar type safety issues as fetchSingle
public static async fetchMultiple<T extends GenericModel>(
modelClass: new() => T,
+ options?: {
+ page?: number;
+ pageSize?: number;
+ filters?: Partial<Record<keyof T, unknown>>;
+ }
): Promise<Array<T>> {
- const model = (modelClass as any)
- const tableName = model.TABLE_NAME
+ const tableName = modelClass.prototype.constructor.TABLE_NAME
+ if (!tableName) {
+ throw new Error(`Table name not defined for model ${modelClass.name}`)
+ }
- const data = await supabase.from(tableName)
- .select('*')
+ let query = supabase.from(tableName).select('*')
+
+ // Apply filters
+ if (options?.filters) {
+ for (const [key, value] of Object.entries(options.filters)) {
+ if (modelClass.prototype.constructor.DATABASE_MAP?.[key]) {
+ query = query.eq(
+ modelClass.prototype.constructor.DATABASE_MAP[key],
+ value
+ )
+ }
+ }
+ }
+
+ // Apply pagination
+ if (options?.page && options?.pageSize) {
+ const start = (options.page - 1) * options.pageSize
+ query = query.range(start, start + options.pageSize - 1)
+ }
+
+ const { data, error } = await query
+
+ if (error) {
+ throw new Error(`Failed to fetch ${modelClass.name} list: ${error.message}`)
+ }
- const records = data.data
+ if (!data || data.length === 0) {
+ return []
+ }
const results: Array<T> = []
- records?.forEach(record => {
- const user = model.parseJson(record)
- results.push(user)
+ data.forEach(record => {
+ results.push(modelClass.prototype.constructor.parseJson(record))
})
return results📝 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.
| public static async fetchMultiple<T extends GenericModel>( | |
| modelClass: new() => T, | |
| ): Promise<Array<T>> { | |
| const model = (modelClass as any) | |
| const tableName = model.TABLE_NAME | |
| const data = await supabase.from(tableName) | |
| .select('*') | |
| const records = data.data | |
| const results: Array<T> = [] | |
| records?.forEach(record => { | |
| const user = model.parseJson(record) | |
| results.push(user) | |
| }) | |
| return results | |
| } | |
| public static async fetchMultiple<T extends GenericModel>( | |
| modelClass: new() => T, | |
| options?: { | |
| page?: number; | |
| pageSize?: number; | |
| filters?: Partial<Record<keyof T, unknown>>; | |
| } | |
| ): Promise<Array<T>> { | |
| const tableName = modelClass.prototype.constructor.TABLE_NAME | |
| if (!tableName) { | |
| throw new Error(`Table name not defined for model ${modelClass.name}`) | |
| } | |
| let query = supabase.from(tableName).select('*') | |
| // Apply filters | |
| if (options?.filters) { | |
| for (const [key, value] of Object.entries(options.filters)) { | |
| if (modelClass.prototype.constructor.DATABASE_MAP?.[key]) { | |
| query = query.eq( | |
| modelClass.prototype.constructor.DATABASE_MAP[key], | |
| value | |
| ) | |
| } | |
| } | |
| } | |
| // Apply pagination | |
| if (options?.page && options?.pageSize) { | |
| const start = (options.page - 1) * options.pageSize | |
| query = query.range(start, start + options.pageSize - 1) | |
| } | |
| const { data, error } = await query | |
| if (error) { | |
| throw new Error(`Failed to fetch ${modelClass.name} list: ${error.message}`) | |
| } | |
| if (!data || data.length === 0) { | |
| return [] | |
| } | |
| const results: Array<T> = [] | |
| data.forEach(record => { | |
| results.push(modelClass.prototype.constructor.parseJson(record)) | |
| }) | |
| return results | |
| } |
🧰 Tools
🪛 eslint (1.23.1)
[error] 67-67: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
| // User.fetchSingle().then(user => { | ||
| // console.log(user) | ||
| // }) | ||
|
|
||
| // const supabaseService = new SupabaseService() | ||
| // SupabaseService.fetchSingle(User, "c7e80cb0-7d3d-411f-9983-5e3addf62980", { email: "ncuapp@test.com" }).then( | ||
| // response => { | ||
| // console.log(response) | ||
| // } | ||
| // ) | ||
|
|
||
| // SupabaseService.fetchMultiple(User).then( | ||
| // response => { | ||
| // console.log(response) | ||
| // } | ||
| // ) | ||
|
|
||
| // async function fetch() { | ||
| // const { data } = await supabase.from('members').select('*') | ||
| // console.log(data) | ||
| // } | ||
| // fetch() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up commented data fetching implementations.
There are multiple commented sections showing different approaches to data fetching. This could lead to confusion about the intended implementation. Additionally, there are hardcoded values (UUID, email) that shouldn't be committed.
Consider:
- Choosing and implementing one data fetching approach
- Moving test data to a dedicated test file
- Using environment variables for sensitive values
| const homeItems = [ | ||
| { title: "test", description: "Hello world." }, | ||
| { title: "test", description: "Hello world." }, | ||
| { title: "test", description: "Hello world." }, | ||
| { title: "test", description: "Hello world." }, | ||
| { title: "test", description: "Hello world." }, | ||
| { title: "test", description: "Hello world." }, | ||
| { title: "test", description: "Hello world." }, | ||
| { title: "test", description: "Hello world." }, | ||
| { title: "test", description: "Hello world." }, | ||
| { title: "test", description: "Hello world." }, | ||
| { title: "test", description: "Hello world." }, | ||
| ] |
There was a problem hiding this comment.
Remove or utilize the unused homeItems array.
The homeItems array contains placeholder data and is currently unused since the mapping code is commented out. Either remove this array or implement its intended functionality.
-const homeItems = [
- { title: "test", description: "Hello world." },
- { title: "test", description: "Hello world." },
- { title: "test", description: "Hello world." },
- { title: "test", description: "Hello world." },
- { title: "test", description: "Hello world." },
- { title: "test", description: "Hello world." },
- { title: "test", description: "Hello world." },
- { title: "test", description: "Hello world." },
- { title: "test", description: "Hello world." },
- { title: "test", description: "Hello world." },
- { title: "test", description: "Hello world." },
-]📝 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.
| const homeItems = [ | |
| { title: "test", description: "Hello world." }, | |
| { title: "test", description: "Hello world." }, | |
| { title: "test", description: "Hello world." }, | |
| { title: "test", description: "Hello world." }, | |
| { title: "test", description: "Hello world." }, | |
| { title: "test", description: "Hello world." }, | |
| { title: "test", description: "Hello world." }, | |
| { title: "test", description: "Hello world." }, | |
| { title: "test", description: "Hello world." }, | |
| { title: "test", description: "Hello world." }, | |
| { title: "test", description: "Hello world." }, | |
| ] |
🧰 Tools
🪛 eslint (1.23.1)
[error] 4-4: 'homeItems' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
| <div className="avatar absolute right-[15px] bottom-[-25px]"> | ||
| <div className="ring-gray-300 w-[125px] rounded-full ring"> | ||
| <img src="https://img.daisyui.com/images/stock/photo-1534528741775-53994a69daeb.webp" /> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace stock photo URL with a proper avatar handling system.
The current implementation uses a hardcoded stock photo URL. Consider:
- Implementing proper avatar handling with fallbacks
- Adding alt text for accessibility
- Handling loading and error states
<div className="avatar absolute right-[15px] bottom-[-25px]">
<div className="ring-gray-300 w-[125px] rounded-full ring">
- <img src="https://img.daisyui.com/images/stock/photo-1534528741775-53994a69daeb.webp" />
+ <img
+ src={user?.avatarUrl ?? defaultAvatarUrl}
+ alt={`${user?.name}'s avatar`}
+ onError={(e) => e.currentTarget.src = defaultAvatarUrl}
+ />
</div>
</div>Committable suggestion skipped: line range outside the PR's diff.
| <h1 className='font-bold text-4xl text-black'>這裡最多七個字</h1> | ||
| <h2 className='font-bold text-black'>中文系 二年級</h2> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Externalize text content for internationalization.
The hardcoded Chinese text should be moved to a localization system to support multiple languages.
- <h1 className='font-bold text-4xl text-black'>這裡最多七個字</h1>
- <h2 className='font-bold text-black'>中文系 二年級</h2>
+ <h1 className='font-bold text-4xl text-black'>{t('home.title')}</h1>
+ <h2 className='font-bold text-black'>{t('home.subtitle')}</h2>Committable suggestion skipped: line range outside the PR's diff.
通用資料模型 & 資料庫連接服務
透過建立通用模型，供各功能自行延伸，並利用 SupabaseService，使 CRUD 可集中處理。目前引入了 fetchSingle() 與 fetchMultiple() 功能，服務會同時自動轉換資料庫欄位，並以給定的 type 輸出結果。
A generic data template model has been implemented. This model can be extended to other classes, such as User. It includes the TABLE_NAME and DATABASE_MAPPING properties for a more efficient database access.
The SupabaseService has been implemented to support general data fetching. It currently supports the fetching of a single object and an array of objects. There is no need to implement the data fetching logic repeatedly for each model.
Note: 首頁 GUI 可暫時忽略
Summary by CodeRabbit
Release Notes
GenericModelfor enhanced data handling.Userclass with various user attributes and JSON parsing capabilities.SupabaseServicefor streamlined database interactions, including methods for fetching single and multiple records.These updates enhance data management and user interface capabilities within the application.