Manage submissions speed #2303
Manage submissions speed #2303
Conversation
📝 WalkthroughWalkthroughSwitched submissions index to DataTables server-side rendering; controller now serves paginated/searchable/sortable JSON and a new formatter method. Client JS manages selection state, delegated events, and dynamic row rendering; view removed server-rendered rows and added a DataTables-driven tbody. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as Browser (manage_submissions.js)
participant Server as Server (SubmissionsController)
participant DB as Database
rect rgb(230, 245, 255)
Note over User,Browser: Page load
User->>Browser: GET /submissions
Browser->>Server: GET /submissions (HTML)
Server->>DB: Query assignments/problems (page scaffold)
DB-->>Server: Return data
Server-->>Browser: Render index view (DataTables config)
Browser->>Browser: Initialize DataTables
end
rect rgb(245, 230, 255)
Note over Browser,Server: DataTables requests rows (server-side)
Browser->>Server: AJAX GET /submissions.json (draw, start, length, search, order)
Server->>DB: Query submissions with filters/sort/limit
DB-->>Server: Return rows
Server->>Server: format_submission_for_datatable per row
Server-->>Browser: JSON response (data, draw, recordsTotal, recordsFiltered)
Browser->>Browser: createdRow + drawCallback -> render checkboxes, actions, selection state
end
rect rgb(230, 255, 235)
Note over User,Browser: Selection flow
User->>Browser: Click row checkbox / select-all
Browser->>Browser: toggleRow / toggleAllRows -> update selectedSubmissions, selectedStudentCids, UI state
Browser->>Browser: updateSelectedCount(), enable/disable batch actions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/assets/javascripts/manage_submissions.js (1)
558-562: Reset selections safely when paging.
selectedSubmissions.map(selectedSubmission => toggleRow(selectedSubmission, false));mutatesselectedSubmissionswhile you iterate it, so at least one previously selected ID survives. After changing pages, destructive operations (delete, excuse, etc.) still include that hidden submission. Iterate over a copy (or just clear the arrays) before toggling to keep selection state accurate.- selectedSubmissions.map(selectedSubmission => toggleRow(selectedSubmission, false)); + [...selectedSubmissions].forEach(selectedSubmission => toggleRow(selectedSubmission, false));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/codeql-analysis.ymlis excluded by!**/*.yml
📒 Files selected for processing (3)
app/assets/javascripts/manage_submissions.js(3 hunks)app/controllers/submissions_controller.rb(1 hunks)app/views/submissions/index.html.erb(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-09-20T20:46:52.775Z
Learnt from: evanyeyeye
Repo: autolab/Autolab PR: 2209
File: app/controllers/autograders_controller.rb:0-0
Timestamp: 2024-09-20T20:46:52.775Z
Learning: In the `AutogradersController`, ensure that the `download_file` method has `action_auth_level :download_file, :instructor` to restrict access to instructors.
Applied to files:
app/controllers/submissions_controller.rb
📚 Learning: 2024-06-10T19:38:42.782Z
Learnt from: jlge
Repo: autolab/Autolab PR: 2011
File: app/controllers/assessments_controller.rb:28-29
Timestamp: 2024-06-10T19:38:42.782Z
Learning: `destroy_no_redirect` in `app/controllers/assessments_controller.rb` is an internal method used within the controller and not a public endpoint. It should not be included in the `before_action :set_assessment` exclusion list.
Applied to files:
app/controllers/submissions_controller.rb
🧬 Code graph analysis (1)
app/controllers/submissions_controller.rb (4)
app/controllers/application_controller.rb (1)
action_auth_level(53-71)app/helpers/application_helper.rb (1)
computed_score(111-118)app/models/submission.rb (1)
final_score(327-335)app/models/assessment_user_datum.rb (1)
final_score(106-109)
| order_direction = params.dig(:order, '0', :dir) || 'desc' | ||
|
|
||
| columns_map = { | ||
| 1 => 'users.email', | ||
| 2 => 'submissions.version', | ||
| 3 => 'calculated_score', | ||
| 4 => 'submissions.created_at', | ||
| } | ||
|
|
||
| order_column = columns_map[order_column_index] || 'submissions.created_at' | ||
| base_query = @assessment.submissions | ||
| .joins(course_user_datum: :user) | ||
| if search_value.present? | ||
| base_query = base_query.where( | ||
| "users.email LIKE :search", | ||
| search: "%#{search_value}%" | ||
| ) | ||
| end | ||
| total_records = @assessment.submissions.count | ||
| filtered_records = base_query.count | ||
| submissions_query = base_query | ||
| .includes(course_user_datum: :user) | ||
| .left_joins(:scores) | ||
| .select('submissions.*') | ||
| .select('COALESCE(SUM(scores.score), 0) as calculated_score') | ||
| .group('submissions.id') | ||
|
|
||
| # Apply sorting | ||
| submissions_query = | ||
| if order_column == 'calculated_score' | ||
| submissions_query.order("calculated_score #{order_direction}") | ||
| else | ||
| submissions_query.order("#{order_column} #{order_direction}") | ||
| end |
There was a problem hiding this comment.
Clamp the sort direction to ASC/DESC.
order_direction is interpolated straight into the ORDER BY clause, so a crafted order[0][dir] parameter lets an attacker inject arbitrary SQL (e.g., desc; DROP TABLE ...). Please whitelist the direction before using it in SQL.
- order_direction = params.dig(:order, '0', :dir) || 'desc'
+ direction_param = params.dig(:order, '0', :dir).to_s.downcase
+ order_direction = %w[asc desc].include?(direction_param) ? direction_param : 'desc'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| order_direction = params.dig(:order, '0', :dir) || 'desc' | |
| columns_map = { | |
| 1 => 'users.email', | |
| 2 => 'submissions.version', | |
| 3 => 'calculated_score', | |
| 4 => 'submissions.created_at', | |
| } | |
| order_column = columns_map[order_column_index] || 'submissions.created_at' | |
| base_query = @assessment.submissions | |
| .joins(course_user_datum: :user) | |
| if search_value.present? | |
| base_query = base_query.where( | |
| "users.email LIKE :search", | |
| search: "%#{search_value}%" | |
| ) | |
| end | |
| total_records = @assessment.submissions.count | |
| filtered_records = base_query.count | |
| submissions_query = base_query | |
| .includes(course_user_datum: :user) | |
| .left_joins(:scores) | |
| .select('submissions.*') | |
| .select('COALESCE(SUM(scores.score), 0) as calculated_score') | |
| .group('submissions.id') | |
| # Apply sorting | |
| submissions_query = | |
| if order_column == 'calculated_score' | |
| submissions_query.order("calculated_score #{order_direction}") | |
| else | |
| submissions_query.order("#{order_column} #{order_direction}") | |
| end | |
| direction_param = params.dig(:order, '0', :dir).to_s.downcase | |
| order_direction = %w[asc desc].include?(direction_param) ? direction_param : 'desc' | |
| columns_map = { | |
| 1 => 'users.email', | |
| 2 => 'submissions.version', | |
| 3 => 'calculated_score', | |
| 4 => 'submissions.created_at', | |
| } | |
| order_column = columns_map[order_column_index] || 'submissions.created_at' | |
| base_query = @assessment.submissions | |
| .joins(course_user_datum: :user) | |
| if search_value.present? | |
| base_query = base_query.where( | |
| "users.email LIKE :search", | |
| search: "%#{search_value}%" | |
| ) | |
| end | |
| total_records = @assessment.submissions.count | |
| filtered_records = base_query.count | |
| submissions_query = base_query | |
| .includes(course_user_datum: :user) | |
| .left_joins(:scores) | |
| .select('submissions.*') | |
| .select('COALESCE(SUM(scores.score), 0) as calculated_score') | |
| .group('submissions.id') | |
| # Apply sorting | |
| submissions_query = | |
| if order_column == 'calculated_score' | |
| submissions_query.order("calculated_score #{order_direction}") | |
| else | |
| submissions_query.order("#{order_column} #{order_direction}") | |
| end |
🤖 Prompt for AI Agents
In app/controllers/submissions_controller.rb around lines 42 to 75,
order_direction is taken directly from params and interpolated into the ORDER BY
clause which allows SQL injection; replace that assignment with a whitelist that
maps the incoming param (case-insensitive) to either "ASC" or "DESC" (defaulting
to "DESC") and use that sanitized value in the order call (or use a conditional
that only uses the order when it matches the allowed values). Ensure you perform
this clamp before any query construction so only "ASC" or "DESC" can ever reach
the SQL string.
KesterTan
requested review from
a team and
KesterTan
and removed request for
a team
dwang3851
force-pushed
the
manage-submissions-speed
branch
from
cfdbab4 to
a6f0c1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/assets/javascripts/manage_submissions.js (2)
527-531: Remove redundantselectedStudentCidsfiltering.Lines 528-530 already conditionally remove
selectedCidfromselectedStudentCidsonly when no other submissions from that student are selected. Line 531 then unconditionally removes it again with_.without(), which is redundant and could incorrectly remove CIDs that should remain selected.Apply this diff:
const hasOtherSelectedSubmissions = selectedSubmissions.some(id => submissions_to_cud[id] === selectedCid); if (!hasOtherSelectedSubmissions) { selectedStudentCids = selectedStudentCids.filter(cid => cid !== selectedCid); } - selectedStudentCids = _.without(selectedStudentCids, selectedCid); }
558-562: Reconsider clearing selections on pagination.This handler clears all selected submissions when the user navigates to a different page. Typically, users expect selections to persist across pages so they can select submissions from multiple pages for batch operations (e.g., regrade, delete, download).
Consider removing this handler to allow selections to persist, or add a clear indicator to users that changing pages will clear their selections.
♻️ Duplicate comments (1)
app/controllers/submissions_controller.rb (1)
42-42: SQL injection vulnerability remains unfixed.The
order_directionparameter is still taken directly from params without validation and later interpolated into SQL (line 84). This allows SQL injection attacks.As noted in the previous review, apply this fix:
- order_direction = params.dig(:order, '0', :dir) || 'desc' + direction_param = params.dig(:order, '0', :dir).to_s.downcase + order_direction = %w[asc desc].include?(direction_param) ? direction_param : 'desc'
🧹 Nitpick comments (1)
app/views/submissions/index.html.erb (1)
14-20: Remove unusedadditional_dataarray.With the migration to DataTables server-side processing, this
additional_dataarray appears unused. The JavaScript now buildssubmissions_to_cuddynamically from the JSON response (seemanage_submissions.jslines 243-248). The server-side loop over@submissionshere no longer serves a purpose.Apply this diff to remove the unused code:
- // additional data for each row - additional_data = [ - <% for s in @submissions do %> - { - "submission-id": "<%= s.id.to_s %>", - }, - <% end %> - ]; -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/assets/javascripts/manage_submissions.js(3 hunks)app/controllers/submissions_controller.rb(1 hunks)app/views/submissions/index.html.erb(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-06-10T19:38:42.782Z
Learnt from: najclark
Repo: autolab/Autolab PR: 2011
File: app/controllers/courses_controller.rb:1316-1320
Timestamp: 2024-06-10T19:38:42.782Z
Learning: The user confirmed the concern about potential security risks when setting `flash[:error]` and `flash[:html_safe]` with user-supplied input, specifically mentioning `course_name` derived from a tar file's contents. It's important to ensure proper sanitization of user-supplied input to prevent XSS vulnerabilities, especially when `flash[:html_safe]` is used, which indicates the content should not be escaped.
Applied to files:
app/controllers/submissions_controller.rb
📚 Learning: 2024-09-20T20:46:52.775Z
Learnt from: evanyeyeye
Repo: autolab/Autolab PR: 2209
File: app/controllers/autograders_controller.rb:0-0
Timestamp: 2024-09-20T20:46:52.775Z
Learning: In the `AutogradersController`, ensure that the `download_file` method has `action_auth_level :download_file, :instructor` to restrict access to instructors.
Applied to files:
app/controllers/submissions_controller.rb
📚 Learning: 2024-06-10T19:38:42.782Z
Learnt from: jlge
Repo: autolab/Autolab PR: 2011
File: app/controllers/assessments_controller.rb:28-29
Timestamp: 2024-06-10T19:38:42.782Z
Learning: `destroy_no_redirect` in `app/controllers/assessments_controller.rb` is an internal method used within the controller and not a public endpoint. It should not be included in the `before_action :set_assessment` exclusion list.
Applied to files:
app/controllers/submissions_controller.rb
🧬 Code graph analysis (1)
app/controllers/submissions_controller.rb (3)
app/models/submission.rb (1)
final_score(327-335)app/controllers/application_controller.rb (1)
action_auth_level(53-71)app/helpers/application_helper.rb (1)
computed_score(111-118)
⏰ 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: test
🔇 Additional comments (6)
app/views/submissions/index.html.erb (1)
120-147: LGTM: Header structure and DataTables placeholder.The streamlined header structure with the select-all checkbox, conditional sort icons, and DataTables-driven tbody placeholder is well-implemented and aligns with the server-side processing migration.
app/controllers/submissions_controller.rb (2)
19-34: LGTM: HTML format initialization.The HTML format branch correctly initializes instance variables and loads a limited set of submissions (100) for the initial page render. The
@submissions_to_cudmapping is efficiently created usingpluckandto_h.
105-137: LGTM: DataTables formatter method.The
format_submission_for_datatablemethod is properly authorized at the:instructorlevel and returns a well-structured array for DataTables consumption. The use of instance variables and helper methods is appropriate.app/assets/javascripts/manage_submissions.js (3)
76-96: LGTM: Delegated event handler.The use of
$(document).on('click', '.score-details', ...)is the correct pattern for handling clicks on dynamically created DataTables rows. This ensures the event handler works for elements rendered after page load.
236-306: LGTM: DataTables server-side configuration.The DataTables initialization with server-side processing is well-implemented:
- AJAX endpoint correctly targets the JSON format
dataSrccallback builds thesubmissions_to_cudmapping from response data- Column definitions properly handle rendering, sorting, and visibility
createdRowanddrawCallbackmaintain selection state across redraws
315-321: LGTM: Select-all checkbox synchronization.The
updateSelectAllCheckboxfunction correctly synchronizes the select-all checkbox state with individual row selections.
| if order_column == 'calculated_score' | ||
| all_submissions = base_query.includes(course_user_datum: :user).to_a | ||
| submissions_with_scores = all_submissions.map do |submission| | ||
| cud = submission.course_user_datum | ||
| [submission, submission.final_score(cud)] | ||
| end | ||
| sorted_submissions = submissions_with_scores.sort_by { |_, score| score } | ||
| sorted_submissions.reverse! if order_direction == 'desc' | ||
| paginated_submissions = sorted_submissions[start, length] || [] | ||
|
|
||
| data = paginated_submissions.map do |submission, score| | ||
| format_submission_for_datatable(submission, score) | ||
| end | ||
| else | ||
| submissions = base_query | ||
| .includes(course_user_datum: :user) | ||
| .order("#{order_column} #{order_direction}") | ||
| .limit(length) | ||
| .offset(start) | ||
|
|
||
| data = submissions.map do |submission| | ||
| cud = submission.course_user_datum | ||
| score = submission.final_score(cud) | ||
| format_submission_for_datatable(submission, score) | ||
| end | ||
| end |
There was a problem hiding this comment.
Consider performance implications of in-memory score sorting.
When sorting by calculated_score, all filtered submissions are loaded into memory, scored, sorted in Ruby, then paginated. For assessments with many submissions matching the search filter, this could consume significant memory and time.
While this is a reasonable tradeoff given that final_score is computed dynamically rather than stored in the database, consider monitoring performance for large datasets or implementing score materialization if this becomes a bottleneck.
🤖 Prompt for AI Agents
In app/controllers/submissions_controller.rb around lines 68-93, the controller
loads all filtered submissions into memory to compute and sort by
calculated_score which can be very expensive; change this to avoid full
in-memory sorting by either (A) materializing/calculating and persisting the
score on submissions (or a denormalized column on course_user_datum) and
updating it when relevant so the DB can order and paginate, or (B) if
materialization is not feasible now, fetch only the paginated IDs first (apply
the same filters and search in SQL with limit/offset), load those submissions,
compute final_score for that page and format — additionally add monitoring
(metrics/timing) around this path to detect when it becomes a bottleneck and
consider adding a background job to backfill scores if you implement
materialization.
Description
This changes implements server-side pagination for the manage submissions page to improve loading times.
Motivation and Context
Previously, it would take about 1.5 seconds for each page of the manage submissions page to load since we would render all of them at once. For example, for a submissions page with 400 submissions, it would take about 6 seconds. This change renders only one page at a time, to take a maximum of 1.5 seconds each time.
How Has This Been Tested?
Tested all of the manage submissions functions locally
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for linting