Introduce editable table for creating score distributions #5723
Conversation
WalkthroughThis update introduces modifications to the database schema and the package dependencies. A new column named Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
app/assets/javascripts/components/input_table.ts (1)
91-121: Remove commented-out condition infirstUpdated.The
firstUpdatedmethod is well-structured, but consider removing the commented-out condition in the resize observer for clarity.- // if (Math.abs(parseInt(this.table.getWidth(2) as string) - this.descriptionColWidth) > 10) {Tools
GitHub Check: codecov/patch
[warning] 91-93: app/assets/javascripts/components/input_table.ts#L91-L93
Added lines #L91 - L93 were not covered by tests
[warning] 116-116: app/assets/javascripts/components/input_table.ts#L116
Added line #L116 was not covered by tests
[warning] 118-118: app/assets/javascripts/components/input_table.ts#L118
Added line #L118 was not covered by tests
bmesuere
added
deploy naos
jorg-vr
added
deploy mestra
There was a problem hiding this comment.
First of all, very nice work!
-
We semi-discussed that clicking on the table would also trigger edit mode. Is there a reason why this isn't implemented?
-
I found an edge-case when entering numbers using
,instead of.. If I enter1,5as a score, I don't get an error that I need to enter a number, but saving also seems to fail silently. I think we should both accept1.5as well as1,5. -
Do we have control over the editor? If so, for the header row and col, maybe try to reduce the font size a little and make it bold to be more in line with the actual table.
-
The Maximum column is centered, but jumps to left aligned when editing. Why not alway align left?
-
The tooltip of the Maximum column mentions steps of 0.25 but I think we allow steps of 0.1?
-
If I remember correctly, you demoed copying entire rows. I can't seem to reproduce this. I can select a row, press ctrl+c and the row gets a dashed line, but pasting doesn't do anything.
-
In the form to add comments to submissions, we didn't give the cancel button an outline. I would also not do that here. This will also increase the visual padding between the two buttons.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
package.json (1)
55-55: Consider bundle size optimization strategies.jspreadsheet-ce is a substantial library (~500KB). To optimize the initial load time:
- Consider using dynamic imports to load the spreadsheet component only when needed:
const Jspreadsheet = await import('jspreadsheet-ce');
- Alternatively, consider lighter alternatives like
handsontable-liteif full Excel-like functionality isn't required.db/schema.rb (1)
442-442: Consider improvements to theordercolumn implementation.The current implementation has several potential issues:
orderis a reserved word in SQL which could cause issues in queries- Column allows NULL values which could affect sorting
- No index is defined which could impact performance when ordering
Consider these improvements:
- Rename the column to avoid SQL reserved word:
- t.integer "order" + t.integer "position", null: false, default: 0
- Add an index to optimize ordering queries:
t.index ["evaluation_exercise_id", "position"], name: "index_score_items_on_evaluation_exercise_id_and_position"🧰 Tools
🪛 rubocop
[convention] 442-442: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
- db/schema.rb (1 hunks)
- package.json (1 hunks)
🧰 Additional context used
🪛 rubocop
db/schema.rb
[convention] 442-442: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
🔇 Additional comments (2)
package.json (2)
55-55: Verify browser compatibility requirements.Version 4.2.1 might have limited browser support. Ensure it meets your browser compatibility requirements, especially for older browsers.
55-55:⚠️ Potential issueConsider upgrading jspreadsheet-ce to the latest version.
The current version 4.2.1 is quite old (released in 2020). The latest version is 4.13.x which includes:
- Security patches
- Performance improvements
- Better browser compatibility
- Bug fixes
Consider upgrading to the latest version:
- "jspreadsheet-ce": "^4.2.1", + "jspreadsheet-ce": "^4.13.0",
We allow steps of 0.01. But when giving grades, the number increase/decrease buttons take steps of 0.25. So It might be advised to also use steps of 0.25 for your max. I am open to removing the from the tooltip, enforcing the 0.25 or simply leaving as is. |
pdawyndt
changed the title
In Firefox everything works as expected In chrome
Update: this is fixed now |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (9)
app/assets/javascripts/score_item.ts (1)
67-71: Consider adding transition animations.The sudden show/hide of elements might be jarring. Consider adding smooth transitions for better UX.
+ // Add CSS classes for transitions + table.classList.add("transition-opacity"); + editBtn.classList.add("transition-opacity"); + form.classList.add("transition-opacity"); + form.addEventListener("cancel", () => { table.classList.remove("d-none"); editBtn.classList.remove("d-none"); form.classList.add("d-none"); });Add these CSS classes to your stylesheets:
.transition-opacity { transition: opacity 0.2s ease-in-out; } .d-none { opacity: 0; pointer-events: none; }app/assets/stylesheets/theme/jspreadsheet.css.scss (2)
197-203: Consider enhancing the disabled state visual feedback.While the current implementation uses muted colors for disabled items, consider adding additional visual cues for better accessibility.
Add a subtle style to make the disabled state more apparent:
.jcontextmenu .jcontextmenu-disabled a { color: var(--d-on-surface-muted); + cursor: not-allowed; + opacity: 0.7; }
1-213: Consider adding documentation for theme customization.Since this implements a custom theme for jspreadsheet-ce, it would be helpful to add documentation comments explaining:
- The available CSS variables and their purposes
- How to customize the theme for different color schemes
- Any accessibility considerations for theme customization
This aligns with the PR objectives mentioning that documentation updates are pending.
Would you like me to help generate the documentation comments?
config/locales/js/en.yml (1)
321-326: Add browser-specific paste instructionsBased on the PR comments, there were issues with copy-paste functionality in Chrome that required workarounds. Consider adding browser-specific instructions to help users.
Consider adding these translations:
jspreadsheet: copy: Copy... deleteSelectedRows: Delete selected rows insertNewRowAfter: Insert new row after insertNewRowBefore: Insert new row before paste: Paste... + paste_help_chrome: In Chrome, right-click to paste or focus an input field before using Ctrl+V + paste_help_firefox: Use Ctrl+V to pasteconfig/locales/js/nl.yml (2)
318-320: LGTM: Warning messages are clear and informative.The warning messages effectively communicate the consequences of user actions. Consider a minor enhancement for the validation message:
- validation_warning: "Alle scoreonderdelen moeten een naam en een maximumscore hebben. De maximumscore moet een getal zijn tussen 0 en 1000." + validation_warning: "Alle scoreonderdelen moeten een naam en een geldige maximumscore hebben. De maximumscore moet een getal zijn tussen 0 en 1000."
321-326: Consider adding browser-specific copy-paste instructions.Given the reported copy-paste behavior differences between browsers, consider adding more detailed tooltips:
jspreadsheet: - copy: Kopiëer... + copy: Kopiëer... (Ctrl+C of rechtsklik) - paste: Plak... + paste: Plak... (Ctrl+V of rechtsklik)app/assets/javascripts/components/input_table.ts (3)
155-179: Consider adding unit tests for thevalidatemethodThe
validatemethod contains important validation logic that ensures data integrity. Adding unit tests for this method would help catch potential issues early and maintain code reliability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 155-155: app/assets/javascripts/components/input_table.ts#L155
Added line #L155 was not covered by tests
[warning] 157-158: app/assets/javascripts/components/input_table.ts#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 161-164: app/assets/javascripts/components/input_table.ts#L161-L164
Added lines #L161 - L164 were not covered by tests
[warning] 166-166: app/assets/javascripts/components/input_table.ts#L166
Added line #L166 was not covered by tests
[warning] 171-171: app/assets/javascripts/components/input_table.ts#L171
Added line #L171 was not covered by tests
[warning] 174-175: app/assets/javascripts/components/input_table.ts#L174-L175
Added lines #L174 - L175 were not covered by tests
[warning] 177-178: app/assets/javascripts/components/input_table.ts#L177-L178
Added lines #L177 - L178 were not covered by tests
80-80: Improve handling of decimal separators in themaximumfieldReplacing commas with dots may not be sufficient for all locales or input formats. Consider using localization libraries or more robust parsing methods to handle different decimal formats.
For example, you could use
parseFloatdirectly and handle potentialNaNresults appropriately.
116-121: Simplify configuration by settingallowManualInsertRowandallowManualInsertColumnSince
allowInsertRowandallowInsertColumnare already set totrueandfalserespectively, you may not need to setallowManualInsertRowandallowManualInsertColumnexplicitly, as they might default to the values ofallowInsertRowandallowInsertColumn.Review the necessity of these configurations to simplify the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- app/assets/javascripts/components/input_table.ts (1 hunks)
- app/assets/javascripts/i18n/translations.json (2 hunks)
- app/assets/javascripts/score_item.ts (2 hunks)
- app/assets/javascripts/utilities.ts (1 hunks)
- app/assets/stylesheets/theme/jspreadsheet.css.scss (1 hunks)
- config/locales/js/en.yml (1 hunks)
- config/locales/js/nl.yml (1 hunks)
- test/javascript/utilities.test.js (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/javascript/utilities.test.js
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/assets/javascripts/components/input_table.ts
[warning] 1-8: app/assets/javascripts/components/input_table.ts#L1-L8
Added lines #L1 - L8 were not covered by tests
[warning] 33-33: app/assets/javascripts/components/input_table.ts#L33
Added line #L33 was not covered by tests
[warning] 35-35: app/assets/javascripts/components/input_table.ts#L35
Added line #L35 was not covered by tests
[warning] 37-37: app/assets/javascripts/components/input_table.ts#L37
Added line #L37 was not covered by tests
[warning] 39-39: app/assets/javascripts/components/input_table.ts#L39
Added line #L39 was not covered by tests
[warning] 43-43: app/assets/javascripts/components/input_table.ts#L43
Added line #L43 was not covered by tests
[warning] 45-46: app/assets/javascripts/components/input_table.ts#L45-L46
Added lines #L45 - L46 were not covered by tests
[warning] 49-49: app/assets/javascripts/components/input_table.ts#L49
Added line #L49 was not covered by tests
[warning] 51-51: app/assets/javascripts/components/input_table.ts#L51
Added line #L51 was not covered by tests
[warning] 55-56: app/assets/javascripts/components/input_table.ts#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 59-61: app/assets/javascripts/components/input_table.ts#L59-L61
Added lines #L59 - L61 were not covered by tests
[warning] 72-73: app/assets/javascripts/components/input_table.ts#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 75-76: app/assets/javascripts/components/input_table.ts#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 90-91: app/assets/javascripts/components/input_table.ts#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 100-100: app/assets/javascripts/components/input_table.ts#L100
Added line #L100 was not covered by tests
[warning] 102-102: app/assets/javascripts/components/input_table.ts#L102
Added line #L102 was not covered by tests
[warning] 104-104: app/assets/javascripts/components/input_table.ts#L104
Added line #L104 was not covered by tests
[warning] 134-135: app/assets/javascripts/components/input_table.ts#L134-L135
Added lines #L134 - L135 were not covered by tests
[warning] 137-138: app/assets/javascripts/components/input_table.ts#L137-L138
Added lines #L137 - L138 were not covered by tests
[warning] 144-145: app/assets/javascripts/components/input_table.ts#L144-L145
Added lines #L144 - L145 were not covered by tests
[warning] 149-150: app/assets/javascripts/components/input_table.ts#L149-L150
Added lines #L149 - L150 were not covered by tests
[warning] 152-152: app/assets/javascripts/components/input_table.ts#L152
Added line #L152 was not covered by tests
[warning] 155-155: app/assets/javascripts/components/input_table.ts#L155
Added line #L155 was not covered by tests
[warning] 157-158: app/assets/javascripts/components/input_table.ts#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 161-164: app/assets/javascripts/components/input_table.ts#L161-L164
Added lines #L161 - L164 were not covered by tests
[warning] 166-166: app/assets/javascripts/components/input_table.ts#L166
Added line #L166 was not covered by tests
[warning] 171-171: app/assets/javascripts/components/input_table.ts#L171
Added line #L171 was not covered by tests
[warning] 174-175: app/assets/javascripts/components/input_table.ts#L174-L175
Added lines #L174 - L175 were not covered by tests
[warning] 177-178: app/assets/javascripts/components/input_table.ts#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 181-184: app/assets/javascripts/components/input_table.ts#L181-L184
Added lines #L181 - L184 were not covered by tests
🪛 Biome
app/assets/javascripts/components/input_table.ts
[error] 218-218: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
🔇 Additional comments (18)
app/assets/javascripts/score_item.ts (2)
3-3: LGTM: Import statement is correctly structured.The import of ScoreItemInputTable aligns with the PR objective of introducing spreadsheet functionality.
49-50: Verify the removal of item visibility functionality.The removal of
initItemVisibilityCheckboxessuggests that item visibility controls have been moved to the new table editor. Let's verify this assumption.✅ Verification successful
Based on the search results, I can now generate a final response:
The removal of item visibility functionality is part of a deliberate refactoring
The visibility controls have been consolidated into the new table-based UI implementation. This is evidenced by:
- The presence of visibility column in
input_table.ts:{ type: "checkbox", title: i18n.t("js.score_items.visible")...}- The remaining
initTotalVisibilityCheckboxesfunction handles only total score visibility through a common checkbox initialization pattern- No references to the old
initItemVisibilityCheckboxesfunction exist in the codebase🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to item visibility functionality rg -l "item.*visibility" --type ts # Search for visibility controls in the new table editor ast-grep --pattern 'class ScoreItemInputTable { $$$ visibility($$$) { $$$ } $$$ }'Length of output: 133
Script:
#!/bin/bash # Let's try a broader search for visibility-related code rg "visibility" --type ts -A 5 # Also search for any references to the old function name rg "initItemVisibilityCheckboxes" --type ts # Look for checkbox-related code in table components rg "checkbox" --type ts -A 5 # Search for potential table editor files fd "table.*\.(ts|tsx)$"Length of output: 27599
app/assets/stylesheets/theme/jspreadsheet.css.scss (8)
1-15: LGTM! Good use of CSS variables for theming.The root variable definition and container styles are well-structured and maintain consistency with the theme system.
30-48: LGTM! Well-structured header styles with clear state management.The header styles effectively handle different states (default, dragging, selected) while maintaining consistent typography.
69-71: Verify color contrast for readonly cells.The readonly state uses 30% opacity (
rgba(var(--d-on-surface-rgb), 0.3)). Please ensure this meets WCAG color contrast requirements for accessibility.Consider using a CSS custom property for the opacity value to make it easier to adjust if needed:
- color: rgba(var(--d-on-surface-rgb), 0.3); + --readonly-opacity: 0.3; + color: rgba(var(--d-on-surface-rgb), var(--readonly-opacity));
81-87: LGTM! Consistent footer styling.The footer styles maintain visual consistency with other table sections.
89-137: LGTM! Clear visual feedback for cell interactions.The highlight and selection styles effectively communicate the current state to users while maintaining visual consistency.
139-150: LGTM! Good editor visibility management.The editor styles ensure good visibility and usability of the rich text interface.
152-170: LGTM! Clear visual feedback for errors and copying.The styles effectively communicate error states and copy operations to users.
26-28: Consider the accessibility impact of disabling text selection.Preventing text selection (
::selection { background-color: transparent; }) might impact accessibility for users who rely on text selection for various purposes, such as using screen readers or copying content.Let's check if this is a requirement from the jspreadsheet library:
✅ Verification successful
The previous search was too specific. Let's broaden our search to understand the context better and see if there's any documentation or code that explains why text selection is being disabled.
I want to verify if this selection behavior is part of the jspreadsheet library's default configuration or if it's a custom override. The previous results show other selection styles in the codebase that don't disable selection, suggesting this might be specific to the spreadsheet component.
Disabling text selection is consistent with spreadsheet functionality
The transparent selection style is intentional and appropriate in this context. The jspreadsheet configuration shows that this is a spreadsheet-like interface where:
- Custom selection behavior is managed by the library (
selectionCopy: false)- It's an input table component with editable cells
- The library provides its own selection mechanisms for spreadsheet operations
The disabled text selection doesn't impact accessibility as users can still:
- Edit cell contents normally
- Use keyboard navigation (standard spreadsheet behavior)
- Copy/paste through the spreadsheet's built-in mechanisms (evidenced by copy/paste i18n strings)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar selection styles in the jspreadsheet source rg -i "::selection.*transparent"Length of output: 34
Script:
#!/bin/bash # Let's search for any selection-related styles in the codebase rg -i "::selection" -A 2 # Also search for any comments or documentation about selection behavior rg -i "selection|selectable" --type md rg -i "selection|selectable" --type txt # Check if there are any JavaScript files that might control selection behavior rg -i "\.select|selectable|selection" --type js -A 2Length of output: 2302
Script:
#!/bin/bash # Let's specifically look for jspreadsheet/jexcel related files and configurations rg -i "jspreadsheet|jexcel" --type js -A 3 # Also search for any documentation about jspreadsheet configuration fd -e md -e txt -H -I | xargs rg -i "jspreadsheet|jexcel" # Look for any JavaScript code that might initialize or configure jspreadsheet ast-grep --pattern 'new Jspreadsheet($$$)' ast-grep --pattern 'jspreadsheet($$$)'Length of output: 2837
app/assets/javascripts/utilities.ts (1)
272-272: LGTM! The export addition is well-aligned with the PR objectives.The
replaceHTMLExecuteScriptsfunction is a necessary export for supporting dynamic content updates in the new table editor implementation. The function is well-implemented with proper security considerations using DOMParser.Let's verify the usage of this newly exported function:
✅ Verification successful
The exported function is correctly used in view templates for dynamic content updates
The verification shows that
replaceHTMLExecuteScriptsis:
- Imported in the main application pack
- Used in score-related views (
scores/show.js.erb) for updating score form wrappers- Used in series views (
series/show.js.erb) for updating series cards- Implementation in
utilities.tsconfirms secure DOM manipulation using DOMParser🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of replaceHTMLExecuteScripts in the codebase # Expected: Usage in table-related components, particularly with jspreadsheet-ce integration # Search for imports of replaceHTMLExecuteScripts echo "Checking imports:" rg "import.*replaceHTMLExecuteScripts.*from.*utilities" -A 2 # Search for usage of the function echo "Checking usage:" rg "replaceHTMLExecuteScripts\(" -A 2Length of output: 1464
config/locales/js/en.yml (2)
309-312: LGTM! Basic field labels are clear and concise.
319-320: LGTM! Standard action button labels.config/locales/js/nl.yml (2)
309-312: LGTM: Core field translations are accurate and complete.The Dutch translations for the basic score item fields are clear and appropriate.
316-317: LGTM: Action button translations are consistent.The translations for save and cancel actions align with existing patterns in the file.
app/assets/javascripts/i18n/translations.json (2)
366-386: LGTM! Translations are consistent and completeThe translations between English and Dutch are well-maintained, with appropriate cultural adaptations while preserving the intended meaning.
Also applies to: 892-912
371-377: LGTM! Translations support all required featuresThe translations properly support:
- Copy/paste functionality for score items
- Row management (insert/delete)
- All necessary UI elements for the jspreadsheet integration
Also applies to: 897-903
app/assets/javascripts/components/input_table.ts (1)
198-220: Avoid usingevaldue to security and performance concernsUsing
evalcan introduce security risks and performance issues. It's recommended to refactor the code to avoid usingevalby directly handling the response or using safer alternatives.🧰 Tools
🪛 Biome
[error] 218-218: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
| editBtn.addEventListener("click", activateEditMode); | ||
| table.addEventListener("click", activateEditMode); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve click target specificity and accessibility.
The current implementation attaches a click handler to the entire table, which might interfere with other interactive elements within the table. Additionally, keyboard accessibility should be considered.
- editBtn.addEventListener("click", activateEditMode);
- table.addEventListener("click", activateEditMode);
+ // Add role and keyboard support
+ editBtn.setAttribute("role", "button");
+ editBtn.setAttribute("tabindex", "0");
+ editBtn.addEventListener("click", activateEditMode);
+ editBtn.addEventListener("keydown", (e: KeyboardEvent) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ activateEditMode(e as unknown as MouseEvent);
+ }
+ });
+
+ // Add edit button to table header instead of entire table
+ const tableHeader = table.querySelector("thead");
+ if (tableHeader) {
+ tableHeader.addEventListener("click", activateEditMode);
+ }📝 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.
| editBtn.addEventListener("click", activateEditMode); | |
| table.addEventListener("click", activateEditMode); | |
| // Add role and keyboard support | |
| editBtn.setAttribute("role", "button"); | |
| editBtn.setAttribute("tabindex", "0"); | |
| editBtn.addEventListener("click", activateEditMode); | |
| editBtn.addEventListener("keydown", (e: KeyboardEvent) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| activateEditMode(e as unknown as MouseEvent); | |
| } | |
| }); | |
| // Add edit button to table header instead of entire table | |
| const tableHeader = table.querySelector("thead"); | |
| if (tableHeader) { | |
| tableHeader.addEventListener("click", activateEditMode); | |
| } |
| modified_warning: You have changed the maximum score of one or more score items. This will mark all completed evaluations with this score item as uncompleted. | ||
| deleted_warning: You have deleted one or more score items. This will also delete all scores for these items. | ||
| validation_warning: All score items must have a name and a maximum score, and the maximum score must be between 0 and 1000. |
There was a problem hiding this comment.
Add warning message for reordering score items
The PR objectives mention that reordering score items after some solutions have been evaluated may require reevaluation. This important information should be communicated to users through the UI.
Consider adding a new warning message:
modified_warning: You have changed the maximum score of one or more score items. This will mark all completed evaluations with this score item as uncompleted.
deleted_warning: You have deleted one or more score items. This will also delete all scores for these items.
validation_warning: All score items must have a name and a maximum score, and the maximum score must be between 0 and 1000.
+ reorder_warning: Reordering score items after some solutions have been evaluated may require those solutions to be reevaluated.📝 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.
| modified_warning: You have changed the maximum score of one or more score items. This will mark all completed evaluations with this score item as uncompleted. | |
| deleted_warning: You have deleted one or more score items. This will also delete all scores for these items. | |
| validation_warning: All score items must have a name and a maximum score, and the maximum score must be between 0 and 1000. | |
| modified_warning: You have changed the maximum score of one or more score items. This will mark all completed evaluations with this score item as uncompleted. | |
| deleted_warning: You have deleted one or more score items. This will also delete all scores for these items. | |
| validation_warning: All score items must have a name and a maximum score, and the maximum score must be between 0 and 1000. | |
| reorder_warning: Reordering score items after some solutions have been evaluated may require those solutions to be reevaluated. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/assets/javascripts/components/input_table.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/assets/javascripts/components/input_table.ts
[warning] 1-8: app/assets/javascripts/components/input_table.ts#L1-L8
Added lines #L1 - L8 were not covered by tests
[warning] 33-33: app/assets/javascripts/components/input_table.ts#L33
Added line #L33 was not covered by tests
[warning] 35-35: app/assets/javascripts/components/input_table.ts#L35
Added line #L35 was not covered by tests
[warning] 37-37: app/assets/javascripts/components/input_table.ts#L37
Added line #L37 was not covered by tests
[warning] 39-39: app/assets/javascripts/components/input_table.ts#L39
Added line #L39 was not covered by tests
[warning] 43-43: app/assets/javascripts/components/input_table.ts#L43
Added line #L43 was not covered by tests
[warning] 45-46: app/assets/javascripts/components/input_table.ts#L45-L46
Added lines #L45 - L46 were not covered by tests
[warning] 49-49: app/assets/javascripts/components/input_table.ts#L49
Added line #L49 was not covered by tests
[warning] 51-51: app/assets/javascripts/components/input_table.ts#L51
Added line #L51 was not covered by tests
[warning] 55-56: app/assets/javascripts/components/input_table.ts#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 59-61: app/assets/javascripts/components/input_table.ts#L59-L61
Added lines #L59 - L61 were not covered by tests
[warning] 72-73: app/assets/javascripts/components/input_table.ts#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 75-76: app/assets/javascripts/components/input_table.ts#L75-L76
Added lines #L75 - L76 were not covered by tests
[warning] 90-91: app/assets/javascripts/components/input_table.ts#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 100-100: app/assets/javascripts/components/input_table.ts#L100
Added line #L100 was not covered by tests
[warning] 102-102: app/assets/javascripts/components/input_table.ts#L102
Added line #L102 was not covered by tests
[warning] 104-104: app/assets/javascripts/components/input_table.ts#L104
Added line #L104 was not covered by tests
[warning] 134-135: app/assets/javascripts/components/input_table.ts#L134-L135
Added lines #L134 - L135 were not covered by tests
[warning] 137-138: app/assets/javascripts/components/input_table.ts#L137-L138
Added lines #L137 - L138 were not covered by tests
[warning] 144-145: app/assets/javascripts/components/input_table.ts#L144-L145
Added lines #L144 - L145 were not covered by tests
[warning] 149-150: app/assets/javascripts/components/input_table.ts#L149-L150
Added lines #L149 - L150 were not covered by tests
[warning] 152-152: app/assets/javascripts/components/input_table.ts#L152
Added line #L152 was not covered by tests
[warning] 155-155: app/assets/javascripts/components/input_table.ts#L155
Added line #L155 was not covered by tests
[warning] 157-158: app/assets/javascripts/components/input_table.ts#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 161-164: app/assets/javascripts/components/input_table.ts#L161-L164
Added lines #L161 - L164 were not covered by tests
[warning] 166-166: app/assets/javascripts/components/input_table.ts#L166
Added line #L166 was not covered by tests
[warning] 171-171: app/assets/javascripts/components/input_table.ts#L171
Added line #L171 was not covered by tests
[warning] 174-175: app/assets/javascripts/components/input_table.ts#L174-L175
Added lines #L174 - L175 were not covered by tests
[warning] 177-178: app/assets/javascripts/components/input_table.ts#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 181-184: app/assets/javascripts/components/input_table.ts#L181-L184
Added lines #L181 - L184 were not covered by tests
🪛 Biome
app/assets/javascripts/components/input_table.ts
[error] 218-218: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
🔇 Additional comments (1)
app/assets/javascripts/components/input_table.ts (1)
198-220:⚠️ Potential issueHandle non-OK HTTP responses in the
savemethodCurrently, the
savemethod only handles successful responses (response.ok). It's advisable to handle non-OK responses to provide feedback to the user in case of errors, such as network issues or server errors.Add error handling for failed requests:
if (response.ok) { const js = await response.text(); eval(js); +} else { + const errorMessage = await response.text(); + alert(`Save failed: ${errorMessage}`); }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 218-218: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
| this.columnConfig.forEach((column, index) => { | ||
| const td = this.tableRef.value.querySelector(`thead td[data-x="${index}"]`); | ||
| if (td && column.tooltip) { | ||
| td.setAttribute("title", column.tooltip); | ||
| new Tooltip(td); | ||
| } |
There was a problem hiding this comment.
Dispose of Tooltip instances to prevent memory leaks
The Tooltip instances created for column headers are not disposed of when the component is disconnected. This may lead to memory leaks. Consider storing references to the tooltips and disposing of them in the disconnectedCallback lifecycle method.
To address this, modify the code as follows:
this.columnConfig.forEach((column, index) => {
const td = this.tableRef.value.querySelector(`thead td[data-x="${index}"]`);
if (td && column.tooltip) {
td.setAttribute("title", column.tooltip);
- new Tooltip(td);
+ const tooltip = new Tooltip(td);
+ this.tooltips.push(tooltip);
}
});
+
+disconnectedCallback(): void {
+ super.disconnectedCallback();
+ this.tooltips.forEach(tooltip => tooltip.dispose());
+ this.tooltips = [];
+}And initialize tooltips in your class:
export class ScoreItemInputTable extends DodonaElement {
+ tooltips: Tooltip[] = [];Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 134-135: app/assets/javascripts/components/input_table.ts#L134-L135
Added lines #L134 - L135 were not covered by tests
[warning] 137-138: app/assets/javascripts/components/input_table.ts#L137-L138
Added lines #L137 - L138 were not covered by tests
| async save(): Promise<void> { | ||
| if (!this.validate()) { | ||
| return; | ||
| } | ||
|
|
||
| if (!this.confirmWarnings()) { | ||
| return; | ||
| } | ||
|
|
||
| const response = await fetch(this.route, { | ||
| method: "PATCH", | ||
| headers: { | ||
| "Content-Type": "application/json" | ||
| }, | ||
| body: JSON.stringify({ | ||
| score_items: this.editedScoreItems | ||
| }) | ||
| }); | ||
| if (response.ok) { | ||
| const js = await response.text(); | ||
| eval(js); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid using eval for executing response scripts
Using eval can introduce security risks and performance issues. Consider refactoring the code to avoid using eval, possibly by processing the JSON response directly or using a safer alternative.
Refactor the save method:
if (response.ok) {
- const js = await response.text();
- eval(js);
+ const data = await response.json();
+ // Process the data as needed
}Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Biome
[error] 218-218: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
This pull request introduces jspreadsheet-ce as table editor to manage score items for an evaluation.
This way, copying, reusing and even initially filling it out all go a lot faster. The excel like layout should be easy to interact with for most users.
It also allows to prepare score items in advance in a spreadsheet program and pasting it here directly.
Reordering is partially supported. Doing when some solutions are already evaluated will force you to redo those evaluations.
test.mp4
Darkmode is also supported
Closes #4940 and closes #2713