[#339] Refactor @forge/consts #343
Conversation
At its current state this implementation is WASHED (all caps)
not sure what I did
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
📝 WalkthroughWalkthroughRestructures constants across the monorepo by migrating from knight-hacks path imports to a consolidated namespace architecture in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Reasoning: This is a large-scale refactor affecting 100+ files across 8 apps and 3 packages with a consistent but repetitive pattern. While the changes follow a predictable namespace-mapping scheme, the heterogeneity of affected modules (components, routers, utilities, schemas) and the introduction of new validation/utility functions in forms require careful verification of:
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @alexanderpaolini. * #343 (comment) The following files were modified: * `apps/blade/src/app/_components/bad-perms.tsx` * `apps/blade/src/app/_components/discord-modal.tsx` * `apps/blade/src/app/admin/_components/GenderPie.tsx` * `apps/blade/src/app/admin/_components/MajorBarChart.tsx` * `apps/blade/src/app/admin/_components/RaceOrEthnicityPie.tsx` * `apps/blade/src/app/admin/_components/SchoolBarChart.tsx` * `apps/blade/src/app/admin/_components/SchoolYearPie.tsx` * `apps/blade/src/app/admin/club/data/_components/EventDemographics.tsx` * `apps/blade/src/app/admin/club/data/_components/event-data/AttendancesBarChart.tsx` * `apps/blade/src/app/admin/club/data/_components/event-data/PopularityRanking.tsx` * `apps/blade/src/app/admin/club/data/_components/event-data/TypePie.tsx` * `apps/blade/src/app/admin/club/data/_components/event-data/WeekdayPopularityRadar.tsx` * `apps/blade/src/app/admin/club/data/_components/member-data/GenderPie.tsx` * `apps/blade/src/app/admin/club/data/_components/member-data/SchoolBarChart.tsx` * `apps/blade/src/app/admin/club/data/_components/member-data/ShirtSizePie.tsx` * `apps/blade/src/app/admin/club/data/_components/member-data/YearOfStudyPie.tsx` * `apps/blade/src/app/admin/club/events/_components/create-event.tsx` * `apps/blade/src/app/admin/club/events/_components/update-event.tsx` * `apps/blade/src/app/admin/club/members/_components/delete-member.tsx` * `apps/blade/src/app/admin/club/members/_components/final-dues-dialog.tsx` * `apps/blade/src/app/admin/club/members/_components/member-profile.tsx` * `apps/blade/src/app/admin/club/members/_components/update-member.tsx` * `apps/blade/src/app/admin/forms/[slug]/client.tsx` * `apps/blade/src/app/admin/forms/[slug]/responses/_components/ResponsePieChart.tsx` * `apps/blade/src/app/admin/forms/[slug]/responses/page.tsx` * `apps/blade/src/app/admin/hackathon/data/_components/HackerCharts.tsx` * `apps/blade/src/app/admin/hackathon/data/_components/LevelOfStudyPie.tsx` * `apps/blade/src/app/admin/hackathon/data/_components/ShirtSizePie.tsx` * `apps/blade/src/app/admin/hackathon/events/_components/create-event.tsx` * `apps/blade/src/app/admin/hackathon/events/_components/update-event.tsx` * `apps/blade/src/app/admin/hackathon/hackers/_components/hacker-profile.tsx` * `apps/blade/src/app/admin/hackathon/hackers/_components/update-hacker.tsx` * `apps/blade/src/app/dashboard/_components/hackathon-dashboard/hackathon-dashboard.tsx` * `apps/blade/src/app/dashboard/_components/member-dashboard/event/event-feedback.tsx` * `apps/blade/src/app/forms/[formName]/_components/question-response-card.tsx` * `apps/blade/src/app/hacker/application/_components/hacker-application-form.tsx` * `apps/blade/src/app/member/application/_components/member-application-form.tsx` * `apps/blade/src/app/settings/_components/delete-member-button.tsx` * `apps/blade/src/app/settings/hacker-profile/hacker-profile-form.tsx` * `apps/blade/src/app/settings/hacker-profile/page.tsx` * `apps/blade/src/app/settings/member-profile-form.tsx` * `apps/blade/src/components/admin/forms/question-edit-card.tsx` * `apps/club/src/app/_components/landing/discover.tsx` * `apps/club/src/app/officers/_components/officers.tsx` * `packages/api/src/utils.ts` * `packages/consts/src/forms/index.ts` * `packages/db/scripts/seed_devdb.ts`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/blade/src/app/sponsor/page.tsx (1)
78-85:⚠️ Potential issue | 🟡 MinorAdd a
titleto the iframe for screen-reader accessibility.Without a title, assistive tech can’t describe the embedded content, which is especially important now that multiple videos are shown.
✅ Suggested fix
- {VIDEOS.map((src) => ( + {VIDEOS.map((src, index) => ( <div key={src} className="overflow-hidden rounded-lg border bg-card"> <div className="aspect-video w-full"> <iframe src={src} + title={`Sponsor video ${index + 1}`} className="h-full w-full" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" allowFullScreen />As per coding guidelines, Accessibility (alt text, ARIA, semantic HTML).
apps/blade/src/components/admin/forms/instruction-edit-card.tsx (1)
19-29:⚠️ Potential issue | 🟠 MajorFix
FORMStype-only import to avoid a TypeScript compile error.The type-only import
import type { FORMS }prevents accessingFORMS.InstructionValidatorin thetypeofexpression. TypeScript can't use runtime values intypeofwhen the value symbol isn't available.Change to a value import:
Fix
-import type { FORMS } from "@forge/consts"; +import { FORMS } from "@forge/consts";Alternatively, if your linting rules require type-only imports, export a type alias from
@forge/consts(e.g.,export type InstructionValidatorType = z.infer<typeof InstructionValidator>) and import it directly:-import type { FORMS } from "@forge/consts"; -type FormInstruction = z.infer<typeof FORMS.InstructionValidator>; +import type { InstructionValidatorType } from "@forge/consts"; +type FormInstruction = InstructionValidatorType;apps/blade/src/app/admin/club/data/_components/event-data/WeekdayPopularityRadar.tsx (1)
36-44:⚠️ Potential issue | 🟠 MajorPre-existing bug: Key mismatch in weekday data accumulation.
This code writes to
weekdayData.Monbut reads fromweekdayData.Monday(and similarly for other days). The accumulation will always start fresh because the keys don't match. This bug predates this PR but may cause incorrect average calculations.🐛 Example fix for Monday case
case 1: { weekdayData.Mon = { totalAttendees: - (weekdayData.Monday?.totalAttendees ?? 0) + + (weekdayData.Mon?.totalAttendees ?? 0) + numAttended + numHackerAttended, - totalEvents: (weekdayData.Monday?.totalEvents ?? 0) + 1, + totalEvents: (weekdayData.Mon?.totalEvents ?? 0) + 1, }; break; }Do you want me to open a separate issue to track this pre-existing bug fix?
apps/blade/src/app/admin/forms/[slug]/responses/page.tsx (1)
21-25:⚠️ Potential issue | 🟠 MajorNext.js 15:
paramsshould be awaited.In Next.js 15, the
paramsprop is now a Promise and must be awaited before accessing its properties. This applies to page, layout, and route handler functions.🐛 Update to await params
export default async function FormResponsesPage({ params, }: { - params: { slug: string }; + params: Promise<{ slug: string }>; }) { + const { slug } = await params; const session = await auth(); if (!session) { redirect(SIGN_IN_PATH); }Then update usages of
params.slugto justslug.As per coding guidelines: This file is in
apps/blade/**which requires proper Next.js App Router patterns.apps/club/src/app/_components/landing/discover.tsx (1)
109-115:⚠️ Potential issue | 🟡 MinorRemove unnecessary type assertion on DISCORD.PERMANENT_INVITE.
The constant
PERMANENT_INVITEis already a string literal ("https://discord.com/invite/Kv5g9vf"), so theas stringcast is unnecessary and violates the no-escape-hatches guideline. Simply remove it:onClick={() => window.open( DISCORD.PERMANENT_INVITE, "_blank", "noopener,noreferrer", ) }apps/blade/src/app/dashboard/_components/hackathon-dashboard/hackathon-dashboard.tsx (1)
45-76:⚠️ Potential issue | 🔴 CriticalFix stale
HACKER_CLASS_INFOreference after namespace switch.Line 75 still uses
HACKER_CLASS_INFO, which is no longer imported and will fail to compile after the switch toDISCORD.KNIGHTHACKS_8. Update the typed lookup to use the new namespace.🛠️ Suggested fix
- const HACKER_CLASS_INFO_TYPED: Record<string, HackerClassInfo> = - HACKER_CLASS_INFO as Record<string, HackerClassInfo>; + const HACKER_CLASS_INFO_TYPED: Record<string, HackerClassInfo> = + DISCORD.KNIGHTHACKS_8.HACKER_CLASS_INFO as Record<string, HackerClassInfo>;
🤖 Fix all issues with AI agents
In
`@apps/blade/src/app/admin/club/data/_components/event-data/PopularityRanking.tsx`:
- Around line 1-6: The file exporting the PopularityRanking component uses React
hooks (useState) but lacks the Next.js App Router client directive; add "use
client" as the very first line of the module to mark this file as a client
component so hooks like useState are allowed, then keep the existing imports and
component implementation (look for PopularityRanking and any uses of useState)
unchanged except for adding that directive at the top.
In `@apps/blade/src/app/admin/hackathon/data/_components/HackerCharts.tsx`:
- Around line 1-4: This component is missing the client directive required for
hooks; add the "use client" directive as the very first line of HackerCharts.tsx
so that React's useState and any tRPC React hooks (the client-side hooks used in
this component) run on the client, ensuring the component is treated as a client
component.
In `@apps/blade/src/app/admin/hackathon/events/_components/delete-event.tsx`:
- Around line 6-7: The import of EVENTS is currently type-only which causes a
compile error when using typeof EVENTS.EVENT_TAGS; change the type-only import
to a value import by removing the `type` keyword (e.g., replace `import type {
EVENTS }` with `import { EVENTS }`) and make the same change for any other
occurrences of `import type { EVENTS }` (e.g., the other import at line 29) so
that typeof EVENTS.EVENT_TAGS can access the runtime value.
In `@packages/consts/src/discord/index.ts`:
- Around line 41-45: Define a dedicated development recruiting channel constant
and use it instead of falling back to DEV_LOG_CHANNEL: add
DEV_RECRUITING_CHANNEL (e.g., alongside PROD_RECRUITING_CHANNEL) and update the
RECRUITING_CHANNEL assignment to choose PROD_RECRUITING_CHANNEL when IS_PROD is
true and DEV_RECRUITING_CHANNEL otherwise, modifying the symbols
PROD_RECRUITING_CHANNEL and RECRUITING_CHANNEL (and adding
DEV_RECRUITING_CHANNEL) in packages/consts/src/discord/index.ts.
In `@packages/consts/src/util.ts`:
- Line 1: The util.ts file exposes IS_PROD by directly reading
process.env.NODE_ENV which violates the rule that env access must be
centralized; either move this constant into a validated env module (create
packages/consts/src/env.ts and export IS_PROD from there using the validated env
import pattern) or remove IS_PROD from this shared consts package and have
consumers derive production mode from their own validated env (import { env }
from "~/env"); if this direct NODE_ENV access is intentionally allowed, rename
util.ts to env.ts and document the exception so it falls under the allowed
pattern.
🧹 Nitpick comments (30)
apps/blade/src/app/_components/navigation/user-dropdown.tsx (1)
88-91: Consider adding fallback content for accessibility.When the avatar image fails to load, users see an empty fallback. Adding initials or a placeholder icon improves the experience, especially for users on slow connections or when images are blocked.
♿ Optional improvement
<Avatar> <AvatarImage src={`${data ? data.avatar : ""}`} /> - <AvatarFallback></AvatarFallback> + <AvatarFallback> + {data?.name?.charAt(0).toUpperCase() ?? "U"} + </AvatarFallback> </Avatar>As per coding guidelines:
apps/blade/**requires checking "Accessibility (alt text, ARIA, semantic HTML)".apps/blade/src/app/admin/club/data/_components/member-data/ShirtSizePie.tsx (1)
68-68: Color lookups correctly updated to useFORMSnamespace.The modulo indexing pattern
FORMS.ADMIN_PIE_CHART_COLORS[index % FORMS.ADMIN_PIE_CHART_COLORS.length]ensures colors cycle safely when there are more data points than available colors—nice defensive coding.One minor formatting note: line 68 is fairly long. Consider breaking it for readability if your linter permits.
,
✨ Optional: Line break for readability
baseConfig[shirtSize] = { label: shirtSize, - color: FORMS.ADMIN_PIE_CHART_COLORS[colorIdx % FORMS.ADMIN_PIE_CHART_COLORS.length], + color: + FORMS.ADMIN_PIE_CHART_COLORS[ + colorIdx % FORMS.ADMIN_PIE_CHART_COLORS.length + ], };Also applies to: 199-201
apps/blade/src/app/admin/forms/[slug]/responses/_components/ResponsePieChart.tsx (1)
14-14: Inconsistent quote style.This import uses single quotes while all other imports in the file use double quotes (lines 5-13). For consistency, consider using double quotes here as well.
🔧 Suggested fix
-import { FORMS } from '@forge/consts'; +import { FORMS } from "@forge/consts";apps/blade/src/components/admin/forms/question-edit-card.tsx (1)
532-534: Type assertion is acceptable but could be safer.The
as keyof typeofassertion works becauseoptionsConstis set via the<Select>above, which only offers valid keys. If you want extra safety against stale or invalid data, consider a fallback:FORMS.AVAILABLE_DROPDOWN_CONSTANTS[ optionsConst as keyof typeof FORMS.AVAILABLE_DROPDOWN_CONSTANTS ] ?? optionsConstThis ensures the UI doesn't break if an unexpected value sneaks in. Low priority given the controlled input source.
apps/blade/src/app/sponsor/page.tsx (1)
12-23: MoveSPONSOR_VIDEO_LINK_2into@forge/conststo avoid drift.Keeping URLs centralized aligns with the refactor goal and prevents this page from becoming a new “constants sink.” Consider exporting the second link alongside
SPONSOR_VIDEO_LINKand importing it here.💡 Suggested update
-import { SPONSOR_VIDEO_LINK } from "@forge/consts"; +import { SPONSOR_VIDEO_LINK, SPONSOR_VIDEO_LINK_2 } from "@forge/consts"; -// TODO: move to consts -const SPONSOR_VIDEO_LINK_2 = - "https://www.youtube.com/embed/OzW_4QeCfM0?si=G8SUf8UbEo2W5MnL";apps/blade/src/app/admin/_components/SchoolYearPie.tsx (1)
28-36: Eliminate array-index coupling to prevent silent mismatches across multiple files.The indices (2, 4, 6 for
LEVELS_OF_STUDYand 0, 1, 2 forSHORT_LEVELS_OF_STUDY) work correctly today, but tying label semantics to array position is fragile. If someone reorders or inserts new entries inLEVELS_OF_STUDY, all three files using this pattern break silently:
SchoolYearPie.tsxmember-profile.tsxhacker-profile.tsxConsider exporting a shared
LEVELS_TO_SHORT_LEVELS_MAPPINGconstant from@forge/consts, or apply guards locally to catch missing entries:✅ Safer local mapping with guards
const shortenLevelOfStudy = (levelOfStudy: string): string => { - const replacements: Record<string, string> = { - // Undergraduate University (2 year - community college or similar) - [FORMS.LEVELS_OF_STUDY[2]]: FORMS.SHORT_LEVELS_OF_STUDY[0], // Undergraduate University (2 year) - // Graduate University (Masters, Professional, Doctoral, etc) - [FORMS.LEVELS_OF_STUDY[4]]: FORMS.SHORT_LEVELS_OF_STUDY[1], // Graduate University (Masters/PhD) - // Other Vocational / Trade Program or Apprenticeship - [FORMS.LEVELS_OF_STUDY[6]]: FORMS.SHORT_LEVELS_OF_STUDY[2], // Vocational/Trade School - }; + const replacements: Record<string, string> = {}; + const pairs: Array<[number, number]> = [ + [2, 0], // Undergraduate University (2 year) + [4, 1], // Graduate University (Masters/PhD) + [6, 2], // Vocational/Trade School + ]; + for (const [longIdx, shortIdx] of pairs) { + const long = FORMS.LEVELS_OF_STUDY[longIdx]; + const short = FORMS.SHORT_LEVELS_OF_STUDY[shortIdx]; + if (long && short) replacements[long] = short; + } return replacements[levelOfStudy] ?? levelOfStudy; };packages/consts/src/discord/project-launch-26.ts (1)
1-1: Missing semicolon for consistency.Minor style nit: consider adding a semicolon for consistency with other files in the codebase.
✨ Suggested fix
-export const MEMBER_ROLE = "1467281189088788489" +export const MEMBER_ROLE = "1467281189088788489";packages/consts/README.md (1)
1-7: Helpful documentation for the consts package.This README provides clear guidance on when to add constants here. The deprecation note about
misc.tshelps prevent misuse.One minor suggestion for line 5—"kind of" could be tightened:
📝 Optional wording tweak
-Some types of data kind of go against this would be, for example, a named Discord channel that we need to remember. Storing it at the top of the file that it is used in makes no sense. Just some food for thought. +Some types of data seem to go against this rule—for example, a named Discord channel ID. Storing it at the top of the file where it's used makes no sense. Just some food for thought.apps/blade/src/app/settings/hacker-profile/page.tsx (1)
10-10: Minor: Inconsistent quote style.This import uses single quotes while surrounding imports (lines 5-8) use double quotes. Prettier should normalize this on save.
-import { DISCORD } from '@forge/consts'; +import { DISCORD } from "@forge/consts";apps/blade/src/app/forms/[formName]/_components/question-response-card.tsx (1)
30-30: Minor: Inconsistent quote style.Single quotes here while the rest of the file uses double quotes. Formatter should catch this.
-import { FORMS } from '@forge/consts'; +import { FORMS } from "@forge/consts";packages/consts/src/index.ts (1)
1-7: Clean namespace structure for the constants package.The
export * as NAMESPACEpattern provides good organization and prevents naming collisions. This is a solid approach for the refactor.Regarding the TODO on line 6: Consider creating an issue to track the removal of
miscexports to avoid this becoming stale technical debt.Would you like me to open an issue to track the migration of remaining
miscexports into proper namespaces?apps/blade/src/app/admin/club/data/_components/event-data/TypePie.tsx (1)
23-23: Import quote style inconsistency.This import uses single quotes while the rest of the file uses double quotes. Consider aligning for consistency.
✨ Suggested fix
-import { FORMS } from '@forge/consts'; +import { FORMS } from "@forge/consts";apps/blade/src/app/admin/club/data/_components/event-data/AttendancesBarChart.tsx (1)
20-20: Import quote style inconsistency.Same as TypePie.tsx - uses single quotes while the rest of the file uses double quotes.
✨ Suggested fix
-import { FORMS } from '@forge/consts'; +import { FORMS } from "@forge/consts";apps/blade/src/app/admin/club/data/_components/event-data/WeekdayPopularityRadar.tsx (1)
19-19: Import quote style inconsistency.Uses single quotes while the rest of the file uses double quotes.
✨ Suggested fix
-import { FORMS } from '@forge/consts'; +import { FORMS } from "@forge/consts";apps/blade/src/app/admin/club/data/_components/event-data/PopularityRanking.tsx (1)
6-6: Import quote style inconsistency.Uses single quotes while the file's other imports use double quotes.
✨ Suggested fix
-import { FORMS } from '@forge/consts'; +import { FORMS } from "@forge/consts";apps/blade/src/app/admin/club/members/_components/member-profile.tsx (1)
19-19: Import quote style inconsistency.Uses single quotes while the rest of the file uses double quotes.
✨ Suggested fix
-import { FORMS, MEMBER_PROFILE_ICON_SIZE } from '@forge/consts'; +import { FORMS, MEMBER_PROFILE_ICON_SIZE } from "@forge/consts";apps/blade/src/app/admin/_components/RaceOrEthnicityPie.tsx (1)
31-36: Pre-existing fragility: hardcoded array indices.While the namespace migration is correct, this code relies on hardcoded indices (
[4],[2],[0]) intoFORMS.RACES_OR_ETHNICITIESandFORMS.SHORT_RACES_AND_ETHNICITIES. If the array order changes, these mappings will silently break.Consider using a lookup map keyed by the full string value instead:
const RACE_ABBREVIATIONS: Record<string, string> = { "Native Hawaiian or Other Pacific Islander": "Native Hawaiian/Pacific Islander", "Hispanic / Latino / Spanish Origin": "Hispanic/Latino", "Native American or Alaskan Native": "Native American/Alaskan Native", };This is a pre-existing issue—not blocking for this PR, but worth addressing.
packages/consts/src/events.ts (1)
45-63: DeriveEventTagsColorfromEVENT_TAGSto avoid drift.
Manually duplicating literals can fall out of sync withEVENT_TAGS/EVENT_POINTS. Deriving the union keeps types aligned while still allowing"Hackathon"if it’s only a color tag. If Hackathon should be a full tag, consider adding it toEVENT_TAGSandEVENT_POINTS.♻️ Suggested refactor
-export type EventTagsColor = - | "GBM" - | "Social" - | "Kickstart" - | "Project Launch" - | "Hello World" - | "Sponsorship" - | "Tech Exploration" - | "Class Support" - | "Workshop" - | "OPS" - | "Hackathon" - | "Collabs" - | "Check-in" - | "Merch" - | "Food" - | "Ceremony" - | "CAREER-FAIR" - | "RSO-FAIR"; +export type EventTagsColor = (typeof EVENT_TAGS)[number] | "Hackathon";packages/consts/src/forms/index.ts (1)
226-255: MakegetDropdownOptionsFromConsttype-safe.
Accepting astringallows typos and silently returns[]. UsingDropdownConstantKeyand a map gives compile‑time coverage when new constants are added.♻️ Suggested refactor
+const DROPDOWN_OPTIONS: Record<DropdownConstantKey, readonly string[]> = { + LEVELS_OF_STUDY, + ALLERGIES, + MAJORS, + GENDERS, + RACES_OR_ETHNICITIES, + COUNTRIES, + SCHOOLS, + COMPANIES, + SHIRT_SIZES, + EVENT_FEEDBACK_HEARD, + SHORT_LEVELS_OF_STUDY, + SHORT_RACES_AND_ETHNICITIES, +}; + -export function getDropdownOptionsFromConst( - constName: string, -): readonly string[] { - switch (constName) { - case "LEVELS_OF_STUDY": - return LEVELS_OF_STUDY; - case "ALLERGIES": - return ALLERGIES; - case "MAJORS": - return MAJORS; - case "GENDERS": - return GENDERS; - case "RACES_OR_ETHNICITIES": - return RACES_OR_ETHNICITIES; - case "COUNTRIES": - return COUNTRIES; - case "SCHOOLS": - return SCHOOLS; - case "COMPANIES": - return COMPANIES; - case "SHIRT_SIZES": - return SHIRT_SIZES; - case "EVENT_FEEDBACK_HEARD": - return EVENT_FEEDBACK_HEARD; - case "SHORT_LEVELS_OF_STUDY": - return SHORT_LEVELS_OF_STUDY; - case "SHORT_RACES_AND_ETHNICITIES": - return SHORT_RACES_AND_ETHNICITIES; - default: - return []; - } -} +export function getDropdownOptionsFromConst( + constName: DropdownConstantKey, +): readonly string[] { + return DROPDOWN_OPTIONS[constName]; +}packages/consts/src/forms/companies.ts (1)
1-5: Add a source/snapshot note for this list.
A brief doc comment with the data source and snapshot date makes updates intentional and traceable for a shared const.📝 Suggested doc note
-export const COMPANIES = [ +// Source: <add data source + snapshot date here>. +export const COMPANIES = [As per coding guidelines:
packages/consts/**: Shared constants. Changes here affect the entire monorepo. Ensure values are correct and changes are intentional.apps/blade/src/app/settings/member-profile-form.tsx (1)
64-67: Type assertion for readonly array comparison is acceptable.The cast
(FORMS.COMPANIES as readonly string[])enables.includes()to work with the member's company string. This is a reasonable workaround for TypeScript's strict readonly array typing.Consider creating a type-guard utility if this pattern appears frequently:
💡 Optional helper utility
// In a shared utils file export function includesValue<T>(arr: readonly T[], value: T): boolean { return arr.includes(value); }packages/consts/src/misc.ts (2)
8-8: Inconsistent byte calculation methods.Line 8 uses
5 * 1000000(decimal megabytes) while line 13 uses2 * 1024 * 1024(binary mebibytes). While both work, mixing conventions can cause confusion.💡 Suggested consistency fix
-export const KNIGHTHACKS_MAX_RESUME_SIZE = 5 * 1000000; // 5MB +export const KNIGHTHACKS_MAX_RESUME_SIZE = 5 * 1024 * 1024; // 5MB (MiB)Or use both consistently with decimal (1000000) if that's intentional. The binary approach (
1024 * 1024) is more common for file size limits.Also applies to: 13-13
184-184: Consider environment variable for personify email.
GOOGLE_PERSONIFY_EMAILcontains a personal email address. While not a secret, hardcoding emails can complicate transitions if the responsible person changes.apps/blade/src/app/admin/roles/configure/_components/roleedit.tsx (2)
82-84: Missing dependency in useEffect.The
useEffectcallsupdateString(form.getValues())butformis not in the dependency array. While this works for initialization, it may cause issues ifformreference changes.💡 Suggested fix
useEffect(() => { updateString(form.getValues()); - }, []); + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentional: run once on mount + }, []);If intentionally running once on mount, add a comment explaining why. Otherwise, include
formin dependencies.
86-101: Dependency arrays may be incomplete.Both
useEffecthooks referencerolesandoldRolein their logic but don't include them in dependency arrays:
- Lines 86-101: Uses
rolesandoldRolebut only depends on[roleID]- Lines 103-110: Uses
rolesandoldRolebut only depends on[name]This could cause stale closure issues if
rolesoroldRolechange.Also applies to: 103-110
packages/api/src/routers/forms.ts (1)
189-189: Type casting is acceptable here, but consider adding runtime validation.The casts
as FORMS.FormTypeare necessary becauseformDatais stored as JSON in the database. However, if corrupted data exists, this could cause runtime issues.Consider adding a safeParse validation when retrieving form data from the database to catch any schema drift:
💡 Optional defensive pattern
const parseResult = FORMS.FormSchemaValidator.safeParse(form.formData); if (!parseResult.success) { throw new TRPCError({ message: "Form data is corrupted or uses an outdated schema", code: "INTERNAL_SERVER_ERROR", }); } const formData = parseResult.data;Also applies to: 428-428
packages/api/src/routers/event-feedback.ts (1)
15-15: Minor: Inconsistent quote style.This import uses single quotes while the rest of the file uses double quotes. Consider updating for consistency:
🔧 Suggested fix
-import { DISCORD, EVENTS } from '@forge/consts'; +import { DISCORD, EVENTS } from "@forge/consts";apps/blade/src/app/admin/club/data/_components/member-data/SchoolBarChart.tsx (1)
1-123: Consider consolidating duplicate SchoolBarChart components.This component is nearly identical to
apps/blade/src/app/admin/_components/SchoolBarChart.tsx, differing only in:
- Interface name (
MembervsPerson)- Data key name (
membersvspeople)- Label text ("member" vs "hacker")
Consider creating a shared, generic chart component that accepts these as props to reduce duplication:
💡 Example approach
interface SchoolBarChartProps<T extends { school?: string }> { data: T[]; dataKey: string; labelSingular: string; labelPlural: string; }packages/api/src/routers/hacker.ts (1)
6-26: Avoid deep relative type imports from consts source.Importing
AssignableHackerClassfrom a deep path couplespackages/apitopackages/constsinternals. Consider re-exporting the type from@forge/constsand importing from the package root to keep the public API stable.♻️ Example adjustment (after re-export)
-import type { AssignableHackerClass } from "../../../consts/src/discord/knight-hacks-8"; +import type { AssignableHackerClass } from "@forge/consts";packages/consts/src/discord/knight-hacks-8.ts (1)
1-5: Break the consts ↔ db type dependency cycle.The deep type import from
packages/dbmakes@forge/constsdepend on db internals. Consider movingHackerClass(orAssignableHackerClass) into a shared types module (or re-exporting it from consts) to remove the cycle.Want me to open an issue or sketch a small shared-types module layout?
apps/blade/src/app/admin/club/data/_components/event-data/PopularityRanking.tsx
Outdated
Show resolved
Hide resolved
apps/blade/src/app/admin/hackathon/events/_components/delete-event.tsx
Outdated
Show resolved
Hide resolved
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 (5)
apps/blade/src/app/admin/roles/configure/_components/roletable.tsx (3)
51-57:⚠️ Potential issue | 🟠 MajorIncluding
discordRolesQin dependencies will cause an infinite loop.The
useQueryhook returns a new object reference on every render. Adding it to the dependency array causes this effect to re-run continuously, triggering repeated API calls.Since the effect only needs to refetch when
roleschanges, removediscordRolesQfrom the dependency array and extract just therefetchfunction:🔧 Suggested fix
+ const { refetch: refetchDiscordRoles } = api.roles.getDiscordRoles.useQuery( + { roles: roles ?? [] }, + { enabled: false, retry: false }, + ); - const discordRolesQ = api.roles.getDiscordRoles.useQuery( - { roles: roles ?? [] }, - { enabled: false, retry: false }, - ); useEffect(() => { async function fetchDiscordRoles() { - setDiscordRoles((await discordRolesQ.refetch()).data); + setDiscordRoles((await refetchDiscordRoles()).data); } if (roles) void fetchDiscordRoles(); - }, [discordRolesQ, roles]); + }, [refetchDiscordRoles, roles]);Note: You'll also need to update line 94 to check
refetchDiscordRolesstatus differently, or keep a separate reference to the query for status checks.
86-92:⚠️ Potential issue | 🟡 MinorMissing
keyprop onTableRow.When rendering lists with
.map(), React needs a uniquekeyprop to efficiently track and update items. Using the item's uniqueidis preferred over the index.🔧 Suggested fix
- <TableRow id={"role" + i} className=""> + <TableRow key={v.id} id={"role" + i} className="">
150-156:⚠️ Potential issue | 🟡 MinorMissing
keyprop on<li>element.Each list item needs a unique
keyfor React's reconciliation algorithm.🔧 Suggested fix
- {getPermsAsList(v.permissions).map((p) => { + {getPermsAsList(v.permissions).map((p, idx) => { return ( - <li className={`p-1 text-sm text-muted-foreground`}> + <li key={p || idx} className={`p-1 text-sm text-muted-foreground`}> {p} </li> ); })}apps/blade/src/app/admin/roles/configure/_components/roleedit.tsx (2)
97-112:⚠️ Potential issue | 🟠 MajorIncluding
roleQin dependencies will cause excessive re-renders.Same issue as in
roletable.tsx: the query object fromuseQuerygets a new reference on every render, causing this effect to run continuously and repeatedly callroleQ.refetch().Extract the
refetchfunction to get a stable reference:🔧 Suggested fix
- const roleQ = api.roles.getDiscordRole.useQuery( + const { refetch: refetchRole } = api.roles.getDiscordRole.useQuery( { roleId: roleID }, { enabled: true, retry: false, }, ); useEffect(() => { if (roles) setIsDupeID( oldRole ? false : roles.find((v) => v.discordRoleId == roleID) != undefined, ); async function doGetRole() { setLoadingRole(true); - setRole((await roleQ.refetch()).data); + setRole((await refetchRole()).data); setLoadingRole(false); } void doGetRole(); - }, [oldRole, roleID, roleQ, roles]); + }, [oldRole, roleID, refetchRole, roles]);
272-294:⚠️ Potential issue | 🟡 MinorMissing
keyprop onFormFielditeration.Each
FormFieldrendered in the map needs a uniquekeyprop. The permission keyv[0]is unique and ideal here.🔧 Suggested fix
{Object.entries(PERMISSION_DATA).map((v) => ( <FormField + key={v[0]} control={form.control} name={v[0]}
🤖 Fix all issues with AI agents
In `@apps/blade/src/components/forms/form-card.tsx`:
- Around line 143-144: The render tries to access (fullForm?.formData as
FORMS.FormType).description which can be undefined during async load and will
throw; change the access to use safe optional chaining and a nullish fallback so
the fallback runs when formData or description is missing (e.g. read description
via the pattern ((fullForm?.formData as FORMS.FormType)?.description) ?? "No
description"), or alternatively guard the whole expression with
fullForm/formData checks before reading description in the component that
renders the form card.
In `@apps/cron/src/crons/role-sync.ts`:
- Around line 40-42: Wrap the direct Discord API call that assigns guildMember
(the discord.get(Routes.guildMember(...)) used in role-sync.ts) with
retry/backoff logic: on transient errors (network/5xx/429) retry a few times
with exponential backoff, but if the response is a 404 do not retry and treat as
“not found”; surface/log the final error when retries are exhausted. Ensure the
retry only surrounds the discord.get call (the Routes.guildMember request) and
preserves the cast to APIGuildMember once a successful response is returned.
🧹 Nitpick comments (7)
apps/blade/src/app/admin/hackathon/hackers/_components/hacker-profile.tsx (2)
124-130: Magic array indices are fragile—consider using a lookup map or named exports.Accessing constants via hardcoded indices (e.g.,
FORMS.LEVELS_OF_STUDY[2]) is brittle. If the array order changes in@forge/consts, this code silently breaks while the inline comments become stale lies.A safer approach is to define a mapping object that pairs the long-form values to their shortened display equivalents:
♻️ Suggested approach
// Could live in `@forge/consts` or locally const LEVEL_OF_STUDY_SHORT_LABELS: Record<string, string> = { "Undergraduate University (2 year - community college or similar)": "Undergraduate University (2 year)", "Graduate University (Masters, Professional, Doctoral, etc)": "Graduate University (Masters/PhD)", "Other Vocational / Trade Program or Apprenticeship": "Vocational/Trade School", }; // Usage in component const displayLevel = LEVEL_OF_STUDY_SHORT_LABELS[hacker.levelOfStudy] ?? hacker.levelOfStudy;This eliminates index coupling, removes the need for explanatory comments, and safely falls back when no mapping exists.
147-153: Same fragility concern with race/ethnicity indices.The pattern here mirrors the level-of-study block—hardcoded indices that will silently break if array order changes. A lookup map would make this more robust:
♻️ Suggested approach
const RACE_SHORT_LABELS: Record<string, string> = { "Native Hawaiian or Other Pacific Islander": "Native Hawaiian/Pacific Islander", "Hispanic / Latino / Spanish Origin": "Hispanic/Latino", "Native American or Alaskan Native": "Native American/Alaskan Native", }; // Usage const displayRace = RACE_SHORT_LABELS[hacker.raceOrEthnicity] ?? hacker.raceOrEthnicity;Consider consolidating both mappings (level of study + race/ethnicity) into
@forge/constsas part of this refactor so all consumers benefit.apps/blade/src/app/member/application/_components/member-application-form.tsx (1)
642-646: Consider extracting hardcoded graduation terms to a constant.For consistency with the refactoring pattern in this PR, the hardcoded
["Spring", "Summer", "Fall"]array could be extracted toFORMS.GRAD_TERMS(or similar). This would centralize all form options and ensure the values stay in sync with theGradTermtype.💡 Suggested refactor
If a
FORMS.GRAD_TERMSconstant is added to@forge/consts:<SelectContent> - {(["Spring", "Summer", "Fall"] as GradTerm[]).map((t) => ( + {FORMS.GRAD_TERMS.map((t) => ( <SelectItem key={t} value={t}> {t} </SelectItem> ))} </SelectContent>This removes the type assertion and keeps graduation terms alongside other form constants.
apps/blade/src/app/admin/forms/[slug]/client.tsx (1)
298-306: Type assertions for API response narrowing are acceptable here.Using
as FORMS.FormTypeto castformData.formDatais a reasonable approach since the API returns a broader type. However, consider adding a runtime validation layer in the future if the form data structure becomes more complex or if you encounter type mismatches at runtime.// Future improvement: validate at runtime const parsedFormData = FORMS.FormSchemaValidator.safeParse(formData.formData); if (!parsedFormData.success) { // handle validation error }This is optional and can be deferred since the current assertion works for the known data flow.
apps/blade/src/app/forms/[formName]/_components/form-responder-client.tsx (2)
113-113: Type assertion is acceptable here, but consider long-term typing.The
as FORMS.FormTypecast provides downstream type safety, which is the right direction for this refactor. Just note that type assertions bypass compile-time checks—if the API ever returns a different shape, this won't catch it until runtime.Ideally, the TRPC router would return properly typed
formData(e.g., via Zod inference), eliminating the need for the cast. That's likely out of scope for this PR, but worth considering as a follow-up improvement.
358-422: Pre-existinganytypes—future cleanup opportunity.This block explicitly disables several TypeScript rules to work around untyped
instructions. Since this PR brings inFORMS.FormTypefor stronger typing, a natural follow-up would be extending that to cover instructions (e.g.,FORMS.InstructionType[]), which would let you remove the eslint-disable directives.Not required for this PR, but flagging it since the refactor sets up the foundation. As per coding guidelines: "Flag usage of 'any' type... suggest 'unknown' with type guards... or proper typing instead."
apps/gemiknights/src/app/_components/ui/background-gradient-animation.tsx (1)
74-87: Throttle the movement updates to avoid a tight render loop.With
curX/curYin deps andsetCurX/setCurYinside the effect, React can re-render in a rapid loop while the cursor moves. ArequestAnimationFrameloop with refs avoids extra renders and keeps updates at frame rate.Proposed refactor (rAF + refs)
- const [curX, setCurX] = useState(0); - const [curY, setCurY] = useState(0); + const curXRef = useRef(0); + const curYRef = useRef(0); useEffect(() => { - function move() { - if (!interactiveRef.current) { - return; - } - setCurX(curX + (tgX - curX) / 20); - setCurY(curY + (tgY - curY) / 20); - interactiveRef.current.style.transform = `translate(${Math.round( - curX, - )}px, ${Math.round(curY)}px)`; - } - - move(); - }, [curX, curY, tgX, tgY]); + let rafId: number; + const move = () => { + if (!interactiveRef.current) return; + curXRef.current += (tgX - curXRef.current) / 20; + curYRef.current += (tgY - curYRef.current) / 20; + interactiveRef.current.style.transform = `translate(${Math.round( + curXRef.current, + )}px, ${Math.round(curYRef.current)}px)`; + rafId = requestAnimationFrame(move); + }; + rafId = requestAnimationFrame(move); + return () => cancelAnimationFrame(rafId); + }, [tgX, tgY]);
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/db/src/schemas/knight-hacks.ts (2)
520-533:⚠️ Potential issue | 🟠 MajorMake
editedAtnullable to distinguish real edits from creation.Currently,
editedAtdefaults tonow(), so it always equalscreatedAton insertion. This defeats the purpose of tracking when responses are actually edited. The update logic atpackages/api/src/routers/forms.ts:570explicitly setseditedAt: new Date()during edits, confirming the field should track real modifications.Setting
editedAtto nullable signals:NULL= never edited,timestamp= last edit timestamp.Suggested change
- editedAt: t.timestamp().notNull().defaultNow(), + editedAt: t.timestamp(),This requires a migration to alter the column and drop the default, allowing existing rows to remain unchanged (NULL = never edited).
491-533:⚠️ Potential issue | 🟠 MajorEnsure schema changes use migrations instead of db:push for production deployments.
The new columns
allowEdit(line 491) andeditedAt(line 532) have proper defaults, but the repo's current setup usesdrizzle-kit push(seepackage.jsonscripts). Per coding guidelines, schema changes need a proper migration path—create and execute migrations rather than relying ondb:pushfor production environments. This ensures deployment safety, auditability, and the ability to rollback if needed.Consider adding a migration file under a
drizzle/migrationsdirectory to handle these schema additions, configured indrizzle.config.ts.apps/blade/src/app/admin/forms/[slug]/client.tsx (1)
195-258:⚠️ Potential issue | 🔴 CriticalRemove
anycasts and fix missingresponseRoleIdsproperty.The explicit
anycasts mask a real type mismatch:responseRoleIdsis not a property on the form object returned bygetForm. It exists in a separateFormResponseRolesjunction table and must be fetched separately. Other files (e.g.,form-card.tsx) successfully callgetFormwithout theas anyescape hatch, so the input type is well-defined and inferred correctly.Fix by:
- Remove the
as anycast on line 12 (input is typed asz.object({ slug_name: z.string() }))- Fetch
responseRoleIdsseparately via a dedicated query instead of castingformData.responseRoleIds- Remove all eslint disables around the mutation—once types are correct, they'll be unnecessary
Example approach
// Fetch form data and response roles separately const { data: formData } = api.forms.getForm.useQuery( { slug_name: slug }, { retry: false, refetchOnWindowFocus: false } ); const { data: formResponseRoleIds = [] } = api.forms.getFormResponseRoles.useQuery( { formId: formData?.id }, { enabled: !!formData?.id } ); // Then in useEffect, use formResponseRoleIds directly setResponseRoleIds(formResponseRoleIds); // And in mutation, pass properly-typed input without casts updateFormMutation.mutate({ id: formData.id, formData: { /* ... */ }, // ... other fields responseRoleIds: formResponseRoleIds, });If a separate query isn't available yet, fetch role IDs in the form's query result by joining the junction table (modify the backend procedure to include them).
🤖 Fix all issues with AI agents
In `@apps/blade/src/app/forms/`[formName]/_components/form-responder-client.tsx:
- Around line 74-76: The existingResponseQuery error branch returns a plain
<div>, causing inconsistent UI; replace that return with the same styled Card
pattern used for other error/gate states in this file (use the Card component,
include the same error icon and a clear message) so the
existingResponseQuery.error path renders a Card with the icon and "Error loading
existing response" text consistent with other error UI.
In `@apps/blade/src/app/forms/`[formName]/_components/form-runner.tsx:
- Line 36: The userName prop declared on the FormRunner component is unused;
remove userName from the function signature and any related type/interface
(props) to clean up dead code, or if it should be shown in the UI, render it
where appropriate (e.g., in the header JSX) using the userName identifier—update
the FormRunner component signature and its callers to match the chosen approach
(either drop userName or add JSX that references userName).
- Around line 123-124: The JSX conditionally renders an empty banner div when
form.banner is present, which creates a useless DOM node; either remove the
placeholder div or replace it with the intended banner rendering (e.g., render
form.banner content, an <img>, or a Banner component). Update the component in
form-runner.tsx where the conditional uses form.banner to either remove that
conditional block or implement the real banner output (reference the form.banner
property and the surrounding Banner/markup where appropriate) so the DOM only
contains meaningful content.
In `@apps/blade/src/app/forms/`[formName]/_components/form-view-edit-client.tsx:
- Around line 71-73: The response guard fails to treat an empty array as
not-found; update the check where responseQuery is validated (the
responseQuery/data guard in form-view-edit-client.tsx) to also verify that
responseQuery.data has at least one item (e.g.,
Array.isArray(responseQuery.data) && responseQuery.data.length > 0) and render
<ResponseNotFound /> when the array is empty; this change corresponds to the
getUserResponse result handling and ensures ResponseNotFound is shown when no
matching responseId is returned.
In `@apps/blade/src/app/forms/`[formName]/_components/utils.ts:
- Around line 15-88: Replace the unsafe runtime evaluation of the string
zodValidator in getValidatorResponse with a pre-built Zod schema or remove
client-side schema evaluation entirely: change getValidatorResponse (and
callers: getValidationError, isFormValid) to accept a z.ZodSchema instead of a
string (or accept a nullable schema and short-circuit to treating validation as
UX-only), remove the new Function(...) usage, and ensure normalizeResponses
remains unchanged; alternatively, if you prefer server-only validation, drop
client schema evaluation and have these functions always return "no client
error" while relying on server endpoints (createResponse/editResponse) for
actual validation.
In `@apps/blade/src/app/forms/`[formName]/_hooks/useSubmissionSuccess.ts:
- Around line 24-26: The initial state for redirectCountdown in
useSubmissionSuccess can be a non-integer because it uses redirectDelayMs / 1000
directly; change the initializer to use Math.ceil(redirectDelayMs / 1000) so it
matches the Math.ceil used later (ensure you update the const
[redirectCountdown, setRedirectCountdown] = useState(...) expression to call
Math.ceil on redirectDelayMs / 1000), keeping references to redirectDelayMs,
redirectCountdown and setRedirectCountdown consistent.
In `@packages/api/src/routers/forms.ts`:
- Around line 342-352: The code checks FormsSchemas.findFirst into form but only
validates form?.allowEdit; add an explicit not-found guard after the lookup: if
form is null/undefined, throw a TRPCError with code "NOT_FOUND" and a clear
message (e.g., "Form not found") before proceeding to the allowEdit check so
subsequent DB operations aren't attempted for an invalid form ID; update the
block around FormsSchemas.findFirst, the form variable, and the existing
TRPCError usage to include this new check.
- Around line 505-575: The editResponse mutation currently updates FormResponse
without re-checking ownership and returns updated[0] unguarded, which can be
undefined if the row was deleted (TOCTOU); modify the db.update(FormResponse)
call to include ownership in the where clause (e.g. combine eq(FormResponse.id,
input.id) AND eq(FormResponse.userId, userId) or use existingResponse.userId) so
the update only affects rows owned by the user, then after the update assert
that updated.length > 0 and throw a TRPCError({ code: "NOT_FOUND", message:
"Response not found or not owned by user" }) if empty instead of returning
updated[0].
In `@packages/db/src/schemas/knight-hacks.ts`:
- Around line 17-28: The file currently declares PostgreSQL enums directly via
pgEnum (shirtSizeEnum, eventTagEnum, genderEnum, raceOrEthnicityEnum,
sponsorTierEnum, hackathonApplicationStateEnum) and relies on db:push; instead
create explicit migrations with drizzle-kit so enum creation/changes are
versioned: run drizzle-kit generate to produce a migration that creates these
enums (one migration per enum change if updating values), commit the generated
SQL migration(s), update the packages/db/package.json script to use migration
commands (migrate up) rather than db:push, and add a short note in the repo docs
that any change to FORMS.* or EVENTS.* enum arrays requires generating and
applying a migration before deployment.
🧹 Nitpick comments (9)
apps/blade/src/app/dashboard/_components/member-dashboard/forms/form-responses.tsx (1)
38-44: Clean refactor! Consider a small accessibility enhancement.The inline Button/Link pattern using
asChildis idiomatic and the simplification looks good. One optional improvement: for screen reader users navigating through multiple form responses, hearing just "Edit" or "View" repeatedly lacks context.Adding an
aria-labelwould improve the experience:♿ Optional accessibility improvement
<Button asChild size="sm"> <Link href={`/forms/${formResponse.formSlug}/${formResponse.id}`} + aria-label={`${formResponse.allowEdit ? "Edit" : "View"} ${formResponse.formName}`} > {formResponse.allowEdit ? "Edit" : "View"} </Link> </Button>apps/blade/src/app/forms/[formName]/_components/form-not-found.tsx (1)
7-10: Hide decorative icon from screen readers.The icon is purely visual; marking it as decorative avoids noisy SR output.
✅ Suggested fix
- <XCircle className="mx-auto mb-4 h-16 w-16 text-destructive" /> + <XCircle + aria-hidden="true" + className="mx-auto mb-4 h-16 w-16 text-destructive" + />As per coding guidelines, "Accessibility (alt text, ARIA, semantic HTML)".
apps/blade/src/app/forms/[formName]/_components/question-response-card.tsx (1)
32-109: Centralize response value type (include boolean consistently).Now that boolean is part of the public API, internal helpers should use the same union to avoid type drift and variance surprises.
✅ Suggested refactor
+type ResponseValue = string | string[] | number | Date | boolean | null; interface QuestionResponseCardProps { question: FormQuestion; - value?: string | string[] | number | Date | boolean | null; - onChange: (value: string | string[] | number | Date | boolean | null) => void; + value?: ResponseValue; + onChange: (value: ResponseValue) => void; onBlur?: () => void; ... } function QuestionBody({ ... - value?: string | string[] | number | Date | boolean | null; - onChange: (value: string | string[] | number | Date | null) => void; + value?: ResponseValue; + onChange: (value: ResponseValue) => void; ... }) { ... } // Then reuse ResponseValue in sub-component onChange signatures.packages/consts/src/forms/index.ts (3)
300-313: Type annotation conflicts withas constassertion.The explicit
string[]type annotation overrides the benefits ofas const. This results in losing the literal type inference thatas constprovides.✨ Suggested fix: Remove explicit type annotations to preserve const assertion
-export const WEEKDAY_ORDER: string[] = [ +export const WEEKDAY_ORDER = [ "Mon", "Tues", "Wed", "Thurs", "Fri", "Sat/Sun", ] as const; -export const RANKING_STYLES: string[] = [ +export const RANKING_STYLES = [ "md:text-lg lg:text-lg font-bold text-yellow-500", "md:text-lg lg:text-lg font-semibold text-gray-400", "md:text-lg lg:text-lg font-medium text-orange-500", ];
288-298: Same type inconsistency withreadonly string[].Using
readonly string[]alongsideas constis redundant and loses the tuple/literal type benefits.✨ Suggested fix
-export const ADMIN_PIE_CHART_COLORS: readonly string[] = [ +export const ADMIN_PIE_CHART_COLORS = [ "#f72585", "#b5179e", "#7209b7", "#3a0ca3", "#4361ee", "#4895ef", "#4cc9f0", "#560bad", "#480ca8", ] as const;
226-257: Consider using a record-based lookup for cleaner maintenance.The switch statement works but requires manual updates whenever a new constant is added to
AVAILABLE_DROPDOWN_CONSTANTS. A map-based approach keeps things DRY.♻️ Suggested refactor using a record lookup
+const DROPDOWN_CONST_MAP: Record<string, readonly string[]> = { + LEVELS_OF_STUDY, + ALLERGIES, + MAJORS, + GENDERS, + RACES_OR_ETHNICITIES, + COUNTRIES, + SCHOOLS, + COMPANIES, + SHIRT_SIZES, + EVENT_FEEDBACK_HEARD, + SHORT_LEVELS_OF_STUDY, + SHORT_RACES_AND_ETHNICITIES, +}; + export function getDropdownOptionsFromConst( constName: string, ): readonly string[] { - switch (constName) { - case "LEVELS_OF_STUDY": - return LEVELS_OF_STUDY; - case "ALLERGIES": - return ALLERGIES; - // ... etc - default: - return []; - } + return DROPDOWN_CONST_MAP[constName] ?? []; }apps/blade/src/app/forms/[formName]/_components/form-submitted-success.tsx (1)
46-48: Consider addingaria-livefor the countdown.Screen reader users won't hear the countdown update without an ARIA live region. This is a minor accessibility enhancement.
♿ Suggested accessibility improvement
- <p className="mt-4 text-sm text-muted-foreground"> + <p className="mt-4 text-sm text-muted-foreground" aria-live="polite"> Redirecting in {redirectCountdown}... </p>apps/blade/src/app/forms/[formName]/_components/form-runner.tsx (1)
12-12: Inconsistent quote style in import.Single quotes used here while double quotes are used elsewhere in the file.
✨ Consistency fix
-import type { FORMS } from '@forge/consts'; +import type { FORMS } from "@forge/consts";apps/blade/src/app/forms/[formName]/_components/form-responder-client.tsx (1)
84-87: Graceful degradation for dues check failures.Defaulting
hasPaidDuestotruewhen the dues check fails is a reasonable UX choice—it avoids blocking users due to API issues. Worth documenting this behavior with a comment for future maintainers.📝 Suggested: Add explanatory comment
+ // If dues check fails, default to allowing access to avoid blocking users + // due to transient API errors const duesCheckFailed = !!duesQuery.error; const hasPaidDues = duesCheckFailed ? true : (duesQuery.data?.duesPaid ?? false);
apps/blade/src/app/forms/[formName]/_components/form-responder-client.tsx
Show resolved
Hide resolved
apps/blade/src/app/forms/[formName]/_components/form-view-edit-client.tsx
Outdated
Show resolved
Hide resolved
Ignoring an issue. This should be fine because dhruv is saying it works on a later tsc version. We can upgrade but for now I'm just pushing.
also removed some stuff I had temp in a file... oops!
alexanderpaolini
added
Onboarding
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/blade/src/app/settings/_components/delete-member-button.tsx (1)
23-29:⚠️ Potential issue | 🟡 MinorUnused props
firstNameandlastName.These props are declared in the component signature but never used in the component body. Either remove them or use them (e.g., in the dialog description for personalization).
Option 1: Remove unused props
export default function DeleteMemberButton({ memberId, -}: { - memberId: string; - firstName: string; - lastName: string; -}) { +}: { + memberId: string; +}) {packages/api/src/routers/guild.ts (1)
250-250:⚠️ Potential issue | 🟡 MinorRemove or convert console.log to structured logging.
console.log("Resumé URL generated:", url)exposes potentially sensitive URL information in logs. Either remove this debug statement or use a proper logging utility with appropriate log levels.Suggested fix
- console.log("Resumé URL generated:", url);apps/blade/src/app/admin/roles/configure/_components/roleedit.tsx (2)
97-112:⚠️ Potential issue | 🟠 MajorPotential infinite loop:
roleQin useEffect dependencies.Adding
roleQto the dependency array is risky because the query object reference changes on every render. Since the effect callsroleQ.refetch(), this can trigger an infinite loop.The original
[roleID]dependency was likely correct—refetch when the role ID changes. Consider removingroleQandrolesfrom deps if they're only needed inside the effect, or restructure to avoid the cycle.Suggested fix
- }, [oldRole, roleID, roleQ, roles]); + }, [oldRole, roleID]);If you need to react to
roleschanges, split into a separate effect that doesn't trigger refetch.
272-294:⚠️ Potential issue | 🟡 MinorMissing
keyprop on FormField in map.React requires a unique
keyprop when rendering lists. TheFormFieldcomponent rendered inside.map()is missing this prop, which can cause rendering issues and warnings.Suggested fix
- {Object.entries(PERMISSIONS.PERMISSION_DATA).map((v) => ( + {Object.entries(PERMISSIONS.PERMISSION_DATA).map(([key, value]) => ( <FormField + key={key} control={form.control} - name={v[0]} + name={key} render={({ field }) => ( <FormItem className="flex flex-row gap-4 border-b p-3 duration-100 hover:bg-muted/50"> <FormControl> <Checkbox className="my-auto" checked={field.value} onCheckedChange={field.onChange} /> </FormControl> <FormLabel className="flex flex-col gap-1"> - <div className="my-auto">{v[1].name}</div> + <div className="my-auto">{value.name}</div> <div className="my-auto text-sm text-muted-foreground"> - {v[1].desc} + {value.desc} </div> </FormLabel> </FormItem> )} /> ))}packages/api/src/routers/member.ts (1)
66-72:⚠️ Potential issue | 🔴 CriticalBug: Wrong constant used for bucket name in
putObject.
minioClient.putObjectreceivesMINIO.BUCKET_REGIONas the first argument, but it should be the bucket name (MINIO.QR_BUCKET_NAME). The region was mistakenly passed instead of the bucket name.🐛 Proposed fix
await minioClient.putObject( - MINIO.BUCKET_REGION, + MINIO.QR_BUCKET_NAME, objectName, qrBuffer, qrBuffer.length, { "Content-Type": "image/png" }, );
🤖 Fix all issues with AI agents
In `@apps/blade/src/app/admin/club/members/_components/member-profile.tsx`:
- Around line 143-173: Add accessible labels to the social icon links and mark
decorative icons as hidden: for the interactive Link wrappers rendered when
member.githubProfileUrl, member.linkedinProfileUrl, or member.websiteUrl are
present, add descriptive aria-label attributes (e.g., aria-label="GitHub
profile", aria-label="LinkedIn profile", aria-label="Personal website") on the
Link elements (or the icon element inside them) so screen readers announce the
target; for the non-interactive gray icon variants rendered when those URLs are
absent, add aria-hidden="true" to the FaGithub, FaLinkedin, and FaGlobe elements
to make them decorative. Ensure the changes are applied where FaGithub,
FaLinkedin, FaGlobe and the Link components are used alongside
member.githubProfileUrl, member.linkedinProfileUrl, and member.websiteUrl
(keeping existing sizing via GUILD.MEMBER_PROFILE_ICON_SIZE).
In `@apps/blade/src/app/dashboard/_components/hacker-dashboard/hacker-data.tsx`:
- Around line 296-300: Remove the unnecessary type assertion on CLUB.USE_CAUTION
in the disabled prop: update the conditional that currently uses
(CLUB.USE_CAUTION as boolean) to use CLUB.USE_CAUTION directly, keeping the rest
of the expression (confirmationText !== "I am absolutely sure" || loading)
unchanged so disabled evaluates correctly with confirmationText and loading.
In `@packages/consts/package.json`:
- Line 32: Remove "discord-api-types" from dependencies and add it under
devDependencies in package.json because it's only used for compile-time enums
(GuildScheduledEventEntityType, GuildScheduledEventPrivacyLevel) that feed
internal constants EVENT_TYPE and EVENT_PRIVACY_LEVEL; update the package.json
entry accordingly and run your package manager (npm/yarn/pnpm) to update
lockfiles so the package is installed only for development and not bundled as a
runtime dependency.
In `@packages/consts/src/hackathons/index.ts`:
- Around line 1-6: Add brief JSDoc-style comments above the exported constants
SPONSOR_VIDEO_LINK and SPONSOR_VIDEO_LINK_2 describing that these are YouTube
embed URLs for sponsor videos (include which sponsor or purpose if known), the
intended usage (e.g., embedded iframe in the event website or sponsor section),
and any constraints (embed format, query params). Place the comments directly
above each constant declaration so consumers of SPONSOR_VIDEO_LINK and
SPONSOR_VIDEO_LINK_2 in the consts package can understand what the URLs
reference and how to use them.
In `@packages/consts/src/hackathons/kh8.ts`:
- Around line 1-4: The current brittle relative import of the HackerClass type
from the db package should be removed and the type moved to a shared, no-deps
location that both packages can depend on; create a minimal shared types package
(or add to `@forge/consts`) that exports HackerClass, update the db schema module
that currently defines/uses HackerClass to import/export the type from that
shared package, then change this file (kh8.ts) to import HackerClass from the
new shared package instead of "../../../db/src/schemas/knight-hacks" so you
eliminate the cross-package cycle and keep package boundaries clean.
- Around line 7-33: Add JSDoc comments describing each Discord role and user ID:
for each constant (e.g. PROD_DISCORD_ROLE_KNIGHT_HACKS_8,
DEV_DISCORD_ROLE_KNIGHT_HACKS_8, KH_EVENT_ROLE_ID, PROD_DISCORD_ROLE_OPERATORS,
DEV_DISCORD_ROLE_OPERATORS, PROD_DISCORD_ROLE_MACHINIST,
DEV_DISCORD_ROLE_MACHINIST, PROD_DISCORD_ROLE_SENTINELS,
DEV_DISCORD_ROLE_SENTINELS, PROD_DISCORD_ROLE_HARBINGER,
DEV_DISCORD_ROLE_HARBINGER, PROD_DISCORD_ROLE_MONSTOLOGIST,
DEV_DISCORD_ROLE_MONSTOLOGIST, PROD_DISCORD_ROLE_ALCHEMIST,
DEV_DISCORD_ROLE_ALCHEMIST, PROD_DISCORD_SUPERADMIN, DEV_DISCORD_SUPERADMIN) add
a brief JSDoc line stating what role/user the ID maps to and note the source as
the Discord server settings (role/user ID), and if applicable indicate which
constant is used in production vs dev (PROD_ vs DEV_) and that KH_EVENT_ROLE_ID
selects between them based on IS_PROD.
🧹 Nitpick comments (25)
packages/consts/src/project-launch/spring-26.ts (1)
1-1: Consider adding a brief comment for clarity.The constant follows SCREAMING_SNAKE_CASE correctly, but per the coding guidelines for
packages/consts/**, Discord IDs should be well-documented since they're shared repository-wide. A short comment helps future maintainers understand what this role represents.📝 Suggested documentation
+/** Discord role ID for Project Launch Spring '26 members */ export const MEMBER_ROLE = "1467281189088788489";apps/blade/src/app/admin/club/members/_components/member-profile.tsx (1)
106-112: Brittle magic indices—consider a lookup map instead.Accessing array constants by hardcoded indices (
[2],[4],[6]) is fragile. If the order inFORMS.LEVELS_OF_STUDYever changes, this code breaks silently. The inline comments help, but they can drift out of sync.A lookup map makes intent explicit and survives reordering:
♻️ Suggested refactor using a map
// Define at module level or inside the component const LEVEL_OF_STUDY_SHORT_LABELS: Record<string, string> = { "Undergraduate University (2 year - community college or similar)": "Undergraduate University (2 year)", "Graduate University (Masters, Professional, Doctoral, etc)": "Graduate University (Masters/PhD)", "Other Vocational / Trade Program or Apprenticeship": "Vocational/Trade School", }; // Then in the JSX: {LEVEL_OF_STUDY_SHORT_LABELS[member.levelOfStudy] ?? member.levelOfStudy}The same pattern applies to the race/ethnicity block below (lines 129–135).
apps/blade/src/app/member/application/_components/member-application-form.tsx (2)
187-193: Tie resume size message to the constant to avoid drift.
If the size limit changes later, the user-facing copy will stay accurate.🔧 Suggested tweak
- message: "File too large: maximum 5MB", + message: `File too large: maximum ${MINIO.MAX_RESUME_SIZE / (1024 * 1024)}MB`,
332-337: Derive grad-term options fromFORMS.TERM_TO_DATEto keep UI and date logic in sync.
Right now the select options are hardcoded while the date mapping is centralized; deriving from the mapping prevents drift if the constants change.♻️ Suggested refactor
- const [showOtherCompany, setShowOtherCompany] = useState(false); + const [showOtherCompany, setShowOtherCompany] = useState(false); + const gradTerms = Object.keys(FORMS.TERM_TO_DATE) as FORMS.GradTerm[];- {(["Spring", "Summer", "Fall"] as FORMS.GradTerm[]).map( - (t) => ( + {gradTerms.map((t) => ( <SelectItem key={t} value={t}> {t} </SelectItem> - ), - )} + ))}Also applies to: 634-640
apps/blade/src/app/admin/hackathon/events/_components/delete-event.tsx (1)
115-118: Remove the unnecessaryas booleantype assertion.
CLUB.USE_CAUTIONis already typed asboolean(it'struein the source). Theas booleancast is a TypeScript escape hatch that masks potential type issues. If the underlying type were ever widened (e.g., toboolean | undefined), this cast would hide the change rather than surfacing a compile error.♻️ Suggested fix
disabled={ - (CLUB.USE_CAUTION as boolean) + CLUB.USE_CAUTION ? confirmationText !== "I am absolutely sure" || isLoading : isLoading }Based on learnings: "No TypeScript escape hatches: Flag usage of 'any' type,
@ts-ignore,@ts-expect-error, and non-null assertions (!.) — suggest 'unknown' with type guards, optional chaining (?.), or proper typing instead."apps/blade/src/app/admin/club/events/_components/delete-event.tsx (1)
115-118: Remove the unnecessaryas booleantype assertion.Same issue as the hackathon variant —
CLUB.USE_CAUTIONis already a boolean literal (true). The cast is redundant and masks type safety.♻️ Suggested fix
disabled={ - (CLUB.USE_CAUTION as boolean) + CLUB.USE_CAUTION ? confirmationText !== "I am absolutely sure" || isLoading : isLoading }Based on learnings: "No TypeScript escape hatches: Flag usage of 'any' type,
@ts-ignore,@ts-expect-error, and non-null assertions (!.) — suggest 'unknown' with type guards, optional chaining (?.), or proper typing instead."packages/consts/src/team.ts (1)
48-79: Consider addingas constand aTeamtype for consistency.
OFFICERShasas constand an exportedOfficertype, butTEAMSlacks both. Adding them would:
- Enable literal type inference for team names/colors
- Provide a typed interface for consumers
♻️ Suggested addition
{ team: "Projects/Mentorship", color: "#3498db", director_role: "1244790444626280550", }, -]; +] as const; + +export type Team = (typeof TEAMS)[number];packages/consts/src/club.ts (1)
1-3: Add comments to clarify currency units.
MEMBERSHIP_PRICE = 2500appears to be cents (Stripe convention), whileDUES_PAYMENT = 25appears to be dollars. Both represent the same $25.00 amount but in different units. A brief comment prevents future confusion:📝 Suggested documentation
+/** Membership price in cents (for Stripe) */ export const MEMBERSHIP_PRICE = 2500; +/** Dues payment amount in dollars (for display) */ export const DUES_PAYMENT = 25;apps/blade/src/app/settings/_components/delete-member-button.tsx (1)
7-7: Remove unnecessary type assertion or fix the source type.The
as booleancast on line 104 is a TypeScript escape hatch. IfCLUB.USE_CAUTIONis already typed asbooleanin@forge/consts, the cast is redundant. If it's typed differently (e.g.,boolean | undefined), consider fixing the type at the source inpackages/consts/src/club.ts.Suggested fix (if already boolean)
disabled={ - (CLUB.USE_CAUTION as boolean) + CLUB.USE_CAUTION ? confirmationText !== "I am absolutely sure" || isLoading : isLoading }As per coding guidelines: "Flag usage of 'any' type,
@ts-ignore,@ts-expect-error, and non-null assertions (!.) — suggest 'unknown' with type guards, optional chaining (?.), or proper typing instead."Also applies to: 104-107
apps/blade/src/app/admin/club/members/_components/delete-member.tsx (1)
6-6: Same type assertion pattern — fix at the source.Like the other delete button component, this uses
(CLUB.USE_CAUTION as boolean). The cast should be eliminated by ensuring proper typing inpackages/consts/src/club.ts.Suggested fix
disabled={ - (CLUB.USE_CAUTION as boolean) + CLUB.USE_CAUTION ? confirm !== "I am absolutely sure" || isLoading : isLoading }Also applies to: 107-110
apps/blade/src/app/admin/club/members/_components/final-dues-dialog.tsx (1)
116-119: Repeated type assertion pattern.This is the third instance of
(CLUB.USE_CAUTION as boolean). Rather than fixing each consumer individually, ensureUSE_CAUTIONis exported asboolean(notboolean | undefinedor another type) inpackages/consts/src/club.ts.packages/consts/src/minio.ts (1)
12-18: Inconsistent size units: MB vs MiB.There's an inconsistency in how file sizes are calculated:
MAX_RESUME_SIZE: Uses decimal megabytes (5 * 1000000= 5 MB)KNIGHTHACKS_MAX_PROFILE_PICTURE_SIZE: Uses binary mebibytes (2 * 1024 * 1024= 2 MiB)Pick one convention for consistency. Binary (1024) is more common for file sizes in computing:
Suggested fix for consistency (using MiB)
// TODO: check if this should be MB or MiB -export const MAX_RESUME_SIZE = 5 * 1000000; // 5MB +export const MAX_RESUME_SIZE = 5 * 1024 * 1024; // 5 MiB export const PRESIGNED_URL_EXPIRY = 7 * 24 * 60 * 60; // 7 days -// TODO: see above -export const KNIGHTHACKS_MAX_PROFILE_PICTURE_SIZE = 2 * 1024 * 1024; // 2MB +export const KNIGHTHACKS_MAX_PROFILE_PICTURE_SIZE = 2 * 1024 * 1024; // 2 MiBpackages/api/src/routers/guild.ts (2)
16-21: Duplicate MinIO client instance.A second
s3Clientis created here with the same configuration as the importedminioClientfrom../minio/minio-client. This appears to be duplicated code—consider usingminioClientconsistently throughout this file.Suggested fix
-const s3Client = new Client({ - endPoint: env.MINIO_ENDPOINT, - useSSL: true, - accessKey: env.MINIO_ACCESS_KEY, - secretKey: env.MINIO_SECRET_KEY, -});Then update line 241 to use
minioClient:- const url = await s3Client.presignedUrl( + const url = await minioClient.presignedUrl(
229-229: Hardcoded bucket name should be centralized.The bucket name
"member-resumes"is hardcoded here while other bucket names (PROFILE_PICTURES_BUCKET_NAME,FORM_ASSETS_BUCKET_NAME,QR_BUCKET_NAME) are now centralized inpackages/consts/src/minio.ts. Add this constant there for consistency.Suggested addition to minio.ts
export const MEMBER_RESUMES_BUCKET_NAME = "member-resumes";packages/consts/src/events.ts (2)
25-45: ExportEventTagfor downstream typing.Consumers will likely need the tag union type; exporting it avoids re-deriving from
EVENT_TAGS.✅ Suggested change
-type EventTag = (typeof EVENT_TAGS)[number]; +export type EventTag = (typeof EVENT_TAGS)[number];
74-84: Add optional documentation comments to clarify purpose.The hardcoded calendar IDs and email address are safe to commit—they're shared calendar resource identifiers and an organizational contact, not credentials. Your actual Google API secrets (private key, client email) are correctly stored in environment variables.
To improve clarity for future maintainers, consider adding brief doc comments explaining the origin of these constants:
Optional documentation suggestion
+/** Shared Google Calendar ID for syncing events (prod/dev). Retrieved from Google Calendar settings. */ export const PROD_GOOGLE_CALENDAR_ID = "c_0b9df2b0062a5d711fc16060ff3286ef404b174bfafc4cbdd4e3009e91536e94@group.calendar.google.com"; + export const DEV_GOOGLE_CALENDAR_ID = "c_178118a9a25d9f278014b123b79b7e9caf9b29ac94bba3934237db00979e0f75@group.calendar.google.com"; export const GOOGLE_CALENDAR_ID = IS_PROD ? PROD_GOOGLE_CALENDAR_ID : DEV_GOOGLE_CALENDAR_ID; +/** Organizational email for Personify integration. */ export const GOOGLE_PERSONIFY_EMAIL = "dylan@knighthacks.org";packages/api/src/routers/hacker.ts (1)
1180-1183: Review the eslint-disable and type assertion.The
eslint-disablefor@typescript-eslint/no-unnecessary-conditioncombined with the type assertion suggests the type system isn't confident about this access pattern. This could mask runtime issues ifassignedClassdoesn't match a valid key inCLASS_ROLE_ID.Consider adding a runtime guard or using a type-safe lookup:
♻️ Safer lookup pattern
if (assignedClass) { + const roleId = HACKATHONS.KNIGHT_HACKS_8.CLASS_ROLE_ID[ + assignedClass as keyof typeof HACKATHONS.KNIGHT_HACKS_8.CLASS_ROLE_ID + ]; + if (roleId) { + await addRoleToMember(discordId, roleId); + } - await addRoleToMember( - discordId, - // eslint-disable-next-line `@typescript-eslint/no-unnecessary-condition` - HACKATHONS.KNIGHT_HACKS_8.CLASS_ROLE_ID[ - assignedClass as HACKATHONS.KNIGHT_HACKS_8.AssignableHackerClass - ] ?? "", - ); }As per coding guidelines: "Flag usage of...
@ts-expect-error... suggest... proper typing instead"packages/consts/src/discord.ts (1)
24-26: Track the TODO for DEV_ALUMNI_ROLE.Currently
ALUMNI_ROLEalways uses the production value. While this works, it may cause issues during development if the prod role ID doesn't exist in the dev Discord server.Would you like me to open an issue to track adding
DEV_ALUMNI_ROLE?packages/consts/src/forms/index.ts (2)
256-269: Inconsistent readonly typing on array constants.
WEEKDAY_ORDERandRANKING_STYLESare typed asstring[]while other array constants useas constfor readonly inference. This creates an inconsistency and allows accidental mutation.♻️ Add consistent readonly typing
-export const WEEKDAY_ORDER: string[] = [ +export const WEEKDAY_ORDER = [ "Mon", "Tues", "Wed", "Thurs", "Fri", "Sat/Sun", -] as const; +] as const; -export const RANKING_STYLES: string[] = [ +export const RANKING_STYLES = [ "md:text-lg lg:text-lg font-bold text-yellow-500", "md:text-lg lg:text-lg font-semibold text-gray-400", "md:text-lg lg:text-lg font-medium text-orange-500", -]; +] as const;
182-213: Consider a type-safe lookup for getDropdownOptionsFromConst.The switch statement works but requires manual maintenance when adding new dropdown constants. A lookup object approach would be more maintainable:
♻️ Type-safe lookup alternative
const DROPDOWN_LOOKUPS: Record<DropdownConstantKey, readonly string[]> = { LEVELS_OF_STUDY, ALLERGIES, MAJORS, GENDERS, RACES_OR_ETHNICITIES, COUNTRIES, SCHOOLS, COMPANIES, SHIRT_SIZES, EVENT_FEEDBACK_HEARD, SHORT_LEVELS_OF_STUDY, SHORT_RACES_AND_ETHNICITIES, }; export function getDropdownOptionsFromConst( constName: string, ): readonly string[] { return DROPDOWN_LOOKUPS[constName as DropdownConstantKey] ?? []; }packages/consts/README.md (1)
1-5: Good addition of package documentation.Clear guidance on when to add constants to this shared package. The exception case example (Discord channel IDs) is helpful for edge cases.
Minor wording suggestion to sound more confident:
📝 Clearer wording
-Some types of data kind of go against this would be, for example, a named Discord channel that we need to remember. Storing it at the top of the file that it is used in makes no sense. Just some food for thought. +Some types of data are exceptions to this rule—for example, a named Discord channel ID that we need to remember. Storing it at the top of the single file where it's used makes no sense. Just some food for thought.apps/blade/src/app/settings/member-profile-form.tsx (1)
654-660: Consider using a constant for graduation terms.The inline array
["Spring", "Summer", "Fall"]is hardcoded here but the typeFORMS.GradTermis derived fromTERM_TO_DATE. Consider extracting this as a constant or deriving it fromObject.keys(FORMS.TERM_TO_DATE)for consistency.♻️ Optional improvement
<SelectContent> - {(["Spring", "Summer", "Fall"] as FORMS.GradTerm[]).map( - (t) => ( + {(Object.keys(FORMS.TERM_TO_DATE) as FORMS.GradTerm[]).map((t) => ( <SelectItem key={t} value={t}> {t} </SelectItem> - ), - )} + ))} </SelectContent>apps/blade/src/app/forms/[formName]/_components/form-view-edit-client.tsx (1)
94-96: Avoidas anycast for responseData.Per coding guidelines,
anytype usage should be avoided. TheresponseDatafield likely has a defined type that can be used instead.♻️ Suggested approach
If
editResponse.mutateexpects a specific type forresponseData, use that type. If the API acceptsFormResponsePayload, cast to that:editResponse.mutate({ id: responseId, - // eslint-disable-next-line `@typescript-eslint/no-unsafe-assignment`, `@typescript-eslint/no-explicit-any` - responseData: payload as any, + responseData: payload, });If the types are genuinely incompatible, consider updating the schema input type or using
unknownwith proper type guards.As per coding guidelines: "Flag usage of 'any' type... suggest 'unknown' with type guards... or proper typing instead."
apps/blade/src/app/admin/forms/[slug]/client.tsx (2)
316-325: Stale comment contradicts the code.Line 325's comment states
// removed handleSaveForm to prevent save-on-every-renderbuthandleSaveFormis actually present in the dependency array. This is confusing for future maintainers.🧹 Fix the stale comment
}, [ duesOnly, allowResubmission, responseRoleIds, isLoading, allowEdit, handleSaveForm, - ]); // removed handleSaveForm to prevent save-on-every-render + ]);
316-341: Multiple auto-save effects may cause redundant API calls.Three separate
useEffecthooks triggerhandleSaveForm:
- Lines 316-325: On toggle changes
- Lines 328-330: On active item change
- Lines 333-341: Every 40 seconds
When switching items (changing
activeItemId), both effects 1 and 2 may fire if a toggle was also changed, potentially causing duplicate saves. ThehandleSaveFormfunction doesn't debounce or deduplicate requests.Consider consolidating these into a single debounced auto-save mechanism to reduce unnecessary API calls.
💡 Example: Debounced auto-save approach
// Add a debounced save hook instead of multiple effects const debouncedSave = useDebouncedCallback(handleSaveForm, 500); useEffect(() => { if (!isLoading) debouncedSave(); }, [duesOnly, allowResubmission, responseRoleIds, allowEdit, activeItemId, isLoading, debouncedSave]); // Keep the periodic save as a separate concern useEffect(() => { if (isLoading) return; const interval = setInterval(handleSaveForm, 40000); return () => clearInterval(interval); }, [isLoading, handleSaveForm]);You'd need to add
useDebouncedCallbackfrom a library likeuse-debounceor implement your own.
apps/blade/src/app/dashboard/_components/hacker-dashboard/hacker-data.tsx
Show resolved
Hide resolved
DVidal1205
left a comment
DVidal1205
left a comment
There was a problem hiding this comment.
Code wise, this looks great. Still think this needs a thorough Blade-wide test on local to see if anything broke from variable renaming. LGTM, but don't merge til that is done
3 participants
Why
Currently all of our "constants" are saved in one
@forge/const/knight-hacks. This file is around 6k lines and hinders workflow.What
Issue(s): #339
This PR proposes a new structure to the
@forge/constspackage. Domain specific constants are moved into their own files. For example, theDISCORDconstants are moved intodiscord/index.ts.Please do note that my changes are primarily to set structure right now. This PR will need to go through a few rounds of reviews before merging would be reasonable.
Test Plan
Merge me twin.
Checklist
db:pushbefore mergingSummary by CodeRabbit
Release Notes