Dataset Table: add new template and highlight logic #2620
Conversation
📝 WalkthroughWalkthroughAdds support for per-row highlighting and linking between evaluation results and dataset rows. DatasetMessagesTable gains a constructor parameter Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @templates/evaluations/dataset_messages_table.html:
- Line 15: The selector using querySelector with `x-data*="selectedMessageRow
=== ${messageId}"` is using a contains match and can produce false positives for
IDs that are substrings; update the selection to use an exact attribute match
instead (e.g., target the full attribute value with `=` rather than `*=`) or,
better, add a stable attribute like `data-message-id` on the row (via
get_row_attrs in apps/evaluations/tables.py, e.g., `"data-message-id":
str(record.id)`) and then change the lookup in the code that defines selectedRow
to use `document.querySelector('tr[data-message-id="' + messageId + '"]')` so
rows are matched exactly by ID (refer to selectedRow, messageId, and the
querySelector call).
🧹 Nitpick comments (2)
apps/evaluations/tables.py (2)
342-348: Well-implemented Alpine.js integration with proper dark mode support.The
get_row_attrsmethod correctly:
- Merges default row attributes from settings
- Adds Alpine.js reactive state with a computed getter for row selection
- Applies conditional Tailwind classes that support both light and dark modes (as per coding guidelines)
The use of
record.idis safe as it's a Django model primary key (integer from the database).Optional refactor suggestion: Consider adding a
data-message-idattribute here to simplify the JavaScript selector logic in the template (see comment on template file).Suggested addition for cleaner selector targeting
def get_row_attrs(self, record): attrs = { **settings.DJANGO_TABLES2_ROW_ATTRS, "x-data": f'{{"get isSelected() {{ return selectedMessageRow === {record.id} }}}}', ":class": "{ 'bg-sky-100/40 dark:bg-sky-950/40': isSelected }", + "data-message-id": str(record.id), } return attrsThis would allow the template to use
document.querySelector(\tr[data-message-id="${messageId}"]`)` for exact matching.
362-362: Minor: row_attrs in Meta is unused due to method override.The
row_attrsdefined in theMetaclass is not used because theget_row_attrs()method takes precedence in Django Tables2. While this doesn't cause any issues, it may cause confusion for future maintainers.Consider removing the unused attribute
class Meta: model = EvaluationMessage fields = ( "source", "human_message_content", "ai_message_content", "context", "history", "participant_data", "session_state", "actions", ) - row_attrs = settings.DJANGO_TABLES2_ROW_ATTRS orderable = False empty_text = "No messages in this dataset yet."Alternatively, add a comment explaining that
get_row_attrs()overrides this setting.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/evaluations/tables.pyapps/evaluations/views/dataset_views.pytemplates/evaluations/dataset_messages_table.html
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{tsx,jsx,html}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{tsx,jsx,html}: Prefer DaisyUI components for UI implementation, with TailwindCSS as fallback for custom styling
All UI components must support both light and dark modes
Files:
templates/evaluations/dataset_messages_table.html
**/*.html
📄 CodeRabbit inference engine (AGENTS.md)
Use Django templates with HTMX for dynamic content and Alpine.js for client-side interactivity
Files:
templates/evaluations/dataset_messages_table.html
**/templates/{**/,}*.html
📄 CodeRabbit inference engine (AGENTS.md)
Template files should extend
web/app/app_base.htmland use{% block %}tags for customization
Files:
templates/evaluations/dataset_messages_table.html
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use Ruff for Python linting and formatting with line length of 120 characters
Files:
apps/evaluations/views/dataset_views.pyapps/evaluations/tables.py
⚙️ CodeRabbit configuration file
**/*.py: Do not review for Python compatibility below 3.13
Flag heavy AI/ML imports at module level (langchain_*, openai, anthropic, google, boto3, pandas, numpy). These should be lazy-loaded inside methods to keep Django startup fast.
Files:
apps/evaluations/views/dataset_views.pyapps/evaluations/tables.py
**/{apps,config}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/{apps,config}/**/*.py: Use lazy imports for heavy AI/ML libraries (langchain_google_vertexai, langchain_google_genai, etc.) to maintain fast Django startup time
UseTYPE_CHECKINGimports for type hints when lazy-loading heavy libraries
Files:
apps/evaluations/views/dataset_views.pyapps/evaluations/tables.py
🧠 Learnings (1)
📚 Learning: 2025-12-19T12:29:01.450Z
Learnt from: CR
Repo: dimagi/open-chat-studio PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T12:29:01.450Z
Learning: Applies to **/*.html : Use Django templates with HTMX for dynamic content and Alpine.js for client-side interactivity
Applied to files:
templates/evaluations/dataset_messages_table.html
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cypress-tests
- GitHub Check: python-tests
🔇 Additional comments (2)
templates/evaluations/dataset_messages_table.html (1)
1-25: Verify template structure: consider extending base template.According to the coding guidelines, template files should extend
web/app/app_base.htmland use{% block %}tags for customization. This template appears to be a partial template used within Django Tables2's rendering context.Please confirm whether this is:
- A partial template rendered by
SingleTableView(current implementation is correct)- A full-page template that should extend
app_base.htmlIf this is a full-page template, it should be restructured to extend the base template. If it's a partial, the current structure is appropriate.
apps/evaluations/views/dataset_views.py (1)
284-284: LGTM - clean integration of custom template.The template name change correctly connects the
DatasetMessagesTableViewto the new custom template that provides message highlighting and auto-scroll functionality.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
|
@coderabbitai please review |
|
@stephherbers, I'll review the changes in this PR. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/evaluations/models.py (1)
343-375: Data loss bug: Multiple evaluators per message results are overwritten instead of accumulated.The refactoring changes from incremental dict updates to replacing the entire row for each result. When a message has multiple evaluators, each iteration creates a new
row_dataOrderedDict and assigns it totable_by_message[result.message.id], overwriting previous evaluator results for the same message.The data model explicitly supports this scenario:
EvaluationConfig.evaluatorsis aManyToManyField, andEvaluationResulthas separate ForeignKeys to both Evaluator and EvaluationMessage. The column naming convention"{key} ({evaluator_name})"also expects to display multiple evaluators' results in the same row.To fix, accumulate evaluator results instead of replacing the entire row. Initialize row data on first encounter of a message, then update with each evaluator's results:
Proposed fix
def get_table_data(self, include_ids: bool = False): results = self.results.select_related("message", "evaluator", "session").order_by("created_at").all() table_by_message = {} for result in results: context_columns = { f"{key}": value for key, value in result.message_context.items() if key != "current_datetime" } # Initialize row data only on first result for this message if result.message.id not in table_by_message: row_data = OrderedDict() row_data["session"] = result.session.external_id if result.session_id else "" row_data["message_id"] = result.message.id row_data["Dataset Input"] = result.input_message row_data["Dataset Output"] = result.output_message row_data["Generated Response"] = result.output.get("generated_response", "") table_by_message[result.message.id] = row_data # Accumulate evaluator results table_by_message[result.message.id].update( {f"{key} ({result.evaluator.name})": value for key, value in result.output.get("result", {}).items()} ) table_by_message[result.message.id].update(context_columns) if result.output.get("error"): table_by_message[result.message.id]["error"] = result.output.get("error") if include_ids is True: table_by_message[result.message.id]["id"] = result.message.id return [{"#": index, **row} for index, row in enumerate(table_by_message.values())]
🧹 Nitpick comments (2)
apps/evaluations/views/evaluation_config_views.py (1)
275-285: Consider using proper URL parameter encoding.While the current implementation works for integer IDs, it's better practice to use Django's URL utilities for query parameter construction. Consider using
urllib.parse.urlencodeor passing parameters through the reverse function if the URL pattern supports it.♻️ Proposed refactor using urlencode
+from urllib.parse import urlencode + def dataset_url_factory(_, __, record, value): if not value: return "#" dataset_id = self.evaluation_run.config.dataset_id message_id = record.get("message_id") url = reverse("evaluations:dataset_edit", args=[self.kwargs["team_slug"], dataset_id]) - return f"{url}?message_id={message_id}" + if message_id: + return f"{url}?{urlencode({'message_id': message_id})}" + return urltemplates/evaluations/dataset_edit.html (1)
59-59: Apply URL encoding to query parameter.While Django auto-escapes HTML content, query parameters in URLs should be properly encoded using the
urlencodefilter to prevent potential issues with special characters.♻️ Proposed fix with urlencode filter
- hx-get="{% url 'evaluations:dataset_messages_table' request.team.slug object.id %}{% if request.GET.message_id %}?message_id={{ request.GET.message_id }}{% endif %}" + hx-get="{% url 'evaluations:dataset_messages_table' request.team.slug object.id %}{% if request.GET.message_id %}?message_id={{ request.GET.message_id|urlencode }}{% endif %}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/evaluations/models.pyapps/evaluations/tables.pyapps/evaluations/views/dataset_views.pyapps/evaluations/views/evaluation_config_views.pytemplates/evaluations/dataset_edit.htmltemplates/evaluations/dataset_messages_table.htmltemplates/evaluations/dataset_messages_table_render.html
🚧 Files skipped from review as they are similar to previous changes (2)
- templates/evaluations/dataset_messages_table.html
- apps/evaluations/tables.py
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{tsx,jsx,html}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{tsx,jsx,html}: Prefer DaisyUI components for UI implementation, with TailwindCSS as fallback for custom styling
All UI components must support both light and dark modes
Files:
templates/evaluations/dataset_edit.htmltemplates/evaluations/dataset_messages_table_render.html
**/*.html
📄 CodeRabbit inference engine (AGENTS.md)
Use Django templates with HTMX for dynamic content and Alpine.js for client-side interactivity
Files:
templates/evaluations/dataset_edit.htmltemplates/evaluations/dataset_messages_table_render.html
**/templates/{**/,}*.html
📄 CodeRabbit inference engine (AGENTS.md)
Template files should extend
web/app/app_base.htmland use{% block %}tags for customization
Files:
templates/evaluations/dataset_edit.htmltemplates/evaluations/dataset_messages_table_render.html
**/{apps,config}/**/models.py
📄 CodeRabbit inference engine (AGENTS.md)
**/{apps,config}/**/models.py: Inherit team-scoped models fromBaseTeamModelwhich automatically includes team FK and audit timestamps
UseVersionsMixinandVersionsObjectManagerMixinfor models requiring version tracking
Use@audit_fieldsdecorator withAuditingManagerfor tracking field changes on important models
Usepublic_id = models.UUIDField(default=uuid.uuid4, unique=True)for external API identifiers
UseBaseModelfor non-team-scoped models when team-scoping is not required
All data must be scoped to teams viaBaseTeamModelwith no cross-team data access without explicit permission
Files:
apps/evaluations/models.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use Ruff for Python linting and formatting with line length of 120 characters
Files:
apps/evaluations/models.pyapps/evaluations/views/evaluation_config_views.pyapps/evaluations/views/dataset_views.py
⚙️ CodeRabbit configuration file
**/*.py: Do not review for Python compatibility below 3.13
Flag heavy AI/ML imports at module level (langchain_*, openai, anthropic, google, boto3, pandas, numpy). These should be lazy-loaded inside methods to keep Django startup fast.
Files:
apps/evaluations/models.pyapps/evaluations/views/evaluation_config_views.pyapps/evaluations/views/dataset_views.py
**/{apps,config}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/{apps,config}/**/*.py: Use lazy imports for heavy AI/ML libraries (langchain_google_vertexai, langchain_google_genai, etc.) to maintain fast Django startup time
UseTYPE_CHECKINGimports for type hints when lazy-loading heavy libraries
Files:
apps/evaluations/models.pyapps/evaluations/views/evaluation_config_views.pyapps/evaluations/views/dataset_views.py
🧠 Learnings (1)
📚 Learning: 2025-12-19T12:29:01.450Z
Learnt from: CR
Repo: dimagi/open-chat-studio PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-19T12:29:01.450Z
Learning: Applies to **/{apps,config}/**/views.py : Filter API queryset responses by team using `MyModel.objects.filter(team=self.request.team)`
Applied to files:
apps/evaluations/views/dataset_views.py
🧬 Code graph analysis (1)
apps/evaluations/views/evaluation_config_views.py (1)
apps/generics/actions.py (2)
chip_action(172-211)url_factory(195-198)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: python-tests
🔇 Additional comments (5)
templates/evaluations/dataset_messages_table_render.html (1)
1-21: LGTM! Highlighting implementation looks solid.The template correctly implements row highlighting with proper dark mode support. The
record.record.idaccess pattern is correct for django-tables2's BoundRow structure, and the conditional classes and ID attributes are applied appropriately.apps/evaluations/views/evaluation_config_views.py (2)
298-306: LGTM! Chip action integration looks correct.The new dataset link action is properly configured with appropriate URL factory, enabled condition, and opens in a new tab as intended. The use of
mark_safefor the static FontAwesome icon is appropriate.
307-309: Good consolidation of related actions.Returning
Nonefor themessage_idcase is the right approach since the message ID is now accessible through the dataset link action in the session column, avoiding redundant column display.apps/evaluations/views/dataset_views.py (2)
298-304: Good input validation for highlight_message_id.The try-except block properly handles invalid or missing
message_idparameters by settinghighlight_message_idtoNone, preventing errors while gracefully degrading when the parameter is absent or malformed.
306-313: LGTM! Table kwargs properly configured.The
get_table_kwargsmethod correctly passeshighlight_message_idto the table constructor with proper validation, enabling the highlighting feature in the table rendering layer.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| "session_state", | ||
| "actions", | ||
| ) | ||
| row_attrs = settings.DJANGO_TABLES2_ROW_ATTRS | ||
| orderable = False | ||
| empty_text = "No messages in this dataset yet." |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| context["highlight_message_id"] = self.get_highlight_message_id() | ||
| return context | ||
|
|
||
| def get_table_kwargs(self): | ||
| kwargs = super().get_table_kwargs() | ||
| kwargs["highlight_message_id"] = self.get_highlight_message_id() | ||
| return kwargs | ||
|
|
||
|
|
||
| @login_and_team_required | ||
| @require_POST |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
stephherbers
changed the title
| self.Meta.row_attrs = { | ||
| **settings.DJANGO_TABLES2_ROW_ATTRS, | ||
| "class": _row_class_factory, | ||
| "data-message-id": lambda record: record.id, |
There was a problem hiding this comment.
You should be able to use the existing id attribute: id="record-{{record.id}}"
| <h2 class="pg-subtitle">{% translate "Dataset Messages" %}</h2> | ||
| <div id="dataset-messages-table" | ||
| hx-get="{% url 'evaluations:dataset_messages_table' request.team.slug object.id %}" | ||
| hx-get="{% url 'evaluations:dataset_messages_table' request.team.slug object.id %}{% if request.GET.message_id %}?message_id={{ request.GET.message_id }}{% endif %}" |
There was a problem hiding this comment.
You can use `{% querystring %} tag: https://docs.djangoproject.com/en/6.0/ref/templates/builtins/#querystring
| hx-get="{% url 'evaluations:dataset_messages_table' request.team.slug object.id %}{% if request.GET.message_id %}?message_id={{ request.GET.message_id }}{% endif %}" | |
| hx-get="{% url 'evaluations:dataset_messages_table' request.team.slug object.id %}{% if request.GET.message_id %}{% querystring %}" |
| **settings.DJANGO_TABLES2_ROW_ATTRS, | ||
| "class": _row_class_factory, | ||
| "data-message-id": lambda record: record.id, | ||
| } |
There was a problem hiding this comment.
This code doesn't need to be in the init method. The _row_class_factor function can also accept a table arg (see https://django-tables2.readthedocs.io/en/latest/pages/column-attributes.html#id1).
def _row_class_factory(table, record):
class_defaults = settings.DJANGO_TABLES2_ROW_ATTRS["class"]
if record.id == table.highlight_message_id:
return f"{class_defaults} bg-yellow-100 dark:bg-yellow-900/20"
return class_defaults
|
@stephherbers bump |
resolves #2580
adds message highlighting feature to DatasetMessagesTable:
Technical Description
rows are highlighted when the URL contains a ?message_id= parameter matching the message ID, and the page automatically scrolls to center the highlighted message on load
Demo
Loom Video
Docs and Changelog