SAK-52090 Gradebook add filter for gb items #14292
Conversation
![accesslint[bot]](https://avatars.githubusercontent.com/in/1502?s=60&v=4){kind=small}
| <li wicket:id="itemFilter" class="me-2 mb-1"> | ||
| <div class="gb-student-filter gb-item-text-filter"> | ||
| <label for="itemFilterInput" class="sr-only"><wicket:message key="filter.items"/></label> | ||
| <input type="text" id="itemFilterInput" class="form-control" aria-controls="gradeTable" /> |
There was a problem hiding this comment.
Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.
WalkthroughAdds an item text filter UI and translations, styles the filter and clear button, integrates an itemFilter WebMarkupContainer and refined focus parameter handling server-side, and implements client-side column item filtering plus horizontal scroll persistence and focus/edit flow adjustments. Changes
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js (1)
3215-3257: Fix Bootstrap 5 popover hide logic and prevent duplicate event handler binding.The popover hiding selector uses
.popover.in(Bootstrap 3 class) which doesn't exist in Bootstrap 5.2.0; BS5 uses.popover.show. Additionally,$('body, button').on(...)without an event namespace will bind duplicate handlers each time this function is called, causing multiple unnecessary hide operations.- $('body, button').on('click keyup touchend', function (e) { - if ($(e.target).data("bs-toggle") !== 'popover' - && $(e.target).parents('.popover.in').length === 0) { - document.querySelectorAll('[data-bs-toggle="popover"]').forEach(el => { - bootstrap.Popover.getInstance(el)?.hide(); - }); - } - }) + $(document) + .off('click.gbNewItemPopover keyup.gbNewItemPopover touchend.gbNewItemPopover') + .on('click.gbNewItemPopover keyup.gbNewItemPopover touchend.gbNewItemPopover', function (e) { + const clickedToggle = $(e.target).data("bs-toggle") === "popover"; + const clickedInsidePopover = $(e.target).closest(".popover").length > 0; + if (!clickedToggle && !clickedInsidePopover) { + document.querySelectorAll('[data-bs-toggle="popover"]').forEach(el => { + const instance = bootstrap.Popover.getInstance(el); + instance && instance.hide(); + }); + } + });
♻️ Duplicate comments (1)
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.html (1)
115-123: A11y + conventions: clear control semantics, placeholder, and kebab-case ids.
- The clear “button” is an
<a href="javascript:void(0)">with onlytitle; prefer a real<button type="button">+aria-label.- Consider adding
wicket:message="placeholder:filter.items"so the input has a placeholder even before JS runs.- New
id="itemFilterInput"is camelCase; as per coding guidelines, prefer kebab-case for ids/classes.- <input type="text" id="itemFilterInput" class="form-control" aria-controls="gradeTable" /> - <a class="gb-item-filter-clear-button" wicket:message="title:filter.itemsclear" href="javascript:void(0)"> + <input type="text" id="item-filter-input" class="form-control" aria-controls="gradeTable" wicket:message="placeholder:filter.items" /> + <button type="button" class="gb-item-filter-clear-button" wicket:message="title:filter.itemsclear,aria-label:filter.itemsclear"> <i class="fa fa-times-circle" aria-hidden="true"></i> - </a> + </button>
🧹 Nitpick comments (5)
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.html (1)
8-30: Prefer moving new CSS into the tool stylesheet (and/or Bootstrap utilities).
Inline<style>blocks make overrides harder to track; consider relocating to the existing GradebookNG CSS and leaning on Bootstrap positioning utilities where possible. As per coding guidelines, leverage Bootstrap 5.2 for consistency.gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js (4)
1-6: Avoid creating new implicit globals for keys/state.
GB_SCROLL_LEFT_KEY = ...andGbGradeTable.itemFilterText = ...are fine conceptually, but considerconstfor keys to prevent accidental reassignment (file already trends toward globals, but this helps). As per coding guidelines, minimize global scope in new work.
196-219: Filtering primitives are solid; consider diacritics-normalization for better matching.
Student filter normalizes accents; item filter currently doesn’t, so “Évaluation” vs “evaluation” won’t match.
933-941: Scroll persistence works; consider scoping the storage key to site/gradebook.
Current key is per-user only, so a stored value could (briefly) bleed across different Gradebook contexts if navigation changes. IncludingsiteId/guidwould make it safer.Also applies to: 1315-1319
1248-1282: Clear action should apply immediately (avoid debounce lag).
Right now clear triggerskeyupand waits for the 300ms debounce; consider callingGbGradeTable.setItemFilterText("")directly after clearing the input value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
gradebookng/bundle/src/main/bundle/gradebookng.properties(1 hunks)gradebookng/bundle/src/main/bundle/gradebookng_fr_FR.properties(1 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.html(2 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.java(2 hunks)gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js(10 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.{html,jsp,vm,xhtml,xml}: Use Bootstrap 5.2 as the preferred UI framework for styling in Sakai frontends
Leverage Bootstrap 5 components for consistent UI/UX
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.html
**/*.{html,jsp,vm,xhtml,xml,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Ensure all UI components work across different screen sizes (Responsive Design)
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.html
**/*.{js,html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Use the web components in the webcomponents/ directory when possible in Sakai frontends
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.htmlgradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
**/*.{html,jsp,jspx,xml,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer kebab-case for values of HTML class and id attributes
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.htmlgradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.java
**/*.js
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.js: Use clean, standard modern JavaScript in Sakai frontends
Update jQuery code to modern JavaScript when making changes, if the changes are minimal
Prefer ES6+ features (arrow functions, template literals, destructuring, etc.) in JavaScript
Write modular, reusable JavaScript components in Sakai frontends
Minimize use of global variables and functions (Avoid Global Scope) in JavaScript
Files:
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Target evergreen browsers; assume ES2022+ features and browser APIs likefetchkeepalive are present; avoid legacy branches, UA sniffing, or fallbacks unless a specific evergreen gap is documented
Replace jQuery with modern DOM APIs when touching code; new work should not add jQuery dependencies
Compose Lit components, ES modules, and encapsulated helpers; keep state local and explicit with modular code
Prefer module scope or class fields; expose intentional APIs instead of incidental globals; avoid global side channels
Files:
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
**/*/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Internal reactive state in Lit components should stay prefixed with
_and is only surfaced through getters/setters when required
Files:
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
📚 Learning: 2025-10-07T15:11:27.298Z
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
Applied to files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.java
🔇 Additional comments (3)
gradebookng/bundle/src/main/bundle/gradebookng.properties (1)
122-123: New i18n keys look good.
Additive keys align with the UI wiring and keep existing filters untouched.gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.java (1)
321-326: Wicket container wiring foritemFilteris clean.
Visibility tied tohasGradebookItemsmatches the markup and avoids empty toolbar chrome.gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js (1)
1938-1977: Good replacement ofcolumn.hiddenchecks withisColumnHidden()(consistency win).
This keeps rendering, data slicing, and summary counts aligned with the new item filter behavior.Also applies to: 2948-2983
gradebookng/bundle/src/main/bundle/gradebookng_fr_FR.properties
Outdated
Show resolved
Hide resolved
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.java (1)
321-326: Consider also gatingitemFilteronhasStudents(UX consistency).Right now
itemFiltershows wheneverhasGradebookItemsis true, even ifhasStudentsis false; if filtering is meaningless without students/rows, consideritemFilter.setVisible(this.hasGradebookItems && this.hasStudents)to avoid a confusing control state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gradebookng/bundle/src/main/bundle/gradebookng_fr_FR.properties(1 hunks)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- gradebookng/bundle/src/main/bundle/gradebookng_fr_FR.properties
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.java
🧠 Learnings (2)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
📚 Learning: 2025-10-07T15:11:27.298Z
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
Applied to files:
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.java
🔇 Additional comments (2)
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.java (2)
683-703: Safer focus-assignment JS emission (XSS hardening) looks good.Parsing
focusAssignmentIdtolongand emitting via%d(only when> 0L) closes the injection path from the query param, and removing both page parameters avoids repeated triggering.
321-326: VerifyitemFilterhas the right markup hooks for client code (id/markup availability).If any JS/CSS/Ajax expects to select
#itemFilter(or similar), ensure the rendered HTML has a stableid(via explicitidinGradebookPage.htmlorsetOutputMarkupId(true)as needed).
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.html
Outdated
Show resolved
Hide resolved
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.html
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
library/src/skins/default/src/sass/modules/tool/gradebook/_gradebook.scss (1)
430-435: Inconsistent button sizing compared to student filter.The item filter clear button (20px) differs from the student filter clear button (16px height at line 426). Review whether this inconsistency is intentional or if both buttons should use the same size for visual and interaction consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.html(1 hunks)library/src/skins/default/src/sass/modules/tool/gradebook/_gradebook.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/pages/GradebookPage.html
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{html,jsp,vm,xhtml,xml,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Ensure all UI components work across different screen sizes (Responsive Design)
Files:
library/src/skins/default/src/sass/modules/tool/gradebook/_gradebook.scss
🧠 Learnings (1)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
library/src/skins/default/src/sass/modules/tool/gradebook/_gradebook.scss
Show resolved
Hide resolved
ern
changed the title
tsarchghs
changed the title
kunaljaykam
left a comment
kunaljaykam
left a comment
There was a problem hiding this comment.
my suggestion is i would do my best to optimize every piece of code i write in gb. do all kinds of testing with the the entire table because it breaks a lot :) thanks @tsarchghs good work
kunaljaykam
changed the title
|
@kunaljaykam this effects library for style and gradebook for the html |
|
So you have to compile two modules > @kunaljaykam and this effects library for style and gradebook for the html |
|
I signed cla. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js(12 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.js
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.js: Use clean, standard modern JavaScript in Sakai frontends
Update jQuery code to modern JavaScript when making changes, if the changes are minimal
Prefer ES6+ features (arrow functions, template literals, destructuring, etc.) in JavaScript
Write modular, reusable JavaScript components in Sakai frontends
Minimize use of global variables and functions (Avoid Global Scope) in JavaScript
Files:
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
**/*.{js,html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Use the web components in the webcomponents/ directory when possible in Sakai frontends
Files:
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Target evergreen browsers; assume ES2022+ features and browser APIs likefetchkeepalive are present; avoid legacy branches, UA sniffing, or fallbacks unless a specific evergreen gap is documented
Replace jQuery with modern DOM APIs when touching code; new work should not add jQuery dependencies
Compose Lit components, ES modules, and encapsulated helpers; keep state local and explicit with modular code
Prefer module scope or class fields; expose intentional APIs instead of incidental globals; avoid global side channels
Files:
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
**/*/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Internal reactive state in Lit components should stay prefixed with
_and is only surfaced through getters/setters when required
Files:
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
**/*.{html,jsp,jspx,xml,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer kebab-case for values of HTML class and id attributes
Files:
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js
🧠 Learnings (1)
📓 Common learnings
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
🧬 Code graph analysis (1)
gradebookng/tool/src/webapp/scripts/gradebook-gbgrade-table.js (3)
chat/chat-tool/tool/src/webapp/js/chat.js (2)
portal(26-26)bootstrap(27-27)gradebookng/tool/src/java/org/sakaiproject/gradebookng/tool/component/GbGradeTable.java (1)
GbGradeTable(49-149)gradebookng/tool/src/webapp/scripts/gradebook-grade-summary.js (1)
column(118-118)
|
@coderabbitai review this PR strictly for performance. the table is built using the Tabulator library. identify hot paths repeated work and high complexity loops over rows and columns. focus on redraws DOM access string allocations and event handler leaks. assume large datasets. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 118647 @kunaljaykam I've analyzed the PR for performance issues with large datasets. Here are the critical concerns: 🔴 Critical Performance Issues1. O(rows × columns) Nested Loop with Heavy AllocationsLocation: GbGradeTable.applyColumnFilter = function(data) {
for (var i=GbGradeTable.columns.length-1; i>=0; i--) {
var column = GbGradeTable.columns[i];
if (GbGradeTable.isColumnHidden(column)) {
for(var row=0; row<data.length; row++) {
data[row] = data[row]
.slice(0, i + GbGradeTable.FIXED_COLUMN_OFFSET)
.concat(data[row].slice(i + (GbGradeTable.FIXED_COLUMN_OFFSET + 1)));
}
}
}
return data;
};Problems:
Impact: With 200 students and 50 columns, this could create thousands of temporary arrays. 2. Column Objects Recreated on Every RedrawLocation: return GbGradeTable._fixedColumns.concat(
GbGradeTable.columns
.filter(col => !GbGradeTable.isColumnHidden(col))
.map((column, colIndex) => ({
field: (GbGradeTable._fixedColumns.length + colIndex).toString(),
formatter: GbGradeTable.cellFormatter,
formatterParams: { _data_: column },
titleFormatter: GbGradeTable.headerFormatter(null, column),
width: 180,
editor: column.type === 'category' || column.externallyMaintained ? false : "GbGradeTableEditor",
}))
);Problems:
3. Double Render in redrawTable()Location: GbGradeTable.instance.redraw(true);
GbGradeTable.instance.setColumns(GbGradeTable.getFilteredColumns());
GbGradeTable.instance.setData(GbGradeTable.getFilteredData());Problems:
4. Repeated Column IterationsLocation: The function loops through 🟡 Moderate Performance Issues5. Event Handler AccumulationLocation: const $itemFilterInput = $("#itemFilterInput");
if ($itemFilterInput.length) {
$itemFilterInput
.on("keyup", function (event) {
// handler
})
.on("keydown", function (event) {
// handler
});
}Problems:
6. String Operations in Hot PathLocation: const studentSearchString = searchableFields.join(";").normalize("NFD").replace(/[\u0300-\u036f]/g, "");
for (var i=0; i<queryStrings.length; i++) {
const queryString = queryStrings[i].normalize("NFD").replace(/[\u0300-\u036f]/g, "");
if (studentSearchString.match(new RegExp(queryString, "i")) == null) {Problems:
7. buildItemFilterHaystacks Array ChurnLocation: Lines ~465-473 GbGradeTable.buildItemFilterHaystacks = function() {
GbGradeTable.columns.forEach(column => {
const haystack = [
column?.title,
column?.abbrevTitle,
column?.name,
column?.categoryName,
].filter(Boolean).join(" ").toLowerCase();
column._itemFilterHaystack = haystack;
});
};Problems:
📊 Performance RecommendationsPriority 1: Fix applyColumnFilterUse a precomputed visible column index map instead of modifying row arrays: // Cache visible column indices
GbGradeTable._visibleColumnIndices = null;
GbGradeTable.updateVisibleColumnIndices = function() {
const indices = [];
GbGradeTable.columns.forEach((col, i) => {
if (!GbGradeTable.isColumnHidden(col)) {
indices.push(i + GbGradeTable.FIXED_COLUMN_OFFSET);
}
});
GbGradeTable._visibleColumnIndices = indices;
};
// Use indices to filter data without slice/concat
GbGradeTable.applyColumnFilter = function(data) {
return data.map(row => {
const filtered = row.slice(0, GbGradeTable.FIXED_COLUMN_OFFSET);
GbGradeTable._visibleColumnIndices.forEach(i => filtered.push(row[i]));
return filtered;
});
};Priority 2: Cache Column DefinitionsOnly recreate column objects when columns actually change, not on every redraw. Priority 3: Clean Up Event HandlersUse Priority 4: Single Column PassCombine the two loops in Would you like me to create a detailed optimization plan or generate refactored code for the critical issues? 🧠 Learnings used |
| GbGradeTable.buildItemFilterHaystacks = function() { | ||
| GbGradeTable.columns.forEach(column => { | ||
| const haystack = [ | ||
| column?.title, | ||
| column?.abbrevTitle, | ||
| column?.name, | ||
| column?.categoryName, | ||
| ].filter(Boolean).join(" ").toLowerCase(); | ||
| column._itemFilterHaystack = haystack; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
@coderabbitai lets write columnSearchText or something else instead of haystack
There was a problem hiding this comment.
@kunaljaykam, great suggestion! Using a more descriptive name like columnSearchText would definitely improve code readability. The term "haystack" is a metaphor (from "needle in a haystack"), but columnSearchText or searchableText makes the purpose clearer.
To keep the naming consistent, I'd recommend updating:
- Function name:
buildItemFilterHaystacks→buildColumnSearchText(orbuildItemFilterSearchText) - Local variable:
haystack→columnSearchText - Property name:
column._itemFilterHaystack→column._searchText(orcolumn._columnSearchText) - Usage in
matchesItemFilter: Update the reference fromcolumn?._itemFilterHaystacktocolumn?._searchText
This would make the code more self-documenting and easier to understand for anyone reading it.
🧠 Learnings used
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
|
@tsarchghs any changes which is not related to this pr "add filter for gb items" lets remove that and we can do that in a seprate pr. for this we only change/add whats needed for the filter feature. it will be easier for me to review and and its less risk |
|
This PR has been automatically marked as stale due to 21 days of inactivity. It will be closed in 7 days unless there is new activity. |
4 participants
Summary by CodeRabbit
New Features
Usability Improvements
Localization
✏️ Tip: You can customize this high-level summary in your review settings.