Skip to content

Conversation

@jesusmmp
Copy link
Contributor

@jesusmmp jesusmmp commented Dec 18, 2025

https://sakaiproject.atlassian.net/browse/SAK-52249

Summary by CodeRabbit

  • New Features

    • The "add item" modal now submits when pressing Enter while a form control is focused, speeding data entry.
  • Bug Fixes

    • Enter-key handling is confined to the modal and is removed when the modal closes to prevent leftover listeners, accidental submissions, and interaction conflicts.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 18, 2025

Walkthrough

Adds a DOMContentLoaded listener that, if #newitem-modal exists, attaches a keydown handler on the modal body to call addItem when Enter is pressed within .form-control, and removes that handler on hidden.bs.modal.

Changes

Cohort / File(s) Summary
Modal-scoped Enter-key handler
microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
On DOMContentLoaded, if #newitem-modal exists, attach a keydown listener to the modal body that invokes addItem when Enter is pressed on .form-control; register cleanup to remove the listener on hidden.bs.modal.

Possibly related PRs

Suggested reviewers

  • juanjmerono
  • ottenhoff

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly describes the main fix: enabling Enter key functionality in the pop-up for creating folders or files, which matches the AI-generated summary showing a DOMContentLoaded listener that submits addItem when Enter is pressed.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e116d8f and b579cd0.

