-
Notifications
You must be signed in to change notification settings - Fork 61
fix: scope job chat sessions by step #3841
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: main
Are you sure you want to change the base?
Conversation
7cbb484 to
51ea011
Compare
51ea011 to
06b76a2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3841 +/- ##
==========================================
+ Coverage 88.84% 88.85% +0.01%
==========================================
Files 422 422
Lines 19116 19136 +20
==========================================
+ Hits 16984 17004 +20
Misses 2132 2132 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
midigofrank
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.
Nicely done
Refactor get_session implementation to use shared query functions and
follow idiomatic Ecto patterns established elsewhere in the codebase:
- Add session_query/2 helper for building queries with optional job scoping
- Add preload_session/1 helper to centralize preloading logic
- Change get_session/1 to return nil instead of {:error, :not_found} tuple
- Add get_session/2 for job-scoped retrieval (returns nil if not found)
- Add get_session!/2 for job-scoped retrieval (raises if not found)
- Refactor get_session!/1 to use shared helpers
This eliminates code duplication, uses single database queries with
efficient preloading, and follows the pattern established by functions
like get_project_user/2 and get_credential_body/2.
Updated callers to handle nil returns instead of error tuples.
|
@elias-ba I think I understand the change, I think this is good to merge. While this change didn't jump out as an issue, the logic around switching the job chat and chat in general makes me nervous about what it's going to take to get this into the new collaborative editor. |
Description
This PR fixes AI chat session job scoping to prevent incorrect chat sessions from appearing when switching between jobs in the workflow editor.
Previously, when a user had a chat session open for Job A and switched to Job B, the chat session from Job A would persist or the wrong session would be shown. This PR implements strict job scoping with three layers of protection:
j-chatparameter when switching jobs (but preserves it on initial page load)Closes #3745
Validation steps
Test URL parameter clearing when switching jobs:
Test session list filtering:
Test URL tampering protection:
?s=job_b_id&j-chat=job_a_session_id)Test persistence on page reload:
Run the test suite:
Additional notes for the reviewer
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
:owner,:admin,:editor,:viewer)