-
Notifications
You must be signed in to change notification settings - Fork 0
Eden/value matching #56
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
Conversation
logger.info( | ||
f"🧰Tool called: search_explanations with query='{query}', " | ||
f"limit={limit}" | ||
f"🧰Tool called: search_explanations with query='{query}', " f"limit={limit}" |
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.
[black-format] reported by reviewdog 🐶
f"🧰Tool called: search_explanations with query='{query}', " f"limit={limit}" | |
f"🧰Tool called: search_explanations with query='{query}', " | |
f"limit={limit}" |
self.cached_candidates[ | ||
"matchers" | ||
] = self.weight_updater.update_matchers( | ||
self.cached_candidates["matchers"] |
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.
[black-format] reported by reviewdog 🐶
self.cached_candidates[ | |
"matchers" | |
] = self.weight_updater.update_matchers( | |
self.cached_candidates["matchers"] | |
self.cached_candidates["matchers"] = ( | |
self.weight_updater.update_matchers( | |
self.cached_candidates["matchers"] | |
) |
Coverage report
Test suite run success3 tests passing in 3 suites. Report generated by 🧪jest coverage report action from 4b7c585 |
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.
Pull Request Overview
This PR implements an enhanced value matching system with a new custom table interface for value comparisons. The changes replace the MaterialReactTable component with a custom implementation that provides better interaction features and enum support.
- Replaces MaterialReactTable with custom table implementation for better control and performance
- Adds enum popup functionality with search and tooltips for GDC attributes
- Enhances value editing capabilities with inline editing and filtering options
Reviewed Changes
Copilot reviewed 10 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
package.json | Removes Redis kill scripts for development workflow |
app/lib/heatmap/heatmap-helper.tsx | Adds enumDef property to GDCAttribute interface |
app/dashboard/components/value-comparisons/value-comparison-table.tsx | Complete rewrite from MaterialReactTable to custom table with enum support |
app/dashboard/components/fileuploading.tsx | Adds close button to file upload dialog |
app/dashboard/components/explanation/RelevantKnowledgeView.tsx | Adds tooltips for enum values using enumDef |
api/utils.py | Updates GDC ontology file path |
api/tools/task_tools.py | Code formatting improvements and cleanup |
api/matching_task.py | Code formatting improvements |
api/langchain/memory.py | Code formatting improvements |
api/index.py | Updates GDC ontology file path |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -1,12 +1,12 @@ | |||
import { useMemo, useContext, useEffect } from "react"; | |||
import { useMemo, useContext, useEffect, useState, useCallback, useRef, useLayoutEffect } from "react"; |
Copilot
AI
Aug 31, 2025
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.
[nitpick] The import statement is too long and imports many hooks. Consider grouping related imports or splitting into multiple lines for better readability.
import { useMemo, useContext, useEffect, useState, useCallback, useRef, useLayoutEffect } from "react"; | |
import { | |
useMemo, | |
useContext, | |
useEffect, | |
useState, | |
useCallback, | |
useRef, | |
useLayoutEffect | |
} from "react"; |
Copilot uses AI. Check for mistakes.
|
||
// Build dynamic column keys in display order | ||
const dynamicColumnKeys = useMemo(() => { | ||
if (!rows.length) return [] as string[]; |
Copilot
AI
Aug 31, 2025
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.
Potential runtime error if rows[0]
is undefined. The condition !rows.length
is checked above, but there's a race condition where rows could be empty between the check and this line.
Copilot uses AI. Check for mistakes.
}, [candidate]); | ||
|
||
const displayedColumnKeys = useMemo(() => { | ||
if (!rows.length) return [] as string[]; |
Copilot
AI
Aug 31, 2025
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.
Same potential runtime error as above - accessing rows[0]
without ensuring rows is not empty could cause a crash.
Copilot uses AI. Check for mistakes.
max-width: 300px !important; | ||
} | ||
.table-container { overflow: auto; border: 1px solid rgba(0,0,0,0.12); border-radius: 8px; width: 100%; } | ||
table { border-collapse: separate; border-spacing: 0; width: max-content; min-width: 100%; table-layout: fixed; font-size: 0.92rem; } |
Copilot
AI
Aug 31, 2025
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.
[nitpick] Inline CSS styles should be moved to a separate CSS file or styled components for better maintainability and reusability.
Copilot uses AI. Check for mistakes.
anchorOrigin={{ vertical: 'top', horizontal: 'center' }} | ||
transformOrigin={{ vertical: 'bottom', horizontal: 'center' }} |
Copilot
AI
Aug 31, 2025
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.
[nitpick] String literals should use consistent quote style. The component uses double quotes elsewhere but single quotes here.
anchorOrigin={{ vertical: 'top', horizontal: 'center' }} | |
transformOrigin={{ vertical: 'bottom', horizontal: 'center' }} | |
transformOrigin={{ vertical: "bottom", horizontal: "center" }} |
Copilot uses AI. Check for mistakes.
@roquelopez Here are the new designs made for value matching annotations.
This pull request introduces several improvements and minor fixes across the backend and frontend codebases. The most significant changes include updates to ontology resource paths to use a new JSON file, UI enhancements for better usability and clarity, and minor code cleanups. Below are the most important changes grouped by theme:
Ontology Resource Path Updates
gdc_ontology_flat.json
togdc_ontology.json
inapi/index.py
,api/utils.py
, and removed redundant path definitions fromapi/tools/task_tools.py
. This ensures consistency and uses the updated ontology data. [1] [2] [3]Frontend UI Enhancements
fileuploading.tsx
for improved user experience.RelevantKnowledgeView.tsx
by adding tooltips with definitions and highlighting search matches, improving clarity and interactivity. [1] [2] [3]Chip
,CircularProgress
) withTooltip
for cleaner imports.CloseIcon
to support the new close button functionality.Backend Minor Fixes and Cleanups
memory.py
andmatching_task.py
. [1] [2]task_tools.py
. [1] [2]Build Script Cleanup
package.json
to simplify development and start scripts. [1] [2]