Skip to content

Shorten subject labels to keep prefix before hyphen - #117

Merged
alvin000009238 merged 2 commits into
mainfrom
dev
Apr 1, 2026
Merged

Shorten subject labels to keep prefix before hyphen#117
alvin000009238 merged 2 commits into
mainfrom
dev

Conversation

@alvin000009238

Copy link
Copy Markdown
Owner

No description provided.

Copilot AI review requested due to automatic review settings April 1, 2026 05:01
@alvin000009238
alvin000009238 merged commit 35fe50c into main Apr 1, 2026
4 checks passed

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the shortenName function to dynamically shorten subject names by extracting the portion before a hyphen, replacing the previous hardcoded mapping logic. Unit tests have been updated to align with this new behavior. Feedback was provided regarding the handling of null values, as the current implementation would cast them to the string "null", potentially affecting the UI display.

Comment thread frontend/dashboard.js
Comment on lines +271 to 275
export function shortenName(name = '') {
const cleanedName = cleanSubjectName(String(name));
const [baseName] = cleanedName.split('-');
return baseName.trim() || cleanedName;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The current implementation will convert null values to the string "null" because String(name) is called before any null check. Since shortenName is used for UI display (e.g., in the standards table), this could result in the literal text "null" appearing in the dashboard if a subject name is missing. The previous implementation handled this gracefully because it returned the original null, which escapeHTML then converted to an empty string. Note that the default parameter name = '' only handles undefined, not null.

Suggested change
export function shortenName(name = '') {
const cleanedName = cleanSubjectName(String(name));
const [baseName] = cleanedName.split('-');
return baseName.trim() || cleanedName;
}
export function shortenName(name = '') {
if (name == null) return '';
const cleanedName = cleanSubjectName(String(name));
const [baseName] = cleanedName.split('-');
return baseName.trim() || cleanedName;
}

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates the frontend subject-label shortening logic to derive a shorter label by keeping only the prefix before a hyphen, and adjusts unit tests accordingly.

Changes:

  • Replaced hardcoded subject-name mappings with a hyphen-prefix rule in shortenName.
  • Updated dashboard unit tests to reflect the new hyphen-prefix behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
frontend/dashboard.js Changes shortenName to split on - (after cleaning) and return the prefix for display labels.
tests/frontend/dashboard.test.js Updates test cases to validate hyphen-prefix shortening and non-hyphen passthrough.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread frontend/dashboard.js
return shortNames[name] || name;
// 縮短科目名稱(僅保留 '-' 前的文字)
export function shortenName(name = '') {
const cleanedName = cleanSubjectName(String(name));

Copilot AI Apr 1, 2026

Copy link

Choose a reason for hiding this comment

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

shortenName(null) currently returns the string "null" because String(name) is called even when name is null. Backend grade payloads can contain SubjectName: null, and existing rendering paths call shortenName(subject.SubjectName), so this would surface "null" in tables/charts instead of an empty label. Treat null/undefined as empty (e.g., String(name ?? '') or an early if (name == null) return '').

Suggested change
const cleanedName = cleanSubjectName(String(name));
// Treat null/undefined as empty to avoid rendering the literal string "null"
if (name == null) return '';
const cleanedName = cleanSubjectName(String(name ?? ''));

Copilot uses AI. Check for mistakes.
it('should return original name when no hyphen exists', () => {
assert.equal(shortenName('數學A'), '數學A');
assert.equal(shortenName('英語文'), '英語文');
});

Copilot AI Apr 1, 2026

Copy link

Choose a reason for hiding this comment

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

Add assertions for shortenName(null) / shortenName(undefined) (expected empty string) since real grade payloads may have missing SubjectName. This prevents regressions where labels become the literal text "null"/"undefined".

Suggested change
});
});
it('should return empty string for null or undefined', () => {
assert.equal(shortenName(null), '');
assert.equal(shortenName(undefined), '');
});

Copilot uses AI. Check for mistakes.
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