Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Generic data model and Supabase data service #124

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/models/GenericModel.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// GenericModel class
export default abstract class GenericModel {
// Abstract property for table name
static TABLE_NAME: string;

static DATABASE_MAP: Object;
Comment on lines +3 to +6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve type safety of DATABASE_MAP.

The Object type is too broad and provides little type safety. Consider:

  1. Defining a specific interface for the mapping structure
  2. 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 object instead.
  • If you want a type meaning "any value", you probably want unknown instead.
  • If you really want a type meaning "any non-nullish value", you probably want NonNullable<unknown> instead.

(@typescript-eslint/ban-types)


// Abstract method for parsing JSON
static parseJson(json: any): GenericModel {
throw new Error("parseJson method not implemented.");
}
Comment on lines +8 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve type safety and error handling of parseJson method.

Several improvements can be made to enhance type safety and developer experience:

  1. Use unknown instead of any for better type safety
  2. Make the return type match the implementing class using generics
  3. 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)

}
36 changes: 36 additions & 0 deletions src/models/User.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import GenericModel from "./GenericModel"

export default class User extends GenericModel {

static TABLE_NAME = "members"
static DATABASE_MAP: Object = {
id: "uuid",
username: "username",
email: "fk_email",
identity: "fk_identity",
avatar: "avatar",
}
Comment on lines +5 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve type safety and documentation of database mappings.

  1. Replace the broad Object type 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",
     }
  1. 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 object instead.
  • If you want a type meaning "any value", you probably want unknown instead.
  • If you really want a type meaning "any non-nullish value", you probably want NonNullable<unknown> instead.

(@typescript-eslint/ban-types)

Comment on lines +6 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Database mapping is incomplete - several properties are missing

The following public properties are not mapped in DATABASE_MAP but might need persistence:

  • phone
  • profileBackground
  • joinedAt
  • department
  • grade
  • bio

This mismatch between class properties and database mappings could lead to data not being properly persisted or retrieved from the database.

🔗 Analysis chain

Verify database mapping completeness.

Ensure that all User properties that need database persistence are included in the DATABASE_MAP.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential missing database mappings

# Get all public properties from the User class
echo "Public properties in User class:"
rg "^\s+public\s+(\w+):" src/models/User.ts

# Compare with database mappings
echo -e "\nDatabase mappings:"
rg "static\s+DATABASE_MAP.*?=.*?{.*?}" src/models/User.ts -U --multiline

Length of output: 896


Script:

#!/bin/bash
# Description: Check for potential missing database mappings with fixed regex

# Get all public properties from the User class (already have this)
# Now let's get the DATABASE_MAP with a proper regex
echo "Database mappings:"
rg "static\s+DATABASE_MAP.*?\{[\s\S]+?\}" src/models/User.ts

# Alternative approach using ast-grep to find the object literal
echo -e "\nUsing ast-grep to find DATABASE_MAP:"
ast-grep --pattern 'static DATABASE_MAP: Object = { $$$ }'

Length of output: 259


Script:

#!/bin/bash
# Let's try a simpler approach with basic pattern matching

echo "Database mappings (simple search):"
rg "DATABASE_MAP" -A 10 src/models/User.ts

# Also get the complete User class to see the context
echo -e "\nComplete User class definition:"
ast-grep --pattern 'class User { $$$ }'

Length of output: 802

🧰 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 object instead.
  • If you want a type meaning "any value", you probably want unknown instead.
  • 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 = ""
Comment on lines +14 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve type definitions and add property documentation.

  1. 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.

Suggested change
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
}
Comment on lines +27 to +35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Added proper type safety with Record<string, unknown>
  2. Replaced this with explicit class name
  3. Added error handling
  4. Added type validation for properties
  5. 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.

Suggested change
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)

}
89 changes: 89 additions & 0 deletions src/routes/home/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { createFileRoute } from '@tanstack/react-router'
import { VStack } from '../../components'

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." },
]
Comment on lines +4 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)


// 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()

Comment on lines +18 to +40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Choosing and implementing one data fetching approach
  2. Moving test data to a dedicated test file
  3. Using environment variables for sensitive values

export const Route = createFileRoute('/home/')({

component: () => <div>
<section className='h-[200px] mb-[40px] bg-yellow-50 relative border-b-[5px] border-gray-600'>

<div className='flex flex-col absolute left-[10px] bottom-[5px]'>
<h1 className='font-bold text-4xl text-black'>這裡最多七個字</h1>
<h2 className='font-bold text-black'>中文系 二年級</h2>
Comment on lines +47 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

</div>

{/* <div className='
w-[125px] h-[125px]
absolute right-[15px] bottom-[-25px]
border-[4px] border-gray-600 rounded-full bg-gray-500
'></div> */}

<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>
Comment on lines +57 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Replace stock photo URL with a proper avatar handling system.

The current implementation uses a hardcoded stock photo URL. Consider:

  1. Implementing proper avatar handling with fallbacks
  2. Adding alt text for accessibility
  3. 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.


<span className='
absolute top-[20px] right-[15px]
text-black font-bold text-xs
'>
變更個人檔案
</span>
</section>

<VStack className='px-[10px] gap-[20px]'>


{/* {homeItems.map((item, index) => (
<WelcomeCard title={item.title} description={item.description} />
))} */}

{/* <NcuInput />

<NcuTextarea>
Hello
</NcuTextarea>

<NcuSwitch label="test">X</NcuSwitch>

<NcuButton>Test</NcuButton> */}
</VStack>
</div>,
})
84 changes: 84 additions & 0 deletions src/services/SupabaseService.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import GenericModel from "../models/GenericModel";
import { supabase } from "../utils/supabase";

