Skip to content

refactor: activities results endpoints & clean code by SonarScan results #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
l7wei wants to merge 4 commits into
base: main
Choose a base branch
from
Open

refactor: activities results endpoints & clean code by SonarScan results #49

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d453fd
refactor: unify eligible voter count source and clean up common smells
Copilot Apr 20, 2026
34d0745
chore: finalize refactor and lint-safe cleanup adjustments
Copilot Apr 20, 2026
c3f8ebe
style: 改善語法規範
l7wei Apr 20, 2026
0805d6a
fix: address review-thread key collisions and add PUT route tests
Copilot Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,11 @@ WORKDIR /app
ENV NODE_ENV=production
ENV NEXT_TELEMETRY_DISABLED=1

RUN addgroup --system --gid 1001 nodejs
RUN adduser --system --uid 1001 nextjs
RUN addgroup --system --gid 1001 nodejs && adduser --system --uid 1001 nextjs

COPY --from=builder /app/public ./public

RUN mkdir .next
RUN chown nextjs:nodejs .next
RUN mkdir .next && chown nextjs:nodejs .next

COPY --from=builder --chown=nextjs:nodejs /app/.next/standalone ./
COPY --from=builder --chown=nextjs:nodejs /app/.next/static ./.next/static
Expand Down
14 changes: 8 additions & 6 deletions app/admin/activities/[id]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,13 +365,13 @@ function ActivityDetailPageContent() {
body: formData,
});
const data = await response.json();
if (!data.success) {
setError(data.error || "上傳選民名冊失敗");
} else {
if (data.success) {
setSuccessMessage(`選民名冊上傳成功，共 ${data.data.eligible_voters_count} 人`);
setVoterCsvFile(null);
await fetchVoterStats();
await refetch();
} else {
setError(data.error || "上傳選民名冊失敗");
}
} catch (err) {
console.error("Error uploading voter list:", err);
Expand Down Expand Up @@ -670,9 +670,11 @@ function ActivityDetailPageContent() {
</div>
)}

