Issue feat: Processing course /content issue on cms#2968
Conversation
![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 modifies KaTableComponent.tsx to include logic for handling content with a 'Processing' status, specifically setting the interaction mode to 'review' and updating the routing behavior. The review feedback correctly identifies a double-routing issue where multiple navigation events could be triggered simultaneously. Other noted improvements include removing debug console logs, ensuring consistent null-safety for the content object, and correcting formatting inconsistencies. A refactored code structure was suggested to address these concerns.
| console.log('Opening all-content review for identifier:', identifier); | ||
| if (content?.status === 'Processing') { | ||
| const pathname = | ||
|
|
||
| `/workspace/content/review` | ||
| router.push({ pathname, query: { identifier } }); | ||
| } | ||
| content.contentType === 'Course' | ||
| ? router.push({ | ||
| pathname: `/course-hierarchy/${identifier}`, | ||
| query: { | ||
| identifier, | ||
| isReadOnly: true, | ||
| previousPage: 'allContents', | ||
| }, | ||
| }) | ||
| : router.push({ | ||
| pathname: `/workspace/content/review`, | ||
| query: { identifier, isReadOnly: true, isAllContents: true }, | ||
| }); |
There was a problem hiding this comment.
The current implementation introduces several issues that should be addressed:
- Double Routing: When
content?.status === 'Processing', the code executes arouter.pushat line 163 and then immediately proceeds to evaluate the ternary operator at line 165, which triggers a secondrouter.push. This results in redundant navigation and potential race conditions in the router. - Debug Logging: The
console.logstatement at line 158 should be removed before merging to production. - Defensive Programming: At line 165,
content.contentTypeis accessed without a null check, even thoughcontentis treated as potentially nullable at line 159 (content?.status). - Code Quality: The indentation and line breaks within the
ifblock (lines 160-162) are inconsistent and hinder readability.
Using an if...else if...else structure resolves the double routing and improves code clarity.
| console.log('Opening all-content review for identifier:', identifier); | |
| if (content?.status === 'Processing') { | |
| const pathname = | |
| `/workspace/content/review` | |
| router.push({ pathname, query: { identifier } }); | |
| } | |
| content.contentType === 'Course' | |
| ? router.push({ | |
| pathname: `/course-hierarchy/${identifier}`, | |
| query: { | |
| identifier, | |
| isReadOnly: true, | |
| previousPage: 'allContents', | |
| }, | |
| }) | |
| : router.push({ | |
| pathname: `/workspace/content/review`, | |
| query: { identifier, isReadOnly: true, isAllContents: true }, | |
| }); | |
| if (content?.status === 'Processing') { | |
| router.push({ | |
| pathname: `/workspace/content/review`, | |
| query: { identifier }, | |
| }); | |
| } else if (content?.contentType === 'Course') { | |
| router.push({ | |
| pathname: `/course-hierarchy/${identifier}`, | |
| query: { | |
| identifier, | |
| isReadOnly: true, | |
| previousPage: 'allContents', | |
| }, | |
| }); | |
| } else { | |
| router.push({ | |
| pathname: `/workspace/content/review`, | |
| query: { identifier, isReadOnly: true, isAllContents: true }, | |
| }); | |
| } |
|
2 participants
No description provided.