// Supabase service for quickly accessing data
export default class SupabaseService {
Comment on lines +1 to +5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider refactoring to instance-based design for better testability

The current static-only class design could make unit testing and dependency injection more difficult. Consider refactoring to an instance-based design:

 import GenericModel from "../models/GenericModel";
 import { supabase } from "../utils/supabase";
 
-// Supabase service for quickly accessing data
-export default class SupabaseService {
+export interface ISupabaseClient {
+  from(table: string): any;
+}
+
+export default class SupabaseService {
+  constructor(private readonly client: ISupabaseClient = supabase) {}
📝 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
import GenericModel from "../models/GenericModel";
import { supabase } from "../utils/supabase";
// Supabase service for quickly accessing data
export default class SupabaseService {
import GenericModel from "../models/GenericModel";
import { supabase } from "../utils/supabase";
export interface ISupabaseClient {
from(table: string): any;
}
export default class SupabaseService {
constructor(private readonly client: ISupabaseClient = supabase) {}



/* Fetch a single object
*
* Parameters
* ----------
* modelClass: class
* The target model class
* targetID: str
* The target record ID
* criteria: Dictionary
* Extra search criteria, please follow the DATABASE_MAPPING of the target model
*
* Returns
* -------
* The target object (of the target model type). Null if record is not found.
*/
public static async fetchSingle<T extends GenericModel>(
modelClass: new() => T,
targetID: String,
criteria?: Partial<Record<string, any>>,
): Promise<T | null> {
Comment on lines +23 to +27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix TypeScript type declarations

The method signature has some type-related issues:

  • Use lowercase string instead of String
  • The criteria type could be more specific
 public static async fetchSingle<T extends GenericModel>(
     modelClass: new() => T,
-    targetID:   String,
-    criteria?: Partial<Record<string, any>>,
+    targetID: string,
+    criteria?: Partial<Record<keyof T, unknown>>,
 ): Promise<T | null> {
📝 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
public static async fetchSingle<T extends GenericModel>(
modelClass: new() => T,
targetID: String,
criteria?: Partial<Record<string, any>>,
): Promise<T | null> {
public static async fetchSingle<T extends GenericModel>(
modelClass: new() => T,
targetID: string,
criteria?: Partial<Record<keyof T, unknown>>,
): Promise<T | null> {
🧰 Tools
🪛 Biome (1.9.4)

[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 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)

const model = (modelClass as any)
const tableName = model.TABLE_NAME

var query = supabase.from(tableName)
.select('*')
.eq("uuid", targetID)

for (const key in criteria)
if (model.DATABASE_MAP[key])
query = query.eq(model.DATABASE_MAP[key], criteria[key])

Comment on lines +28 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve type safety and query building

The current implementation has several areas for improvement:

  • Avoid type casting to any
  • Use const instead of var
  • Add error handling for invalid table name
-    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}`)
+    }
 
-    var query = supabase.from(tableName)
+    let query = supabase.from(tableName)
                     .select('*')
                     .eq("uuid", targetID)
     
     for (const key in criteria)
-        if (model.DATABASE_MAP[key])
-            query = query.eq(model.DATABASE_MAP[key], criteria[key])
+        if (modelClass.prototype.constructor.DATABASE_MAP?.[key]) {
+            query = query.eq(
+              modelClass.prototype.constructor.DATABASE_MAP[key],
+              criteria[key]
+            )
+        }
📝 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
const model = (modelClass as any)
const tableName = model.TABLE_NAME
var query = supabase.from(tableName)
.select('*')
.eq("uuid", targetID)
for (const key in criteria)
if (model.DATABASE_MAP[key])
query = query.eq(model.DATABASE_MAP[key], criteria[key])
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('*')
.eq("uuid", targetID)
for (const key in criteria)
if (modelClass.prototype.constructor.DATABASE_MAP?.[key]) {
query = query.eq(
modelClass.prototype.constructor.DATABASE_MAP[key],
criteria[key]
)
}
🧰 Tools
🪛 eslint (1.23.1)

[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)

const data = await query;

if (!data.data || data.data.length == 0) {
return null
}
const record = data.data[0]

return model.parseJson(record)
}




/* Fetch an array of objects
*
* Parameters
* ----------
* modelClass: class
* The target model class
*
* Returns
* -------
* Array of objects (of the target model type).
* Returns empty array if no records can be found.
*/
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
}
Comment on lines +64 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add pagination and filtering support to fetchMultiple

The current implementation has several limitations:

  1. No pagination support could lead to performance issues with large datasets
  2. No filtering capabilities limits usefulness
  3. 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.

Suggested change
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)

}
Loading