-
Notifications
You must be signed in to change notification settings - Fork 28
WIP - linked to cdtn admin #7110
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
base: dev
Are you sure you want to change the base?
Conversation
revu-bot
left a comment
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.
Summary
This WIP PR introduces changes to search functionality and IDCC handling. The review identifies 3 critical issues and 2 important concerns that need attention:
Critical Issues
- Unused import causing potential build failures
- Debug console.log left in production code
- Type safety regression in IDCC parsing (parseFloat → parseInt)
Important Issues
- Commented-out code should be removed or documented
- Incomplete TODO comment needs clarification
Overall Assessment
The PR shows good progress on search improvements, but contains debugging artifacts and incomplete refactoring that should be addressed before merging.
|
| import { PresearchClass, PreSearchResult, SEARCH_ALGO } from "./types"; | ||
| import { format } from "path"; |
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.
[CRITICAL] Unused import will cause build failures
- Why it matters: This unused import from 'path' module will trigger linting errors and potentially break the build
- Impact: Build pipeline failure, code quality degradation
- Action required: Remove this import as it's not used anywhere in the file
| import { PresearchClass, PreSearchResult, SEARCH_ALGO } from "./types"; | |
| import { format } from "path"; | |
| import { PresearchClass, PreSearchResult, SEARCH_ALGO } from "./types"; |
| const pq = this.searchParams.get("pq"); | ||
| console.log({ q, pq }); | ||
| const response = await searchWithQuery(q as string, 5, pq != "false"); |
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.
[CRITICAL] Debug console.log left in production code
- Why it matters: Console logs in production expose internal query parameters and degrade performance
- Security concern: May leak sensitive search patterns or user behavior
- Action required: Remove this debug statement before merging
| const pq = this.searchParams.get("pq"); | |
| console.log({ q, pq }); | |
| const response = await searchWithQuery(q as string, 5, pq != "false"); | |
| const pq = this.searchParams.get("pq"); | |
| const response = await searchWithQuery(q as string, 5, pq != "false"); |
| // { | ||
| // match: { | ||
| // source: { | ||
| // boost: 1.1, | ||
| // query: SOURCES.SHEET_SP, | ||
| // }, | ||
| // }, | ||
| // }, |
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.
[IMPORTANT] Commented-out code should be removed or documented
- Why it matters: Commented code creates confusion about intent and clutters the codebase
- Options:
- If this boost is no longer needed, remove it entirely
- If it's temporarily disabled for testing, add a TODO with context and timeline
- If it's for future reference, move to documentation or git history
Recommendation: Either remove completely or add a clear comment explaining why it's disabled and when it should be reconsidered.
|
🎉 Deployment for commit 9a55999 : Docker images
|



No description provided.