📒 Files selected for processing (1)
  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js (1 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{js,ts,jsx,tsx}: Target evergreen browsers; assume ES2022+ features and browser APIs like fetch keepalive 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: maven-build
  • GitHub Check: sakai-deploy
  • GitHub Check: maven-build

@jesusmmp
Copy link
Contributor Author

@coderrabitai full review

@jesusmmp
Copy link
Contributor Author

jesusmmp commented Dec 19, 2025

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

✅ Actions performed

Full review triggered.

@jesusmmp jesusmmp marked this pull request as ready for review December 19, 2025 09:38
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

✅ Actions performed

Full review triggered.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js (1)

515-540: Good fix for modal scoping, with minor polish opportunities.

The modal-scoped listener correctly addresses the previous review. The setup and teardown logic is sound.

Two minor refinements:

  1. Line 535: Use the modal variable (defined at line 516) instead of re-querying document.getElementById('newitem-modal'). This avoids an unnecessary DOM lookup.

  2. Lines 525–531: The indentation appears inconsistent—the nested if statements should align with standard JavaScript formatting.

🔎 Suggested improvements
 document.addEventListener('DOMContentLoaded', function() {
 	const modal = document.getElementById('newitem-modal');
 
 	if (!modal) {
 		return;
 	}
 
 	modal.addEventListener('shown.bs.modal', function() {
 		const modalElem = document.getElementById('newitem-modal-body-container');
 		const handleEnter = (event) => {
-		if (event.key === 'Enter' || event.keyCode === KEYCODE.ENTER) {
-			if (event.target.classList.contains('form-control')) {
-					event.preventDefault();
-					event.stopPropagation();
-					addItem();
-				}
-			}
+			if (event.key === 'Enter' || event.keyCode === KEYCODE.ENTER) {
+				if (event.target.classList.contains('form-control')) {
+					event.preventDefault();
+					event.stopPropagation();
+					addItem();
+				}
+			}
 		};
 		modalElem.addEventListener('keydown', handleEnter);
 
-		document.getElementById('newitem-modal').addEventListener('hidden.bs.modal', function cleanup() {
+		modal.addEventListener('hidden.bs.modal', function cleanup() {
 			modalElem.removeEventListener('keydown', handleEnter);
 			this.removeEventListener('hidden.bs.modal', cleanup);
 		}, { once: true });
 	});
 });
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e116d8f and 1d19607.

📒 Files selected for processing (1)
  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js (1 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{js,ts,jsx,tsx}: Target evergreen browsers; assume ES2022+ features and browser APIs like fetch keepalive 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: sakai-deploy
  • GitHub Check: maven-build
  • GitHub Check: maven-build

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

✅ Actions performed

Full review triggered.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js (2)

535-538: Simplify cleanup pattern and reuse existing modal reference.

Two minor refinements:

  1. Line 535 calls document.getElementById('newitem-modal') when the modal variable (line 516) is already available—use modal directly.
  2. Line 537 manually removes the hidden.bs.modal listener, but line 538 already specifies { once: true }, which auto-removes the listener after it fires. The manual removal is redundant.
🔎 Proposed cleanup refactor
-	document.getElementById('newitem-modal').addEventListener('hidden.bs.modal', function cleanup() {
+	modal.addEventListener('hidden.bs.modal', function cleanup() {
 		modalElem.removeEventListener('keydown', handleEnter);
-		this.removeEventListener('hidden.bs.modal', cleanup);
 	}, { once: true });

524-532: Fix indentation for consistency.

The indentation inside the handleEnter function is inconsistent—lines 526–531 have extra spacing that doesn't align with standard formatting.

🔎 Suggested indentation fix
 	const handleEnter = (event) => {
-	if (event.key === 'Enter' || event.keyCode === KEYCODE.ENTER) {
-		if (event.target.classList.contains('form-control')) {
-				event.preventDefault();
-				event.stopPropagation();
-				addItem();
-			}
-		}
+		if (event.key === 'Enter' || event.keyCode === KEYCODE.ENTER) {
+			if (event.target.classList.contains('form-control')) {
+				event.preventDefault();
+				event.stopPropagation();
+				addItem();
+			}
+		}
 	};
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e116d8f and 1d19607.

📒 Files selected for processing (1)
  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js (1 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{js,ts,jsx,tsx}: Target evergreen browsers; assume ES2022+ features and browser APIs like fetch keepalive 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: sakai-deploy
🔇 Additional comments (1)
microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js (1)

515-540: Implementation addresses the previous scoping concern.

The Enter key handler is now properly scoped to the modal body container and only active when the modal is shown, which resolves the prior issue of triggering addItem() for any .form-control element across the entire page.

@jesusmmp
Copy link
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

✅ Actions performed

Full review triggered.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 57fcc2a and 9c29751.

📒 Files selected for processing (1)
  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js (1 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{js,ts,jsx,tsx}: Target evergreen browsers; assume ES2022+ features and browser APIs like fetch keepalive 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
🧠 Learnings (1)
📚 Learning: 2025-11-24T19:11:31.821Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: .cursor/rules/frontend.mdc:0-0
Timestamp: 2025-11-24T19:11:31.821Z
Learning: Applies to **/*.js : Update jQuery code to modern JavaScript when making changes, if the changes are minimal

Applied to files:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: maven-build
  • GitHub Check: sakai-deploy
  • GitHub Check: maven-build

… the pop-up when creating a Folder or a file
@jesusmmp
Copy link
Contributor Author

jesusmmp commented Dec 22, 2025

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

✅ Actions performed

Full review triggered.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js (1)

515-544: Good progress addressing previous feedback, but two minor refinements remain.

The null check for modalElem (lines 525-527) and reuse of the modal variable on line 540 are excellent improvements. However, two items from the previous review are still pending:

  1. Line 530: The event.keyCode check is a legacy IE fallback. Since the coding guidelines specify targeting evergreen browsers with ES2022+ features, event.key === 'Enter' alone is sufficient.

  2. Line 541: The manual removeEventListener call is redundant because { once: true } on line 542 automatically removes the listener after it fires once.

🔎 Proposed refinements
 	modal.addEventListener('shown.bs.modal', function() {
 		const modalElem = document.getElementById('newitem-modal-body-container');
 
 		if (!modalElem) {
 			return;
 		}
 
 		const handleEnter = (event) => {
-			if (event.key === 'Enter' || event.keyCode === KEYCODE.ENTER) {
+			if (event.key === 'Enter') {
 				if (event.target.classList.contains('form-control')) {
 						event.preventDefault();
 						event.stopPropagation();
 						addItem();
 				}
 			}
 		};
 		modalElem.addEventListener('keydown', handleEnter);
 
 		modal.addEventListener('hidden.bs.modal', function cleanup() {
 			modalElem.removeEventListener('keydown', handleEnter);
 		}, { once: true });
 	});

As per coding guidelines, target evergreen browsers and avoid legacy fallbacks.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 57fcc2a and ccc515e.

📒 Files selected for processing (1)
  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
🧰 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{js,ts,jsx,tsx}: Target evergreen browsers; assume ES2022+ features and browser APIs like fetch keepalive 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: sakai-deploy
  • GitHub Check: maven-build
  • GitHub Check: maven-build

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

✅ Actions performed

Full review triggered.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js (1)

515-544: Good progress addressing previous feedback, but remove the legacy keyCode check.

Most of the previous review feedback has been successfully implemented:

  • ✅ Modal variable is now reused (line 540) instead of re-querying the DOM
  • ✅ Null check for modalElem is present (lines 525-527)
  • ✅ The redundant manual removal of the hidden.bs.modal listener has been eliminated

However, one issue remains:

Line 530: The legacy event.keyCode check should be removed. Modern evergreen browsers fully support event.key, and the coding guidelines explicitly state to avoid legacy fallbacks.

Additionally, lines 532-534 have inconsistent indentation (extra leading whitespace).

🔎 Proposed refactor
 document.addEventListener('DOMContentLoaded', function() {
 	const modal = document.getElementById('newitem-modal');
 
 	if (!modal) {
 		return;
 	}
 
 	modal.addEventListener('shown.bs.modal', function() {
 		const modalElem = document.getElementById('newitem-modal-body-container');
 
 		if (!modalElem) {
 			return;
 		}
 
 		const handleEnter = (event) => {
-			if (event.key === 'Enter' || event.keyCode === KEYCODE.ENTER) {
+			if (event.key === 'Enter') {
 				if (event.target.classList.contains('form-control')) {
-						event.preventDefault();
-						event.stopPropagation();
-						addItem();
+					event.preventDefault();
+					event.stopPropagation();
+					addItem();
 				}
 			}
 		};
 		modalElem.addEventListener('keydown', handleEnter);
 
 		modal.addEventListener('hidden.bs.modal', function cleanup() {
 			modalElem.removeEventListener('keydown', handleEnter);
 		}, { once: true });
 	});
 });

As per coding guidelines, target evergreen browsers and prefer modern JavaScript without legacy fallbacks.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 57fcc2a and ccc515e.

📒 Files selected for processing (1)
  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
🧰 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{js,ts,jsx,tsx}: Target evergreen browsers; assume ES2022+ features and browser APIs like fetch keepalive 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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.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:

  • microsoft-integration/collaborative-documents/src/main/webapp/WEB-INF/templates/js/common.js
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: sakai-deploy

@jesusmmp
Copy link
Contributor Author

Ready to merge

@jesusmmp jesusmmp merged commit 990a043 into sakaiproject:master Dec 22, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants