Dataset Table View: Update only single row #2566
Conversation
|
@coderabbitai please review |
|
@stephherbers I'll review the changes in this PR for you. 🧠 Learnings used✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request modifies the evaluation messages workflow to support row-level updates in the dataset messages table. It adds unique Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly Related PRs
Suggested Reviewers
Pre-merge checks✅ Passed checks (5 passed)
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 please review |
|
@stephherbers I'll review the changes in this PR for you. 🧠 Learnings used✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (1)
apps/evaluations/tables.py (1)
354-355: Row ids for HTMX look good; tidy up RUF012 on mutable class attributeThe new
row_attrswith anidper record is exactly what you need for#message-row-{{ message.id }}targeting; behavior-wise this is solid.Ruff’s RUF012 warning here is about having a mutable class attribute (
dict) on the innerMetaclass. If your lint is strict, consider one of:+from typing import ClassVar @@ class DatasetMessagesTable(tables.Table): class Meta: @@ - row_attrs = {**settings.DJANGO_TABLES2_ROW_ATTRS, "id": lambda record: f"message-row-{record.id}"} + row_attrs: ClassVar[dict] = { + **settings.DJANGO_TABLES2_ROW_ATTRS, + "id": lambda record: f"message-row-{record.id}", + }or, if you prefer not to annotate Meta attributes, add a targeted ignore:
row_attrs = {**settings.DJANGO_TABLES2_ROW_ATTRS, "id": lambda record: f"message-row-{record.id}"} # noqa: RUF012
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cypress/screenshots/dashboard.cy.js/Dashboard Application -- Dashboard Reset -- clicks the reset filters button (failed).pngis excluded by!**/*.png
📒 Files selected for processing (3)
apps/evaluations/tables.py(1 hunks)apps/evaluations/views/dataset_views.py(1 hunks)templates/evaluations/edit_message_modal_content.html(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for Python linting and formatting with a line length of 120 characters
Use isort via Ruff for Python import sorting
Files:
apps/evaluations/tables.pyapps/evaluations/views/dataset_views.py
⚙️ CodeRabbit configuration file
**/*.py: Do not review for Python compatibility below 3.13
Files:
apps/evaluations/tables.pyapps/evaluations/views/dataset_views.py
apps/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/*.py: Avoid importing heavy AI/ML libraries (langchain_google_vertexai, langchain_google_genai, langchain_anthropic, langchain_openai, boto3, pandas, numpy) at module level; use lazy imports inside functions instead
Use TYPE_CHECKING guard for type-only imports from heavy AI/ML libraries to avoid module-level import costs
Files:
apps/evaluations/tables.pyapps/evaluations/views/dataset_views.py
apps/*/views{,/**}/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/*/views{,/**}/*.py: Use @login_and_team_required decorator or LoginAndTeamRequiredMixin for all Django views to enforce team-based security
Use @permission_required decorator or PermissionRequiredMixin to enforce permission checks on sensitive views
Use class-based views with appropriate mixins (LoginAndTeamRequiredMixin, PermissionRequiredMixin) instead of function-based views
Files:
apps/evaluations/views/dataset_views.py
**/*.{html,tsx,ts,jsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Use DaisyUI components as the primary UI framework for all frontend components before falling back to TailwindCSS custom styling
Files:
templates/evaluations/edit_message_modal_content.html
**/*.{html,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
All UI components must support both light and dark modes using theme-aware styling
Files:
templates/evaluations/edit_message_modal_content.html
🧬 Code graph analysis (1)
apps/evaluations/tables.py (1)
apps/channels/datamodels.py (1)
id(41-42)
🪛 Ruff (0.14.8)
apps/evaluations/tables.py
354-354: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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
| dataset = message.evaluationdataset_set.first() | ||
| if dataset: | ||
| # Get the full table view to render properly with request context | ||
| dataset_id = dataset.id | ||
| table_view = DatasetMessagesTableView() | ||
| table_view.request = request | ||
| table_view.kwargs = {"dataset_id": dataset_id} | ||
|
|
||
| queryset = table_view.get_queryset() | ||
| table = table_view.table_class(queryset, request=request) | ||
|
|
||
| # Render the full table - HTMX will extract just the updated row via hx-select | ||
| return render(request, "table/single_table.html", {"table": table}) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp/repo && find . -type f -name "*.py" | grep -E "(dataset_views|views)" | head -20Repository: dimagi/open-chat-studio
Length of output: 126
🏁 Script executed:
cd /tmp/repo && rg "class DatasetMessagesTableView" -A 20Repository: dimagi/open-chat-studio
Length of output: 126
🏁 Script executed:
cd /tmp/repo && rg "def update_message" -A 30 apps/evaluations/views/Repository: dimagi/open-chat-studio
Length of output: 126
🏁 Script executed:
cd /tmp/repo && find . -type f -name "*.html" | xargs grep -l "edit_message_modal" | head -5Repository: dimagi/open-chat-studio
Length of output: 126
🏁 Script executed:
cd /tmp/repo && find . -type f -name "*.html" | xargs grep -l "update_url\|update_message" | head -10Repository: dimagi/open-chat-studio
Length of output: 126
🏁 Script executed:
git ls-files | grep -E "dataset_views|views\.py" | head -20Repository: dimagi/open-chat-studio
Length of output: 701
🏁 Script executed:
rg "class DatasetMessagesTableView" -A 20Repository: dimagi/open-chat-studio
Length of output: 1709
🏁 Script executed:
rg "def update_message" -B 5 -A 30 apps/evaluations/views/Repository: dimagi/open-chat-studio
Length of output: 2657
🏁 Script executed:
find . -type f -name "*.html" | xargs grep -l "edit_message_modal" 2>/dev/null | head -5Repository: dimagi/open-chat-studio
Length of output: 123
🏁 Script executed:
rg "def update_message" -A 50 apps/evaluations/views/dataset_views.pyRepository: dimagi/open-chat-studio
Length of output: 1824
🏁 Script executed:
cat -n apps/evaluations/views/dataset_views.py | sed -n '360,420p'Repository: dimagi/open-chat-studio
Length of output: 2541
🏁 Script executed:
cat apps/templates/evaluations/dataset_message_edit_action.htmlRepository: dimagi/open-chat-studio
Length of output: 158
🏁 Script executed:
find . -type f -name "edit_message_modal_content.html"Repository: dimagi/open-chat-studio
Length of output: 122
🏁 Script executed:
cat apps/templates/evaluations/edit_message_modal_content.htmlRepository: dimagi/open-chat-studio
Length of output: 157
🏁 Script executed:
cat ./templates/evaluations/dataset_message_edit_action.htmlRepository: dimagi/open-chat-studio
Length of output: 400
🏁 Script executed:
cat ./templates/evaluations/edit_message_modal_content.htmlRepository: dimagi/open-chat-studio
Length of output: 1533
🏁 Script executed:
rg "edit_message_modal\|update_message" ./templates/evaluations/ -A 5 -B 2Repository: dimagi/open-chat-studio
Length of output: 49
🏁 Script executed:
rg "def add_message_to_dataset" -A 40 apps/evaluations/views/dataset_views.pyRepository: dimagi/open-chat-studio
Length of output: 1742
🏁 Script executed:
rg "def edit_message_modal" -A 20 apps/evaluations/views/dataset_views.pyRepository: dimagi/open-chat-studio
Length of output: 1007
🏁 Script executed:
rg "test.*update_message\|test.*edit.*message" apps/evaluations/tests/ -lRepository: dimagi/open-chat-studio
Length of output: 49
🏁 Script executed:
rg "SingleTableView\|table_pagination" apps/evaluations/views/dataset_views.py -B 2 -A 5Repository: dimagi/open-chat-studio
Length of output: 49
🏁 Script executed:
rg "event.detail.successful" ./templates/ -rRepository: dimagi/open-chat-studio
Length of output: 480
🏁 Script executed:
cat ./templates/evaluations/edit_message_modal_content.html | grep -A 2 "hx-on"Repository: dimagi/open-chat-studio
Length of output: 199
Fix pagination and error handling in update_message flow
The current implementation has two critical issues:
-
Pagination is lost on edit: When
update_messagerenders the table (line 406), it passesrequest=requestto the table constructor, which causes django-tables2 to checkrequest.GETfor a page parameter. Since theupdate_urlconstructed at line 371 has no query string (onlyreverse("evaluations:update_message", args=[team_slug, message_id])), the POST request contains no page param. The table then defaults to page 1, so if a user edited a message on page > 1, that row won't appear in the rendered response, causinghx-select="#message-row-{{ message.id }}"to silently fail. -
Error responses close the modal prematurely: Line 382 returns
status=200for validation errors. The modal template checksevent.detail.successful, which is true for all 2xx responses, so the modal closes even when errors are returned (lines showing the errors briefly). Use a 4xx status code for validation errors so the modal stays open.
Fixes:
- Propagate the current page into
update_urlinedit_message_modal:
update_url = reverse("evaluations:update_message", args=[team_slug, message_id])
+page = request.GET.get("page")
+if page:
+ update_url = f"{update_url}?page={page}"- Return a 4xx status on validation errors:
- status=200,
+ status=400,- Optionally align
add_message_to_dataset(line 326) for consistency by addingrequest=requestto the table constructor to match the update path, though note that the add path avoids the pagination issue by not passingrequestin the first place.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@stephherbers point 2 here seems like it could be valid. Have you tested that this works when there are form errors? An easy for error to try is to remove the 'Human Message' field.
|
@stephherbers I'll review the changes in this PR for you. 🧠 Learnings used✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
snopoke
left a comment
snopoke
left a comment
There was a problem hiding this comment.
@stephherbers this approach looks good.
|
@stephherbers bump |
| hx-target="{% if errors %}#editMessageModal{% else %}#record-{{ message.id }}{% endif %}" | ||
| hx-select="{% if errors %}#editMessageModal{% else %}#record-{{ message.id }}{% endif %}" | ||
| hx-swap="{% if errors %}innerHTML{% else %}outerHTML{% endif %}" | ||
| hx-on::after-request="if(event.detail.successful) document.getElementById('editMessageModal').close()" |
There was a problem hiding this comment.
I'm not sure this will work if the user first submits with an error in the form:
- User clicks edit and modal loads
errorsisFalseso target isrecord-N
- User submits form but there are errors
- View returns the re-rendered form with errors but htmx swaps it into the table row instead of the dialog.
Since the decision to change the target happens on the server side I think you'll need to use the htmx headers instead of these attributes:
- HX-Reswap
- HX-Retarget
- HX-Reselect
- I'm not sure this is necessary now that you're only rendering a single table row.
You can use the django-htmx utilities: https://django-htmx.readthedocs.io/en/latest/http.html#response-modifying-functions
Resolves #2419
When you edit a message in the dataset table, only that specific row now updates instead of reloading the entire table.
Technical Description
Why render the full table when we only need one row?
I initially tried the straightforward approach: render just the single row that changed. But it broke with KeyError: 'request' because django-tables2's TemplateColumn rendering happens in a nested context where the request object isn't accessible when rendering rows manually.
I tried three different ways to fix the context propagation - none worked without fighting django-tables2's internals.
This approach
Use HTMX's hx-select attribute:
Server renders the full table
hx-select="#message-row-123" extracts only the target row from the response
hx-target="#message-row-123" replaces the existing row
HTMX discards the rest
From the user's perspective, only one row updates. From the server's perspective, we use existing working code.
Is it inefficient?
Slightly - we render extra HTML that gets thrown away. But the table is paginated, and this approach has zero maintenance burden compared to rewriting the Actions column or creating custom table rendering.
Docs and Changelog
no