{option.vice &&
option.vice.map((vice, viceIndex) => (
<div key={viceIndex} className="ml-4 mb-1 text-sm">
{option.vice?.map((vice, viceIndex) => (
<div
key={`${option._id}-${vice.name}-${vice.department}-${vice.college}`}

Copilot AI Apr 20, 2026

Copy link

Choose a reason for hiding this comment

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

The vice list item key is derived from vice fields that can be missing or duplicated. Duplicate keys can cause incorrect rendering/state reuse. Consider including viceIndex (or another stable unique identifier) in the key.

Suggested change
key={`${option._id}-${vice.name}-${vice.department}-${vice.college}`}
key={`${option._id}-${viceIndex}-${vice.name}-${vice.department}-${vice.college}`}

Copilot uses AI. Check for mistakes.
className="ml-4 mb-1 text-sm"
>
<span className="text-muted-foreground">
副選 {viceIndex + 1}:{" "}
</span>
Expand Down
6 changes: 3 additions & 3 deletions app/admin/activities/[id]/verification/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,14 @@ function VerificationPageContent() {

if (!authData.authenticated || !authData.user?.isAdmin) {
// Not authenticated or not an admin, redirect to home
window.location.href = "/?error=admin_required";
globalThis.location.href = "/?error=admin_required";
return;
}

fetchVerificationData();
} catch (err) {
console.error("Error checking admin access:", err);
window.location.href = "/?error=auth_failed";
globalThis.location.href = "/?error=auth_failed";
}
};

Expand Down Expand Up @@ -135,7 +135,7 @@ function VerificationPageContent() {
link.style.visibility = "hidden";
document.body.appendChild(link);
link.click();
document.body.removeChild(link);
link.remove();
};

if (loading) {
Expand Down
2 changes: 1 addition & 1 deletion app/admin/activities/_components/ActivityFormFields.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export function ActivityFormFields({
formData,
onChange,
disabled = false,
}: ActivityFormFieldsProps) {
}: Readonly<ActivityFormFieldsProps>) {
return (
<div className="space-y-6">
<div className="grid grid-cols-1 gap-4 md:grid-cols-2">
Expand Down
2 changes: 1 addition & 1 deletion app/admin/activities/_components/CandidateFormFields.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export function CandidateFormFields({
onChange,
label,
required = false,
}: CandidateFormFieldsProps) {
}: Readonly<CandidateFormFieldsProps>) {
return (
<div className="space-y-3">
<h4 className="font-semibold">{label}</h4>
Expand Down
29 changes: 16 additions & 13 deletions app/admin/activities/_components/OptionFormSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,29 +37,28 @@ export function OptionFormSection({
editOption,
removeOption,
resetForm,
}: OptionFormSectionProps) {
}: Readonly<OptionFormSectionProps>) {
const handleAddOrUpdate = () => {
if (!currentOption.candidate.name) {
return;
if (currentOption.candidate.name) {
addOrUpdateOption();
}
addOrUpdateOption();
};

const handleRemove = (index: number) => {
removeOption(index);
};

const cardTitle =
editingIndex === null
? `新增候選人組合 #${options.length + 1}`
: `編輯候選人 #${editingIndex + 1}`;

return (
<div className="space-y-6">
{/* Current option form */}
<Card className="border-primary/20 bg-primary/5">
<CardHeader>
<CardTitle className="text-lg">
{editingIndex !== null
? `編輯候選人 #${editingIndex + 1}`
: `新增候選人組合 #${options.length + 1}`
}
</CardTitle>
<CardTitle className="text-lg">{cardTitle}</CardTitle>
</CardHeader>
<CardContent className="space-y-4">
<div className="space-y-2">
Expand Down Expand Up @@ -134,7 +133,12 @@ export function OptionFormSection({
已新增的候選人 ({options.length})
</h3>
{options.map((option, index) => (
<Card key={index} className={editingIndex === index ? "border-primary" : ""}>
<Card
key={`${option.label}-${option.candidate.name}-${option.vice
.map((v) => v.name)
.join("-")}`}

@gemini-code-assist gemini-code-assist Bot Apr 20, 2026

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

Using a key derived from editable fields like label and candidate.name is fragile. If two options have the same name and an empty label, the keys will collide, causing React rendering issues. Furthermore, any update to these fields will cause the entire component to unmount and remount because its key changed. It is recommended to use a stable unique identifier for each option in the local state.

Copilot AI Apr 20, 2026

Copy link

Choose a reason for hiding this comment

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

This list item key is built from label (optional) and candidate/vice names. It can easily collide (e.g., undefined label, duplicate names, empty vice names), leading to React key duplication and rendering/state issues. Prefer a stable unique key (e.g., include index or assign a per-option id when creating options).

Suggested change
key={`${option.label}-${option.candidate.name}-${option.vice
.map((v) => v.name)
.join("-")}`}
key={`option-${index}`}

Copilot uses AI. Check for mistakes.
className={editingIndex === index ? "border-primary" : ""}
>
<CardContent className="flex items-center justify-between py-4">
<div>
<p className="font-medium">
Expand Down Expand Up @@ -176,5 +180,4 @@ export function OptionFormSection({
);
}

// Export the hook for external use
export { useOptionForm };
export { useOptionForm } from "./useOptionForm";
4 changes: 2 additions & 2 deletions app/admin/activities/_components/ViceCandidateSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ export function ViceCandidateSection({
onAddVice,
onRemoveVice,
onViceChange,
}: ViceCandidateSectionProps) {
}: Readonly<ViceCandidateSectionProps>) {
return (
<div className="space-y-3">
{vices.map((vice, index) => (
<div
key={index}
key={`${vice.name}-${vice.department}-${vice.college}`}

Copilot AI Apr 20, 2026

Copy link

Choose a reason for hiding this comment

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

The key is derived from editable vice fields (name/department/college). When multiple vices are newly added (often empty strings/undefined), this can produce duplicate keys and cause React to reuse DOM/state between items. Use a stable unique key per vice entry (e.g., include index, or generate an id when adding a vice).

Suggested change
key={`${vice.name}-${vice.department}-${vice.college}`}
key={`vice-${index}-${vice.name ?? ""}-${vice.department ?? ""}-${vice.college ?? ""}`}

Copilot uses AI. Check for mistakes.
className="relative rounded-lg border border-border p-4 bg-background"
>
<Button
Expand Down
14 changes: 8 additions & 6 deletions app/admin/activities/_components/useOptionForm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,16 @@ export function useOptionForm() {
};

const addOrUpdateOption = () => {
if (editingIndex !== null) {
const newOptions = [...options];
newOptions[editingIndex] = currentOption;
setOptions(newOptions);
setEditingIndex(null);
} else {
if (editingIndex === null) {
setOptions([...options, currentOption]);
resetForm();
return;
}

const newOptions = [...options];
newOptions[editingIndex] = currentOption;
setOptions(newOptions);
setEditingIndex(null);
resetForm();
};

Expand Down
58 changes: 30 additions & 28 deletions app/admin/settings/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,35 @@ export default function AdminSettingsPage() {
}
};

const adminListContent = loading ? (
<p className="text-sm text-muted-foreground">載入中...</p>
) : admins.length === 0 ? (
<p className="text-sm text-muted-foreground">尚無資料</p>
) : (
<div className="space-y-2">
{admins.map((admin) => (
<div
key={admin.student_id}
className="flex items-center justify-between rounded-md border p-3"
>
<div>
<p className="font-medium">{admin.student_id}</p>
{admin.name && (
<p className="text-sm text-muted-foreground">{admin.name}</p>
)}
</div>
<Button
variant="destructive"
size="sm"
onClick={() => handleDelete(admin.student_id)}
>
<Trash2 className="h-4 w-4" />
</Button>
</div>
))}
</div>
);

return (
<div className="min-h-screen bg-background">
<Header />
Expand Down Expand Up @@ -162,34 +191,7 @@ export default function AdminSettingsPage() {
</CardHeader>
<Separator />
<CardContent className="pt-6">
{loading ? (
<p className="text-sm text-muted-foreground">載入中...</p>
) : admins.length === 0 ? (
<p className="text-sm text-muted-foreground">尚無資料</p>
) : (
<div className="space-y-2">
{admins.map((admin) => (
<div
key={admin.student_id}
className="flex items-center justify-between rounded-md border p-3"
>
<div>
<p className="font-medium">{admin.student_id}</p>
{admin.name && (
<p className="text-sm text-muted-foreground">{admin.name}</p>
)}
</div>
<Button
variant="destructive"
size="sm"
onClick={() => handleDelete(admin.student_id)}
>
<Trash2 className="h-4 w-4" />
</Button>
</div>
))}
</div>
)}
{adminListContent}
</CardContent>
</Card>
</main>
Expand Down
54 changes: 40 additions & 14 deletions app/api/activities/[id]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,31 @@
import { validateDateRange, isValidRule } from "@/lib/validation";
import { API_CONSTANTS } from "@/lib/constants";

interface ActivityUpdateBody {
name?: string;
type?: string;
description?: string;

@gemini-code-assist gemini-code-assist Bot Apr 20, 2026

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 description field should allow null to support clearing the value in the database. Without null, the field can only be updated to a new string or left unchanged.

Suggested change
description?: string;
description?: string | null;

rule?: string;
open_from?: string;
open_to?: string;
}

function buildActivityUpdateData(body: ActivityUpdateBody) {
const { name, type, description, rule, open_from, open_to } = body;
const updateData: Record<string, unknown> = {
updated_at: new Date(),
};

if (name) updateData.name = name;
if (type) updateData.type = type;
if (description !== undefined) updateData.description = description;
if (rule) updateData.rule = rule;
if (open_from) updateData.open_from = new Date(open_from);
if (open_to) updateData.open_to = new Date(open_to);

return updateData;
}

// GET /api/activities/[id] - Get single activity
export async function GET(
request: NextRequest,
Expand Down Expand Up @@ -72,8 +97,18 @@
return invalidIdResponse;
}

const body = await request.json();
const { name, type, description, rule, open_from, open_to } = body;
const rawBody = (await request.json()) as Record<string, unknown>;
const body: ActivityUpdateBody = {
name: typeof rawBody.name === "string" ? rawBody.name : undefined,
type: typeof rawBody.type === "string" ? rawBody.type : undefined,
description:
typeof rawBody.description === "string" ? rawBody.description : undefined,
Comment on lines +104 to +105

@gemini-code-assist gemini-code-assist Bot Apr 20, 2026

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 manual type check for description should include null to allow the field to be cleared. Currently, if rawBody.description is null, it is mapped to undefined, which prevents the update from reaching the database. This is a regression from the previous implementation where the raw body was destructured directly.

Suggested change
description:
typeof rawBody.description === "string" ? rawBody.description : undefined,
description:
(typeof rawBody.description === "string" || rawBody.description === null) ? rawBody.description : undefined,

rule: typeof rawBody.rule === "string" ? rawBody.rule : undefined,
open_from:
typeof rawBody.open_from === "string" ? rawBody.open_from : undefined,
open_to: typeof rawBody.open_to === "string" ? rawBody.open_to : undefined,
};
Comment on lines +100 to +110

Copilot AI Apr 20, 2026

Copy link

Choose a reason for hiding this comment

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

This route now contains non-trivial request-body parsing/coercion and date/rule validation logic. The repo has Jest coverage for other API routes, but this handler doesn’t appear to be tested; adding unit tests for PUT (valid/invalid rule, invalid dates, partial updates) would help prevent regressions.

Copilot uses AI. Check for mistakes.
const { rule, open_from, open_to } = body;

// Validate rule if provided
if (rule && !isValidRule(rule)) {
Expand All @@ -82,25 +117,16 @@

// Validate dates if provided
if (open_from && open_to) {
const openFrom = new Date(open_from);
const openTo = new Date(open_to);
const openFrom = new Date(open_from as string);

Check warning on line 120 in app/api/activities/[id]/route.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

This assertion is unnecessary since it does not change the type of the expression. 


See more on https://sonarcloud.io/project/issues?id=NTHU-SA_Voting-System&issues=AZ2sFjH7hhF0ZNhqkZF6&open=AZ2sFjH7hhF0ZNhqkZF6&pullRequest=49
const openTo = new Date(open_to as string);

Check warning on line 121 in app/api/activities/[id]/route.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

This assertion is unnecessary since it does not change the type of the expression. 


See more on https://sonarcloud.io/project/issues?id=NTHU-SA_Voting-System&issues=AZ2sFjH7hhF0ZNhqkZF7&open=AZ2sFjH7hhF0ZNhqkZF7&pullRequest=49

const dateValidation = validateDateRange(openFrom, openTo);
if (!dateValidation.valid) {
return createErrorResponse(dateValidation.error!);
}
}

const updateData: Record<string, unknown> = {
updated_at: new Date(),
};

if (name) updateData.name = name;
if (type) updateData.type = type;
if (description !== undefined) updateData.description = description;
if (rule) updateData.rule = rule;
if (open_from) updateData.open_from = new Date(open_from);
if (open_to) updateData.open_to = new Date(open_to);
const updateData = buildActivityUpdateData(body);

const activity = await Activity.findByIdAndUpdate(id, updateData, {
new: true,
Expand Down
9 changes: 5 additions & 4 deletions app/api/activities/[id]/voters/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import connectDB from "@/lib/db";
import { Activity } from "@/lib/models/Activity";
import { ActivityVoter } from "@/lib/models/ActivityVoter";
import { API_CONSTANTS } from "@/lib/constants";
import { getEligibleVotersCount } from "@/lib/activityVoterService";

function extractStudentIds(csvText: string): string[] {
const records = parse(csvText, {
Expand Down Expand Up @@ -151,7 +152,7 @@ export async function GET(
return createErrorResponse(API_CONSTANTS.ERRORS.ACTIVITY_NOT_FOUND, 404);
}

const count = await ActivityVoter.countDocuments({ activity_id: id });
const count = await getEligibleVotersCount(id);

return createSuccessResponse({
activity_id: id,
Expand Down Expand Up @@ -286,9 +287,7 @@ export async function POST(

const supportsTransactions = await supportsMongoTransactions(db);

if (!supportsTransactions) {
await replaceVotersWithoutTransaction();
} else {
if (supportsTransactions) {
const session = await db.startSession();
try {
await session.withTransaction(async () => {
Expand All @@ -302,6 +301,8 @@ export async function POST(
} finally {
await session.endSession();
}
} else {
await replaceVotersWithoutTransaction();
}

return createSuccessResponse({
Expand Down
Loading
Loading