-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feat frontend api refactor #24
Changes from all commits
47bf8ff
bb7478a
7f7a682
2fd07ab
ff0011c
7afa251
ac9c5a2
a034248
666d159
ffbe0dd
9c1924e
d90bf48
72209ba
4902e36
2314719
f2575a3
42c7a88
652c9b5
73a810d
ce340f5
806894b
3729eb7
bb28f69
ff17f54
d07ef4c
fb4319f
a2310f7
eb8ccca
d89f771
abd7b81
723a6e4
6279339
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,4 +12,7 @@ dist/ | |
|
||
# Models | ||
models/ | ||
*/**/models/ | ||
*/**/models/ | ||
|
||
*/**/database.sqlite | ||
./backend/src/database.sqlite |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"typescript.tsdk": "node_modules/typescript/lib", | ||
"typescript.enablePromptUseWorkspaceTsdk": true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"cSpell.words": ["upsert"] | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,33 @@ | ||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||
Injectable, | ||||||||||||||||||||||||||||||
NestInterceptor, | ||||||||||||||||||||||||||||||
ExecutionContext, | ||||||||||||||||||||||||||||||
CallHandler, | ||||||||||||||||||||||||||||||
Logger, | ||||||||||||||||||||||||||||||
} from '@nestjs/common'; | ||||||||||||||||||||||||||||||
import { Observable } from 'rxjs'; | ||||||||||||||||||||||||||||||
import { GqlExecutionContext } from '@nestjs/graphql'; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
@Injectable() | ||||||||||||||||||||||||||||||
export class LoggingInterceptor implements NestInterceptor { | ||||||||||||||||||||||||||||||
private readonly logger = new Logger('GraphQLRequest'); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
intercept(context: ExecutionContext, next: CallHandler): Observable<any> { | ||||||||||||||||||||||||||||||
const ctx = GqlExecutionContext.create(context); | ||||||||||||||||||||||||||||||
const { operation, fieldName } = ctx.getInfo(); | ||||||||||||||||||||||||||||||
let variables = ''; | ||||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||||
variables = ctx.getContext().req.body.variables; | ||||||||||||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||||||||||||
variables = ''; | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
Comment on lines
+19
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve error handling in variable extraction. The current implementation silently fails and sets variables to an empty string, which might hide important debugging information. Apply this diff to improve error handling: try {
variables = ctx.getContext().req.body.variables;
} catch (error) {
- variables = '';
+ this.logger.warn(
+ `Failed to extract variables: ${error.message}`,
+ error.stack,
+ );
+ variables = undefined;
} 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Check: autofix[warning] 21-21: |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
this.logger.log( | ||||||||||||||||||||||||||||||
`${operation.operation.toUpperCase()} \x1B[33m${fieldName}\x1B[39m${ | ||||||||||||||||||||||||||||||
variables ? ` Variables: ${JSON.stringify(variables)}` : '' | ||||||||||||||||||||||||||||||
}`, | ||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||
Comment on lines
+25
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance logging implementation with security and maintainability improvements. Several suggestions to improve the logging implementation:
Here's an improved implementation: - this.logger.log(
- `${operation.operation.toUpperCase()} \x1B[33m${fieldName}\x1B[39m${
- variables ? ` Variables: ${JSON.stringify(variables)}` : ''
- }`,
- );
+ const sanitizedVariables = this.sanitizeVariables(variables);
+ const startTime = Date.now();
+
+ this.logger.log({
+ type: operation.operation.toUpperCase(),
+ field: fieldName,
+ variables: sanitizedVariables,
+ timestamp: new Date().toISOString()
+ });
+
+ return next.handle().pipe(
+ tap(() => {
+ const duration = Date.now() - startTime;
+ this.logger.log(`Operation completed in ${duration}ms`);
+ })
+ );
// Add this method to the class
private sanitizeVariables(variables: any): any {
if (!variables) return undefined;
const sensitiveFields = ['password', 'token', 'secret'];
return Object.entries(variables).reduce((acc, [key, value]) => {
acc[key] = sensitiveFields.includes(key.toLowerCase()) ? '[REDACTED]' : value;
return acc;
}, {});
}
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
return next.handle(); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
Comment on lines
+31
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider adding request timing and error handling to the response pipeline. The current implementation doesn't track request completion or handle errors in the response pipeline. - return next.handle();
+ return next.handle().pipe(
+ tap({
+ error: (error) => {
+ this.logger.error(
+ `Operation failed: ${error.message}`,
+ error.stack
+ );
+ }
+ })
+ );
|
||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,9 +40,12 @@ | |
"class-validator": "^0.14.1", | ||
"fs-extra": "^11.2.0", | ||
"graphql": "^16.9.0", | ||
"graphql-subscriptions": "^2.0.0", | ||
"graphql-ws": "^5.16.0", | ||
"reflect-metadata": "^0.2.2", | ||
"rxjs": "^7.8.1", | ||
"sqlite3": "^5.1.7", | ||
"subscriptions-transport-ws": "^0.11.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Remove The codebase is already properly configured to use the modern
You can safely:
🔗 Analysis chainConsider removing The Let's verify if we can safely remove this dependency: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check if any files are still using subscriptions-transport-ws
rg -l "subscriptions-transport-ws"
# Check for specific usage of the legacy protocol
ast-grep --pattern 'transport: "ws"'
Length of output: 204 Script: #!/bin/bash
# Let's check the actual usage in app.module.ts and look for any subscription setup
rg -A 10 "subscriptions-transport-ws" backend/src/app.module.ts
# Also check for any GraphQL subscription setup patterns
ast-grep --pattern 'GraphQLModule.forRoot({
$$$
})'
# Check if graphql-ws is also present
rg -l "graphql-ws"
Length of output: 619 Script: #!/bin/bash
# Let's check the GraphQL client setup in frontend
rg -A 10 "graphql-ws" frontend/src/lib/client.ts
# Check if there are any WebSocket transport configurations
rg -A 5 "WebSocketLink|wsLink" frontend/src/lib/client.ts
# Check the complete GraphQL module configuration
rg -A 15 "GraphQLModule.forRoot" backend/src/app.module.ts
Length of output: 1380 |
||
"typeorm": "^0.3.20" | ||
}, | ||
"devDependencies": { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,27 +10,31 @@ | |
import { TokenModule } from './token/token.module'; | ||
import { UserModule } from './user/user.module'; | ||
import { InitModule } from './init/init.module'; | ||
import { RolesGuard } from './guard/roles.guard'; | ||
import { MenuGuard } from './guard/menu.guard'; | ||
import { User } from './user/user.model'; | ||
import { AppResolver } from './app.resolver'; | ||
import { APP_INTERCEPTOR } from '@nestjs/core'; | ||
import { LoggingInterceptor } from 'interceptor/LoggingInterceptor'; | ||
|
||
@Module({ | ||
imports: [ | ||
ConfigModule.forRoot({ isGlobal: true }), | ||
GraphQLModule.forRoot<ApolloDriverConfig>({ | ||
driver: ApolloDriver, | ||
autoSchemaFile: join(process.cwd(), 'src/schema.gql'), | ||
autoSchemaFile: join(process.cwd(), '../frontend/src/graphql/schema.gql'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Schema file location creates deployment risks and tight coupling The verification confirms the architectural concerns. The only cross-directory reference is the schema path in
Recommended solution: - autoSchemaFile: join(process.cwd(), '../frontend/src/graphql/schema.gql'),
+ autoSchemaFile: join(process.cwd(), 'src/schema.gql'), Then implement one of these approaches:
🔗 Analysis chainConsider architectural implications of schema location. Moving the GraphQL schema file to the frontend directory creates a tight coupling between backend and frontend, which could lead to deployment and build issues. This approach:
Consider these alternatives:
- autoSchemaFile: join(process.cwd(), '../frontend/src/graphql/schema.gql'),
+ autoSchemaFile: join(process.cwd(), 'src/schema.gql'), Then add a build step to copy the schema to frontend.
- autoSchemaFile: join(process.cwd(), '../frontend/src/graphql/schema.gql'),
+ autoSchemaFile: join(process.cwd(), '../shared/schema.gql'),
Let's verify the potential impact: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check if there are any direct imports or references between frontend and backend
rg -g '!node_modules' -l 'frontend|backend' backend/
Length of output: 130 Script: #!/bin/bash
# Let's check the content of these files to understand the nature of frontend/backend references
echo "=== app.module.ts ==="
cat backend/src/app.module.ts
echo -e "\n=== protocol.ts ==="
cat backend/src/chat/protocol.ts
echo -e "\n=== package.json ==="
cat backend/package.json
Length of output: 8602 |
||
sortSchema: true, | ||
playground: true, | ||
installSubscriptionHandlers: true, | ||
subscriptions: { | ||
'graphql-ws': true, | ||
'subscriptions-transport-ws': true, | ||
}, | ||
context: ({ req, res }) => ({ req, res }), | ||
}), | ||
TypeOrmModule.forRoot({ | ||
type: 'sqlite', | ||
database: join(process.cwd(), 'src/database.sqlite'), | ||
synchronize: true, | ||
logging: true, | ||
entities: [__dirname + '/**/*.model{.ts,.js}'], | ||
}), | ||
InitModule, | ||
|
@@ -41,6 +45,12 @@ | |
ChatModule, | ||
TypeOrmModule.forFeature([User]), | ||
], | ||
providers: [AppResolver], | ||
providers: [ | ||
AppResolver, | ||
{ | ||
provide: APP_INTERCEPTOR, | ||
useClass: LoggingInterceptor, | ||
}, | ||
], | ||
}) | ||
export class AppModule {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,7 +55,7 @@ | |
|
||
async login( | ||
loginUserInput: LoginUserInput, | ||
): Promise<{ access_token: string }> { | ||
): Promise<{ accessToken: string }> { | ||
const { username, password } = loginUserInput; | ||
|
||
const user = await this.userRepository.findOne({ | ||
|
@@ -73,10 +73,10 @@ | |
} | ||
|
||
const payload = { userId: user.id, username: user.username }; | ||
const access_token = this.jwtService.sign(payload); | ||
this.jwtCacheService.storeToken(access_token); | ||
const accessToken = this.jwtService.sign(payload); | ||
this.jwtCacheService.storeToken(accessToken); | ||
Comment on lines
+76
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider enhancing token management configuration. While the current implementation is functional, consider these improvements:
Here's a suggested implementation: const payload = { userId: user.id, username: user.username };
-const accessToken = this.jwtService.sign(payload);
+const accessToken = this.jwtService.sign(payload, {
+ expiresIn: this.configService.get('JWT_EXPIRATION', '1h'),
+}); And in the validateToken method: try {
await this.jwtService.verifyAsync(params.token);
return this.jwtCacheService.isTokenStored(params.token);
} catch (error) {
- console.log(error);
+ Logger.error('Token validation failed', {
+ error: error.message,
+ token: params.token.substring(0, 10) + '...',
+ });
return false;
}
|
||
|
||
return { access_token }; | ||
return { accessToken }; | ||
} | ||
|
||
async validateToken(params: CheckTokenInput): Promise<boolean> { | ||
|
@@ -84,7 +84,7 @@ | |
await this.jwtService.verifyAsync(params.token); | ||
return this.jwtCacheService.isTokenStored(params.token); | ||
} catch (error) { | ||
console.log(error); | ||
return false; | ||
} | ||
} | ||
|
@@ -92,7 +92,7 @@ | |
Logger.log('logout token', token); | ||
try { | ||
await this.jwtService.verifyAsync(token); | ||
} catch (error) { | ||
return false; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -18,6 +18,15 @@ import { Message } from 'src/chat/message.model'; | |||||||||||||||||
import { SystemBaseModel } from 'src/system-base-model/system-base.model'; | ||||||||||||||||||
import { User } from 'src/user/user.model'; | ||||||||||||||||||
|
||||||||||||||||||
export enum StreamStatus { | ||||||||||||||||||
STREAMING = 'streaming', | ||||||||||||||||||
DONE = 'done', | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
registerEnumType(StreamStatus, { | ||||||||||||||||||
name: 'StreamStatus', | ||||||||||||||||||
}); | ||||||||||||||||||
|
||||||||||||||||||
@Entity() | ||||||||||||||||||
@ObjectType() | ||||||||||||||||||
export class Chat extends SystemBaseModel { | ||||||||||||||||||
|
@@ -44,6 +53,18 @@ class ChatCompletionDelta { | |||||||||||||||||
content?: string; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
@ObjectType('ChatCompletionChoiceType') | ||||||||||||||||||
class ChatCompletionChoice { | ||||||||||||||||||
@Field() | ||||||||||||||||||
index: number; | ||||||||||||||||||
|
||||||||||||||||||
@Field(() => ChatCompletionDelta) | ||||||||||||||||||
delta: ChatCompletionDelta; | ||||||||||||||||||
|
||||||||||||||||||
@Field({ nullable: true }) | ||||||||||||||||||
finishReason: string | null; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
@ObjectType('ChatCompletionChunkType') | ||||||||||||||||||
export class ChatCompletionChunk { | ||||||||||||||||||
@Field() | ||||||||||||||||||
|
@@ -59,20 +80,15 @@ export class ChatCompletionChunk { | |||||||||||||||||
model: string; | ||||||||||||||||||
|
||||||||||||||||||
@Field({ nullable: true }) | ||||||||||||||||||
system_fingerprint: string | null; | ||||||||||||||||||
systemFingerprint: string | null; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Update required: Inconsistent field naming across the codebase The field rename from
While the model and frontend use camelCase, protocol files still use snake_case, which needs to be synchronized. 🔗 Analysis chainVerify the impact of the systemFingerprint rename. The field rename from Also applies to: 88-90 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any remaining snake_case usage of system_fingerprint
rg -l "system_fingerprint" --type ts --type tsx
# Search for GraphQL queries/mutations using the old field name
rg -l "system_fingerprint" --type graphql
Length of output: 149 Script: #!/bin/bash
# Search for any remaining snake_case usage of system_fingerprint in all files
rg "system_fingerprint"
# Search for camelCase usage to understand the context
rg "systemFingerprint"
Length of output: 666 |
||||||||||||||||||
|
||||||||||||||||||
@Field(() => [ChatCompletionChoice]) | ||||||||||||||||||
choices: ChatCompletionChoice[]; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
@ObjectType('ChatCompletionChoiceType') | ||||||||||||||||||
class ChatCompletionChoice { | ||||||||||||||||||
@Field() | ||||||||||||||||||
index: number; | ||||||||||||||||||
|
||||||||||||||||||
@Field(() => ChatCompletionDelta) | ||||||||||||||||||
delta: ChatCompletionDelta; | ||||||||||||||||||
@Field(() => StreamStatus) | ||||||||||||||||||
status: StreamStatus; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
@Field({ nullable: true }) | ||||||||||||||||||
finish_reason: string | null; | ||||||||||||||||||
export function isDoneStatus(chunk: ChatCompletionChunk): boolean { | ||||||||||||||||||
return chunk.status === StreamStatus.DONE; | ||||||||||||||||||
Comment on lines
+92
to
+93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider making the status check more flexible. While the current implementation works, consider making it more flexible to handle different status checks. -export function isDoneStatus(chunk: ChatCompletionChunk): boolean {
- return chunk.status === StreamStatus.DONE;
+export function hasStatus(chunk: ChatCompletionChunk, status: StreamStatus): boolean {
+ return chunk.status === status;
+}
+
+export const isDoneStatus = (chunk: ChatCompletionChunk): boolean =>
+ hasStatus(chunk, StreamStatus.DONE); This approach allows for more reusability while maintaining the existing functionality. 📝 Committable suggestion
Suggested change
|
||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding explicit return types rule.
Since you're enhancing TypeScript support, consider also adding the
@typescript-eslint/explicit-function-return-type
rule to ensure all functions have explicit return types.📝 Committable suggestion