Delegate report character limit - #15403
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents delegate reports from exceeding the database TEXT size limit by adding server-side validation and client-side UI safeguards in the markdown editor, improving both reliability and user feedback.
Changes:
- Add
DelegateReport::MAX_SECTION_LENGTHand validate report section size before saving. - Add a live counter + warning styling and prevent typing/pasting past the limit in the markdown editor.
- Tighten ESLint configuration by enabling the
importplugin and enforcingimport/order.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
app/models/delegate_report.rb |
Introduces the size constant and validates report sections. |
spec/models/delegate_report_spec.rb |
Adds coverage for the new section length validation. |
app/views/delegate_reports/edit.html.erb |
Passes the section limit to the markdown editor via data-max-length. |
app/webpacker/lib/markdown-editor.js |
Shows a live count and blocks edits that exceed the limit. |
app/webpacker/stylesheets/markdown-editor.scss |
Styles the warning state for the character counter. |
.eslintrc.json |
Adds import plugin and import/order rule. |
Suppressed comments (2)
app/models/delegate_report.rb:82
MAX_SECTION_LENGTHis used as a character limit, but the underlying MySQLTEXTlimit is in bytes (and the schema shows this table usesutf8mb4). With multibyte characters (e.g., emoji),lengthvalidation can pass while the DB still rejects the value, reintroducing the original unhandled DB error.
validates :wrc_incidents, presence: true, if: :wrc_feedback_requested
validates :wic_incidents, presence: true, if: :wic_feedback_requested
validates(*AVAILABLE_SECTIONS, length: { maximum: MAX_SECTION_LENGTH })
app/webpacker/lib/markdown-editor.js:170
- The input cap logic also uses
.length, so it can still allow text that exceeds MySQL’sTEXTbyte limit when users enter multibyte characters. This can defeat the purpose of preventing DB-level errors.
const currentLength = instance.getValue().length;
const removedLength = instance.getRange(changeObj.from, changeObj.to).length;
const addedLength = changeObj.text.join('\n').length;
const newLength = currentLength - removedLength + addedLength;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let editor; | ||
| const status = ['upload-image']; | ||
| if (maxLength) { | ||
| status.push({ | ||
| className: 'markdown-editor-character-count', | ||
| defaultValue: (el) => { | ||
| const target = el; | ||
| target.innerHTML = `${this.value.length} / ${maxLength} characters`; | ||
| }, | ||
| onUpdate: (el) => { | ||
| const target = el; | ||
| const { length } = editor.value(); | ||
| target.innerHTML = `${length} / ${maxLength} characters`; | ||
| target.classList.toggle('markdown-editor-character-count-warning', length >= maxLength * 0.9); | ||
| }, | ||
| }); | ||
| } |
| it "rejects sections longer than the database character limit" do | ||
| dr = build(:delegate_report, remarks: "a" * DelegateReport::MAX_SECTION_LENGTH) | ||
| expect(dr).to be_valid | ||
|
|
||
| dr.remarks = "a" * (DelegateReport::MAX_SECTION_LENGTH + 1) | ||
| expect(dr).to be_invalid_with_errors remarks: ["is too long (maximum is 65535 characters)"] | ||
| end |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| '|', ...helps, | ||
| ]; | ||
| const editor = new EasyMDE({ | ||
| let editor; |
There was a problem hiding this comment.
We don't really like let in our codebase. Please try to rework this into immutable code using const
| let editor; | ||
| const status = ['upload-image']; | ||
| if (maxLength) { | ||
| status.push({ |
There was a problem hiding this comment.
Same as above: Mutating objects should really only happen as a super-duper last resort exception in our code. And this PR is not one of those exceptions :D
| $(this).trigger('change'); | ||
| }); | ||
|
|
||
| if (maxLength) { |
There was a problem hiding this comment.
I am curious: Have you tried looking at the documentation of the NPM package we're using to find out whether they natively support a character limit?
There was a problem hiding this comment.
This feels like a pretty common-place feature that (just by gut feeling, I didn't actually check) you shouldn't have to code yourself.
There was a problem hiding this comment.
Again, unrelated changes. Please discard them or pull them out into a separate PR
…de, remove unrelated ESLint changes - Reduce MAX_SECTION_LENGTH to 16,383 to safely fit within MySQL TEXT's 65,535 byte limit with UTF-8MB4 (4 bytes/char max) - Replace let declarations with const throughout markdown-editor.js - Avoid object mutations: refactor uploadsAndInserts and status array creation to use immutable patterns - Remove unrelated import plugin and import/order rule from .eslintrc.json - Fixes feedback from both Copilot and gregorbg reviews
no-param-reassign and no-use-before-define were triggered by the mutation-avoidance refactor. Restore the const target = el alias and hoisted let editor declaration needed for the status bar closures.
The expected error message still referenced the old 65535-character limit after MAX_SECTION_LENGTH was reduced to 16383 to account for UTF-8MB4 byte width. Interpolate the constant instead of hardcoding it.
|
@gregorbg Should be fixed now. Thank you so much for the feedback and moving forward I'll try to get it right the first time |
Fixes #15310
Delegate report sections (summary, equipment, venue, organization, incidents, remarks) are stored in TEXT columns, which have a hard 65,535-character database limit. Previously, exceeding it caused an unhandled DB error and a generic "Something went wrong" message with no indication of what went wrong.
This adds a Rails validation enforcing the limit so users get a real error message, plus a live character counter and input cap in the report editor so they can't hit the limit in the first place.