resume data added with contentType#126
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request changes the type of resumeData from number to string in both the DTO and the entity, and introduces logic in TrackingContentService to parse resumeData from contentType when it contains a pipe delimiter. The review highlights two critical issues: first, a type mismatch between the TypeScript entity property (string) and the database column (double precision), which could lead to runtime errors or database failures; second, the parsing logic is only executed during updates and is skipped for new records, which will result in raw unparsed content types being saved. It is recommended to keep resumeData as a number, parse the string values appropriately, and move the parsing logic to the beginning of the method to cover both creation and update paths.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| nullable: true, | ||
| }) | ||
| resumeData: number; | ||
| resumeData: string; |
There was a problem hiding this comment.
The database column type is defined as double precision (line 49), but the TypeScript property type has been changed to string.\n\nThis mismatch can lead to:\n1. Type Pollution: When TypeORM fetches records from the database, it will return a number for resumeData, but TypeScript will treat it as a string at compile-time, potentially causing runtime errors if string methods are called on it.\n2. Database Errors: Attempting to insert a non-numeric string into a double precision column will cause database insert/update failures.\n\nRecommendation: Keep resumeData typed as number in the entity. Any string values from the DTO or parsed from contentType should be converted to a number (e.g., using parseFloat) before saving to the database.
| resumeData: string; | |
| resumeData: number; |
| try{ | ||
| let temp_contentType=createContentTrackingDto.contentType; | ||
| if(temp_contentType.includes("|")){ | ||
| const parts = temp_contentType.split('|'); | ||
| createContentTrackingDto.contentType=parts[0]; | ||
| if(parts.length>1) | ||
| { | ||
| createContentTrackingDto.resumeData=parts[1]; | ||
| } | ||
| else{ | ||
| createContentTrackingDto.resumeData="0"; | ||
| } | ||
| } | ||
| else{ | ||
| createContentTrackingDto.resumeData="0"; | ||
| } | ||
| } | ||
| catch(e){ | ||
| } |
There was a problem hiding this comment.
The parsing of contentType and extraction of resumeData is currently placed inside the if (result_content.length > 0) block (which handles updates). For new tracking records (the else block), this parsing is completely skipped. This means the raw contentType containing the | character (e.g., 'video/mp4|120.5') will be saved directly to the database, and resumeData will not be extracted.\n\nAdditionally, if createContentTrackingDto.contentType is undefined or null, calling .includes() will throw a TypeError. The empty catch(e) {} block silently swallows errors, making debugging difficult.\n\nRecommendation:\nMove this parsing logic to the beginning of the createContentTracking method (before the result_content query around line 229) so that it applies to both new and existing records. Once moved, you can simplify this block to just parse the resumeData as a float.\n\nExample parsing logic to place before line 229:\ntypescript\nconst contentType = createContentTrackingDto.contentType;\nif (typeof contentType === 'string' && contentType.includes('|')) {\n const [cleanType, resumeStr] = contentType.split('|');\n createContentTrackingDto.contentType = cleanType;\n createContentTrackingDto.resumeData = resumeStr || '0';\n}\n
// Parsing logic moved before line 229 to support both new and existing records1 participant
No description provided.