✨ Enable to import JAG prelim and regional tasks (#1514) #1517
Conversation
WalkthroughThe changes in this pull request introduce a new contest type, Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (7)src/lib/clients/aizu_online_judge.ts (7)Line range hint The new type system effectively maps contest types to their corresponding rounds, preventing invalid combinations at compile time.
The cache implementation includes all essential features:
Line range hint The configuration system allows flexible customization of cache parameters while maintaining type safety.
The dispose method properly releases resources by disposing of both caches, preventing memory leaks.
The implementation includes:
Line range hint The implementation mirrors the robust approach used in contest fetching, ensuring consistency across the codebase. Line range hint The concurrent fetching implementation with Promise.allSettled ensures partial results are returned even if some requests fail. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
src/lib/types/contest.ts (1)
74-83: Consider enhancing type safety for language codesWhile the implementation is functional, consider improving type safety by using literal types for language codes instead of generic string keys.
Here's a suggested improvement:
-export type ContestLabelTranslations = { - [key: string]: string; -}; +type LanguageCode = 'en' | 'ja'; // Add more language codes as needed +export type ContestLabelTranslations = { + [key in LanguageCode]: string; +};This change would:
- Prevent accidental use of invalid language codes
- Make it clear which languages are supported
- Provide better IDE autocompletion
src/test/lib/utils/test_cases/task_url.ts (1)
141-152: Consider enhancing the pattern documentation.While the contest ID format is documented, it would be helpful to also document the task ID patterns and their relationship with contest years, similar to how it's documented for PCK contests.
Add this to the comment block:
// JAG contests follow these patterns: // - Contest ID format: JAG(Prelim|Regional)<YEAR> +// - Task ID format: +// - Recent contests (2021+): 33xx-33xx +// - Older contests (2005-2006): 20xx-20xxprisma/schema.prisma (1)
227-227: Consider database migration strategy.Since this change adds a new enum value, ensure you have a proper migration strategy in place:
- Deploy schema changes before application code
- Consider existing records and default values
- Update any relevant database seeds or test data
- Have a rollback plan ready
Would you like assistance in creating a comprehensive migration plan or reviewing the migration scripts?
src/lib/utils/task.ts (1)
Line range hint
7-15: Well-structured for platform extensibilityThe URL generation system's chain of responsibility pattern makes it easy to add new platforms like JAG. This aligns well with the TODO comment about supporting additional platforms (Codeforces, yukicoder, BOJ).
Consider documenting the pattern for adding new platforms in the codebase, as this will help future contributors implement support for the mentioned platforms in the TODO.
src/test/lib/utils/test_cases/contest_name_labels.ts (1)
359-444: Add documentation for year coverage patternThe test cases cover various years with some gaps (e.g., 2007-2008, 2012-2019 for prelims). Consider adding a comment explaining if these gaps represent years when contests weren't held, similar to the documentation pattern used for university contests above (see UTPC/TUPC comment).
export const aojJag = [ + // Note: Test cases cover years when JAG contests were actually held + // Prelims: 2005-2006, 2009-2011, 2020-2024 + // Regionals: 2005-2006, 2009-2011, 2016-2017, 2020-2022src/test/lib/utils/test_cases/contest_name_and_task_index.ts (1)
485-500: Consider refactoring to reduce code duplication.The
generateAojJagTestCasesfunction follows a similar pattern to other test case generators. Consider extracting the common pattern into a reusable utility function to reduce duplication.Example refactor:
+const generateContestTestCases = <T extends string>( + contestIds: T[], + taskIndices: string[], + formatName: (contestId: T, taskIndex: string) => string, + formatExpected: (contestId: T, taskIndex: string) => string, +) => { + return zip(contestIds, taskIndices).map(([contestId, taskIndex]) => { + return createTestCaseForContestNameAndTaskIndex(formatName(contestId, taskIndex))({ + contestId: contestId, + taskTableIndex: taskIndex, + expected: formatExpected(contestId, taskIndex), + }); + }); +}; -const generateAojJagTestCases = ( - contestIds: JagContestIds, - taskIndices: string[], -): { name: string; value: TestCaseForContestNameAndTaskIndex }[] => { - return zip(contestIds, taskIndices).map(([contestId, taskIndex]) => { - const testCase = createTestCaseForContestNameAndTaskIndex( - `AOJ, JAG${contestId} - ${taskIndex}`, - )({ - contestId: `JAG${contestId}`, - taskTableIndex: taskIndex, - expected: `AOJ - JAG${contestId.replace('Prelim', '模擬国内予選').replace('Regional', '模擬アジア地区予選')} - ${taskIndex}`, - }); - return testCase; - }); -}; +const generateAojJagTestCases = (contestIds: JagContestIds, taskIndices: string[]) => + generateContestTestCases( + contestIds, + taskIndices, + (contestId, taskIndex) => `AOJ, JAG${contestId} - ${taskIndex}`, + (contestId, taskIndex) => + `AOJ - JAG${contestId.replace('Prelim', '模擬国内予選').replace('Regional', '模擬アジア地区予選')} - ${taskIndex}`, + );src/test/lib/utils/contest.test.ts (1)
491-497: Consider adding test cases for edge cases in contest name labels.While the basic test structure is good, consider adding edge cases such as:
- Invalid contest IDs
- Mixed case variations
- Special characters in contest names
src/lib/utils/contest.ts (1)
Line range hint
300-307: Avoid reassigning function parameters ingetAojChallengeLabel.Assigning new values to function parameters can lead to confusion and potential bugs. Consider using a new variable to hold the modified contest ID for better clarity and maintainability.
Apply this diff to improve code clarity:
function getAojChallengeLabel(translations: ContestLabelTranslations, contestId: string): string { const baseLabel = 'AOJ - '; + let label = contestId; Object.entries(translations).forEach(([abbrEnglish, japanese]) => { - contestId = contestId.replace(abbrEnglish, japanese); + label = label.replace(abbrEnglish, japanese); }); - return baseLabel + contestId; + return baseLabel + label; }src/lib/clients/aizu_online_judge.ts (2)
197-197: UsetoUpperCase()instead oftoLocaleUpperCase()for consistent casingWhen converting
contestTypeto uppercase for logging, usingtoUpperCase()ensures consistent behavior across all locales. ThetoLocaleUpperCase()method's output can vary depending on the user's locale, which might lead to inconsistent logging.Apply this change:
- const contestTypeLabel = contestType.toLocaleUpperCase(); + const contestTypeLabel = contestType.toUpperCase();
358-358: UsetoUpperCase()instead oftoLocaleUpperCase()for consistent casingAs with the previous instance, replace
toLocaleUpperCase()withtoUpperCase()to maintain consistency in string casing regardless of locale settings.Apply this change:
- const contestTypeLabel = contestType.toLocaleUpperCase(); + const contestTypeLabel = contestType.toUpperCase();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
prisma/ERD.md(1 hunks)prisma/migrations/20241120113542_add_aoj_jag_to_contest_type/migration.sql(1 hunks)prisma/schema.prisma(1 hunks)src/lib/clients/aizu_online_judge.ts(10 hunks)src/lib/clients/index.ts(1 hunks)src/lib/types/contest.ts(3 hunks)src/lib/utils/contest.ts(4 hunks)src/lib/utils/task.ts(1 hunks)src/test/lib/utils/contest.test.ts(4 hunks)src/test/lib/utils/task.test.ts(1 hunks)src/test/lib/utils/test_cases/contest_name_and_task_index.ts(1 hunks)src/test/lib/utils/test_cases/contest_name_labels.ts(1 hunks)src/test/lib/utils/test_cases/contest_type.ts(1 hunks)src/test/lib/utils/test_cases/task_url.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- prisma/migrations/20241120113542_add_aoj_jag_to_contest_type/migration.sql
- src/lib/clients/index.ts
🔇 Additional comments (21)
src/lib/types/contest.ts (1)
26-29: LGTM! Well-documented addition of AOJ_JAG contest type
The new contest type and its documentation are well-implemented, maintaining consistency with the existing pattern and clearly describing its purpose.
Also applies to: 51-51
prisma/ERD.md (1)
32-32: LGTM! Verify schema consistency.
The addition of AOJ_JAG to the ContestType entity is well-integrated and aligns with the PR objectives to support JAG preliminary and regional tasks.
Let's verify the consistency of this change with related schema files:
✅ Verification successful
Schema changes are consistent across ERD, schema, and migrations
The addition of AOJ_JAG is properly reflected in:
- ERD diagram (
prisma/ERD.md) - Prisma schema with appropriate comment (
prisma/schema.prisma) - Migration file with correct ALTER TYPE statement (
prisma/migrations/20241120113542_add_aoj_jag_to_contest_type/migration.sql)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that AOJ_JAG is consistently defined across schema files
# Check if AOJ_JAG is present in the Prisma schema
echo "Checking Prisma schema..."
rg "AOJ_JAG" prisma/schema.prisma
# Check if AOJ_JAG is included in migration files
echo "Checking migration files..."
rg "AOJ_JAG" prisma/migrations/
Length of output: 367
src/test/lib/utils/test_cases/task_url.ts (3)
131-139: LGTM! Improved naming and URL structure.
The changes improve naming consistency and correctly reflect the AOJ URL structure for PCK contests.
Line range hint 154-161: LGTM! Consistent implementation.
The JAG contest export follows the established patterns and correctly implements the URL generation for JAG tasks.
Line range hint 141-161: Verify JAG contest coverage and URL format.
Let's verify that:
- All JAG contest types are covered
- The URL format matches AOJ's API expectations
✅ Verification successful
Let me gather more information about the JAG contest implementation to ensure comprehensive verification.
JAG contest implementation is complete and correctly structured
The verification confirms that:
- JAG contest types (Prelim and Regional) are properly covered in the implementation:
- Contest pattern is correctly defined in regex:
^JAG(Prelim|Regional|Summer|Winter|Spring)\d*$ - Test cases cover both Prelim and Regional contests from 2005 to 2024
- Contest pattern is correctly defined in regex:
- URL format is consistent with AOJ's API:
- Tasks URL is correctly defined as
${AOJ_BASE_URL}/problems - Task URLs are consistently formatted as
${AOJ_TASKS_URL}/${taskId}
- Tasks URL is correctly defined as
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify JAG contest coverage and URL format
# Check if we have test cases for all JAG contest types
echo "Checking JAG contest types in the codebase..."
rg -g '!test_cases' "JAG(Prelim|Regional)"
# Verify URL format consistency with AOJ API
echo "Verifying URL format in AOJ API related files..."
rg -g '*.ts' "AOJ_TASKS_URL.*task"
Length of output: 630
Script:
#!/bin/bash
# Check for JAG contest-related implementations
echo "Checking JAG contest implementations..."
rg "JAG.*contest" -A 5
# Look for AOJ URL constants and their usage
echo "Checking AOJ URL constants..."
rg "AOJ_.*URL.*=.*" -A 2
# Check for any other JAG-related patterns
echo "Looking for additional JAG patterns..."
rg "JAG.*\.(Prelim|Regional)" -A 3
Length of output: 7886
prisma/schema.prisma (1)
227-227: LGTM! Verify enum value handling in the application.
The addition of AOJ_JAG to ContestType is well-placed and properly documented. The enum value follows the established naming convention and is logically grouped with other AOJ-related types.
Let's verify the handling of this new enum value across the codebase:
✅ Verification successful
LGTM! The new AOJ_JAG enum value is properly integrated.
The verification shows that the new AOJ_JAG contest type is well-integrated across the codebase:
- Properly added to the
ContestTypeenum insrc/lib/types/contest.ts - Correctly handled in contest classification logic in
src/lib/utils/contest.ts - Test cases are implemented in
src/test/lib/utils/test_cases/contest_type.ts - Contest priority is properly set in the priority map (priority 20)
- Pattern matching for JAG contests is implemented (
/^JAG(Prelim|Regional|Summer|Winter|Spring)\d*$/) - Test data structure exists in
contest_name_and_task_index.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper handling of the new AOJ_JAG contest type
# across the codebase
# Check for enum usage in TypeScript/JavaScript files
echo "Checking for ContestType enum usage:"
rg -t typescript -t javascript "ContestType|AOJ_JAG" -A 3
# Check for any hardcoded contest type lists that might need updating
echo "Checking for potential places that need updates:"
rg -t typescript -t javascript "AOJ_PCK|AOJ_COURSES" -A 3
Length of output: 379
Script:
#!/bin/bash
# Let's try with just the file extension patterns instead of file type flags
echo "Checking for ContestType enum usage:"
rg "ContestType|AOJ_JAG" --glob "*.{ts,js,tsx,jsx}" -A 3
echo -e "\nChecking for potential places that need updates:"
rg "AOJ_PCK|AOJ_COURSES" --glob "*.{ts,js,tsx,jsx}" -A 3
Length of output: 50730
src/lib/utils/task.ts (1)
33-37: LGTM! Verify JAG contest ID patterns
The implementation correctly extends the canHandle method to support JAG contests. The code is clean and follows the existing pattern.
Let's verify the JAG contest ID patterns to ensure comprehensive coverage:
✅ Verification successful
Let me check the contest ID pattern more precisely by looking at the regex pattern used in the codebase.
JAG contest ID pattern is well-defined and verified
The implementation is correct and complete. The codebase shows that:
- JAG contest IDs follow a strict pattern:
^JAG(Prelim|Regional|Summer|Winter|Spring)\d*$ - Test cases cover both Prelim and Regional contests extensively
- The simple
startsWith('JAG')check in thecanHandlemethod is sufficient as other validation is handled by the contest type detection logic incontest.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any JAG-related contest IDs in the codebase to verify the pattern
# and ensure we're not missing any edge cases
# Search for JAG-related patterns in test files and source code
rg -i "jag.*contest" --type ts
rg "contestId.*['\"]JAG" --type ts
Length of output: 9655
Script:
#!/bin/bash
# Check for any regex patterns or validations for JAG contest IDs
ast-grep --pattern 'if ($_ /^JAG$$$/.exec($_)) {
$$$
}'
Length of output: 59
Script:
#!/bin/bash
# Let's try a different pattern to find JAG-related regex validations
rg "JAG.*\{" -A 2 --type ts
Length of output: 9669
src/test/lib/utils/test_cases/contest_type.ts (1)
347-368: LGTM! Well-structured contest data
The aojJagContestData follows consistent naming patterns and includes a comprehensive set of both preliminary and regional contests. The data structure aligns well with other contest type definitions in the file.
src/test/lib/utils/test_cases/contest_name_labels.ts (1)
359-444: Well-structured implementation with consistent naming patterns
The test cases follow consistent naming patterns for both contest IDs and expected labels, maintaining the same structure as other contest types in the file. The implementation effectively supports the PR objective of enabling JAG task imports.
src/test/lib/utils/test_cases/contest_name_and_task_index.ts (3)
420-467: LGTM! Well-structured test data with clear documentation.
The test data follows consistent patterns and provides good coverage for both preliminary and regional rounds across multiple years.
502-504: LGTM! Export implementation is consistent.
The export statement follows the established pattern and correctly flattens the test cases array.
469-484: Verify completeness of JAG contest years.
The JagYear type includes years from 2005 to 2024, but several years are missing (2007-2008, 2012-2016, 2018-2019). Please verify if these gaps are intentional or if we should include test cases for these years.
Run the following script to check for any JAG contests in the missing years:
✅ Verification successful
Add missing JAG contest years to the type definition
Several JAG contest years found in the codebase are not included in the JagYear type:
- 2012-2015: Found in
atcoder_problems/tasks.jsonwith contest IDs likejag2012autumn,jag2013summer-day3, etc. - 2016: Found in test cases with
JAGRegional2016
Let's verify if these contests follow the same Prelim or Regional pattern before suggesting their addition.
The JagYear type definition is correct as is
The missing years in the JagYear type appear to be intentional. While there are JAG contests from 2012-2016 in the codebase, they follow different naming patterns (like jag2012autumn, jag2013summer-day3) except for JAGRegional2016. The JagYear type is specifically used with JagContestId type that only accepts ${JagRound}${JagYear} format where JagRound is either 'Prelim' or 'Regional'. The search results confirm that contests from missing years don't follow this specific format, except for 2016 which is already included.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for JAG contest problems from missing years
# Expected: No results if the gaps are intentional
missing_years=(2007 2008 2012 2013 2014 2015 2016 2018 2019)
for year in "${missing_years[@]}"; do
echo "Checking year $year:"
rg -i "JAG.*${year}" || true
done
Length of output: 2221
Script:
#!/bin/bash
# Check the contest format for these years
rg -i "jag.*201[2-6]" -A 2 src/test/lib/clients/test_data/atcoder_problems/tasks.json
rg -i "JAG(Prelim|Regional)201[2-6]" src/test/lib/utils/test_cases/
Length of output: 1037
src/test/lib/utils/task.test.ts (1)
81-88: LGTM! Test suite follows established patterns.
The new test suite for AOJ JAG contests is well-structured and consistent with existing test patterns.
Let's verify the test cases are properly defined:
✅ Verification successful
Test cases for AOJ JAG are properly defined and well-structured
The verification shows that:
- Test cases are properly defined in
src/test/lib/utils/test_cases/task_url.ts - The implementation includes both JAG Preliminary and Regional contests
- Test data is generated using
createTestCaseForTaskUrlhelper - Test cases follow the expected structure with
contestId,taskId, andexpectedURL - The test suite is consistently used across multiple test files for different functionalities (contest type, task URL, contest name)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test cases for AOJ JAG contests
# Check if test cases file exists and contains AOJ JAG test cases
rg -A 5 "aojJag.*=.*\[" src/test/lib/utils/test_cases/task_url.ts
# Verify test case structure matches the type definition
ast-grep --pattern 'interface TestCaseForTaskUrl {
$$$
contestId: string;
taskId: string;
expected: string;
$$$
}'
Length of output: 194
Script:
#!/bin/bash
# Let's try a different approach to locate and verify the test cases
# First, find the test cases file
fd "task_url.ts" --type f
# Then check its content for AOJ JAG related test cases
rg "aojJag" --type ts -C 5
# Also verify the interface definition
rg "interface TestCaseForTaskUrl" --type ts -C 5
Length of output: 8401
src/test/lib/utils/contest.test.ts (3)
625-635: LGTM! Task index tests are well-structured.
The test implementation for adding contest names to task indices follows the established pattern and maintains consistency with other AOJ contest types.
341-347: LGTM! Priority tests maintain consistency.
The priority tests for AOJ JAG contests align well with the existing test patterns and verify the contest priority mapping.
Let's verify the priority mapping is defined:
✅ Verification successful
Priority mapping for AOJ JAG contests is properly defined and consistent
The verification confirms that:
- AOJ_JAG is defined as a contest type in
src/lib/types/contest.ts - The priority for AOJ_JAG is set to 20 in the
contestTypePrioritiesMap insrc/lib/utils/contest.ts - The contest type is correctly identified through regex pattern
/^JAG(Prelim|Regional|Summer|Winter|Spring)\d*$/ - Test cases properly verify the priority mapping using
getContestPriority()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if AOJ_JAG priority is defined in the contest priorities
rg "AOJ_JAG.*contestTypePriorities" ./src/lib/utils/
Length of output: 54
Script:
#!/bin/bash
# Let's first check the contest type definitions
rg "AOJ_JAG" ./src -A 3 -B 3
# Then check contest priorities separately
rg "contestTypePriorities" ./src -A 3 -B 3
Length of output: 16921
175-181: LGTM! Test structure follows established patterns.
The test cases for AOJ JAG contests are well-organized and consistent with the existing test structure.
Let's verify the test cases are comprehensive:
✅ Verification successful
Test cases for AOJ JAG contests are comprehensive and well-structured
The test cases in contest_type.ts and contest_name_labels.ts cover both JAG Preliminary (2005-2024) and Regional (2005-2022) contests, with proper contest IDs and expected labels. The test structure follows the established patterns and maintains consistency with other contest types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all AOJ JAG test cases are covered
rg -A 2 "aojJag.*forEach" ./src/test/lib/utils/test_cases/
Length of output: 60
Script:
#!/bin/bash
# Let's first find the test cases file
fd "test_cases" --type f
# Then let's look for AOJ JAG related constants or test cases
rg -l "aojJag"
Length of output: 308
Script:
#!/bin/bash
# Let's examine the test cases for AOJ JAG contests
cat src/test/lib/utils/test_cases/contest_type.ts
cat src/test/lib/utils/test_cases/contest_name_labels.ts
cat src/test/lib/utils/test_cases/contest_name_and_task_index.ts
Length of output: 38425
src/lib/utils/contest.ts (5)
84-86: The AOJ_JAG contest type is properly classified.
The regular expression correctly matches contest IDs starting with JAG followed by Prelim, Regional, Summer, Winter, or Spring, and any subsequent digits. The classification logic is consistent with existing patterns.
227-227: AOJ_JAG added to contestTypePriorities with appropriate priority.
Assigning a priority of 20 to ContestType.AOJ_JAG aligns with the existing priority structure, placing it after AOJ_PCK and maintaining the intended ordering.
279-283: Correctly handle JAG contests in getContestNameLabel.
The addition of the condition for contest IDs starting with JAG and utilizing getAojChallengeLabel with JAG_TRANSLATIONS ensures that JAG contests are properly labeled.
289-293: PCK_TRANSLATIONS defined correctly for label translations.
The translation mappings for PCK contests are accurate, enabling proper substitution in contest labels.
295-298: JAG_TRANSLATIONS defined correctly for label translations.
The translation mappings for JAG contests are accurately defined, facilitating correct label generation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/lib/utils/contest.ts (3)
227-227: Update priority documentationThe priority for
AOJ_JAG(20) is higher thanAOJ_PCK(19), but this isn't reflected in the documentation comment above. Consider updating the comment to include the rationale for AOJ_JAG's priority.* Priority assignment rationale: * - Educational contests (0-10): ABS, ABC, APG4B, etc. * - Contests for genius (11-15): ARC, AGC, and their variants * - Special contests (16-17): UNIVERSITY, OTHERS - * - External platforms (18-19): AOJ_COURSES, AOJ_PCK + * - External platforms (18-20): AOJ_COURSES, AOJ_PCK, AOJ_JAG
295-298: Add JSDoc documentationConsider adding JSDoc documentation for
JAG_TRANSLATIONSsimilar to other translation constants in the file.+/** + * Maps JAG contest type abbreviations to their Japanese translations. + * + * @example + * { + * Prelim: '模擬国内予選', + * Regional: '模擬アジア地区予選' + * } + */ const JAG_TRANSLATIONS = { Prelim: '模擬国内予選', Regional: '模擬アジア地区予選', };
300-308: Enhance type safetyThe function implementation is clean and reusable. Consider enhancing type safety by adding a return type annotation and making the translations parameter readonly.
-function getAojChallengeLabel(translations: ContestLabelTranslations, contestId: string): string { +function getAojChallengeLabel(translations: Readonly<ContestLabelTranslations>, contestId: string): string { const baseLabel = 'AOJ - '; let label = contestId; Object.entries(translations).forEach(([abbrEnglish, japanese]) => { label = label.replace(abbrEnglish, japanese); }); return baseLabel + label; }src/lib/clients/aizu_online_judge.ts (3)
129-144: Consider enhancing the logging message with contest type details.While the implementation is solid, the final log message could be more informative by breaking down the count by contest type.
- console.log(`Found AOJ: ${contests.length} contests.`); + console.log( + `Found AOJ contests - Total: ${contests.length} ` + + `(Courses: ${courses.length}, PCK: ${pckPrelims.length + pckFinals.length}, ` + + `JAG: ${jagPrelims.length + jagRegionals.length})` + );
Line range hint
191-236: Consider caching API responses for frequently accessed contests.The implementation is robust, but consider adding caching for the API responses to improve performance and reduce API calls. This would be particularly beneficial if the contests data is accessed frequently.
Consider implementing a simple in-memory cache:
private contestCache: Map<string, ContestsForImport> = new Map(); private async fetchChallengeContests<T extends ChallengeContestType>( contestType: T, round: ChallengeRoundMap[T], ): Promise<ContestsForImport> { const cacheKey = `${contestType}_${round}`; if (this.contestCache.has(cacheKey)) { return this.contestCache.get(cacheKey)!; } // ... existing implementation ... this.contestCache.set(cacheKey, contests); return contests; }
Line range hint
266-288: Enhance logging message with task type details.Similar to the contests logging, the tasks logging could be more informative by breaking down the count by task type.
- console.log(`Found AOJ: ${tasks.length} tasks.`); + console.log( + `Found AOJ tasks - Total: ${tasks.length} ` + + `(Courses: ${courses.length}, PCK: ${pckPrelims.length + pckFinals.length}, ` + + `JAG: ${jagPrelims.length + jagRegionals.length})` + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/lib/clients/aizu_online_judge.ts(10 hunks)src/lib/utils/contest.ts(4 hunks)src/test/lib/utils/test_cases/contest_name_and_task_index.ts(1 hunks)src/test/lib/utils/test_cases/contest_name_labels.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/lib/utils/test_cases/contest_name_labels.ts
🔇 Additional comments (8)
src/lib/utils/contest.ts (2)
282-283: LGTM!
The JAG contest label handling follows the same pattern as PCK contests and correctly uses the new generic getAojChallengeLabel function.
84-86:
Verify handling of seasonal JAG contests
The regex pattern includes Summer, Winter, and Spring variants, but these aren't covered in the JAG_TRANSLATIONS. This might lead to inconsistent labeling for seasonal JAG contests.
Consider either:
- Removing the seasonal variants if they're not needed, or
- Adding translations for them in
JAG_TRANSLATIONS:
const JAG_TRANSLATIONS = {
Prelim: '模擬国内予選',
Regional: '模擬アジア地区予選',
+ Summer: '夏季',
+ Winter: '冬季',
+ Spring: '春季',
};src/lib/clients/aizu_online_judge.ts (2)
Line range hint 33-109: LGTM! Well-structured type definitions with proper type safety.
The new enums and type definitions are well-documented and provide excellent type safety through the ChallengeRoundMap type. This ensures that only valid combinations of contest types and rounds can be used.
Line range hint 353-399: Enhance error handling and consider caching API responses.
While the implementation is solid, consider these improvements:
- Add caching similar to the suggested improvement for
fetchChallengeContests - Verify that API errors are properly propagated and handled by the caller
✅ Verification successful
Let me verify the caching patterns to provide a complete response.
Let me check for any caching implementations using a different pattern.
Error handling is properly implemented, but caching could be beneficial
Based on the error handling patterns found in the codebase:
- The method follows the consistent error handling pattern used throughout the codebase
- Errors are properly logged with context and propagated up
- The error messages are descriptive and include the contest type and round information
Adding caching would indeed be beneficial to:
- Reduce API calls for frequently accessed contest data
- Improve response times for repeated requests
- Reduce load on the AOJ API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check error handling patterns in the codebase
rg -A 2 "catch \(error\)" | grep -v "return \[\]"
Length of output: 6779
Script:
#!/bin/bash
# Check caching patterns in the codebase
rg -A 5 "private\s+cache" src/lib/clients/
Length of output: 44
Script:
#!/bin/bash
# Check for any caching implementations or memoization
rg -A 5 "cache|memoize|memo" src/lib/clients/
Length of output: 47
src/test/lib/utils/test_cases/contest_name_and_task_index.ts (4)
420-470: LGTM! Well-structured test data with comprehensive coverage.
The test data structure is well-organized and properly documented, covering both preliminary and regional rounds across different years with clear task ID patterns.
491-504: LGTM! Well-designed generic function.
The generateContestTestCases function is a great example of code reuse and abstraction. It effectively generalizes the test case generation pattern while maintaining type safety.
506-513: LGTM! Clean implementation of JAG-specific test case generator.
The generateAojJagTestCases function effectively utilizes the generic generator while handling JAG-specific formatting requirements.
515-517: LGTM! Consistent export implementation.
The export follows the established pattern in the file, making it easy to understand and maintain.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/lib/utils/contest.ts (2)
289-307: Add JSDoc documentation for PCK_TRANSLATIONSFor consistency, consider adding similar JSDoc documentation to
PCK_TRANSLATIONSas provided forJAG_TRANSLATIONS.+/** + * Maps PCK contest type abbreviations to their Japanese translations. + * + * @example + * { + * PCK: 'パソコン甲子園', + * Prelim: '予選', + * Final: '本選' + * } + */ const PCK_TRANSLATIONS = { PCK: 'パソコン甲子園', Prelim: '予選', Final: '本選', };
309-320: Consider moving constant outside function scopeThe
baseLabelconstant is recreated on every function call but its value never changes.+const AOJ_BASE_LABEL = 'AOJ - '; + function getAojChallengeLabel( translations: Readonly<ContestLabelTranslations>, contestId: string, ): string { - const baseLabel = 'AOJ - '; let label = contestId; Object.entries(translations).forEach(([abbrEnglish, japanese]) => { label = label.replace(abbrEnglish, japanese); }); - return baseLabel + label; + return AOJ_BASE_LABEL + label; }src/lib/clients/aizu_online_judge.ts (3)
117-130: Consider implementing a cache invalidation strategyWhile the cache implementation is clean and well-documented, it lacks a cache invalidation mechanism. This could potentially lead to:
- Memory leaks in long-running applications
- Stale data if the upstream API is updated
Consider implementing one of these strategies:
- Time-based expiration
- LRU (Least Recently Used) cache
- Maximum cache size limit
const contestCache: Map<string, ContestsForImport> = new Map(); +const CACHE_TTL = 3600000; // 1 hour in milliseconds +type CacheEntry<T> = { + data: T; + timestamp: number; +}; + +function getCacheEntry<T>(cache: Map<string, CacheEntry<T>>, key: string): T | undefined { + const entry = cache.get(key); + if (!entry) return undefined; + + if (Date.now() - entry.timestamp > CACHE_TTL) { + cache.delete(key); + return undefined; + } + + return entry.data; +}
151-164: Enhance error handling for better debuggingWhile the concurrent fetching is efficient, the error handling could be more specific to help with debugging issues. Consider:
- Logging which specific fetch operation failed
- Preserving partial results if only some fetches fail
- const [courses, pckPrelims, pckFinals, jagPrelims, jagRegionals] = await Promise.all([ - this.fetchCourseContests(), - this.fetchChallengeContests(ChallengeContestType.PCK, PckRound.PRELIM), - this.fetchChallengeContests(ChallengeContestType.PCK, PckRound.FINAL), - this.fetchChallengeContests(ChallengeContestType.JAG, JagRound.PRELIM), - this.fetchChallengeContests(ChallengeContestType.JAG, JagRound.REGIONAL), - ]); + const results = await Promise.allSettled([ + { type: 'courses', promise: this.fetchCourseContests() }, + { type: 'pck_prelim', promise: this.fetchChallengeContests(ChallengeContestType.PCK, PckRound.PRELIM) }, + { type: 'pck_final', promise: this.fetchChallengeContests(ChallengeContestType.PCK, PckRound.FINAL) }, + { type: 'jag_prelim', promise: this.fetchChallengeContests(ChallengeContestType.JAG, JagRound.PRELIM) }, + { type: 'jag_regional', promise: this.fetchChallengeContests(ChallengeContestType.JAG, JagRound.REGIONAL) }, + ].map(async ({ type, promise }) => { + try { + return { type, data: await promise }; + } catch (error) { + console.error(`Failed to fetch ${type} contests:`, error); + return { type, data: [] }; + } + })); + + const contests = results.flatMap(result => result.data);
Line range hint
224-264: Consider making endpoint construction more robustThe endpoint construction could be made more robust by:
- Using URL path joining to prevent double slashes
- URL encoding the path segments
+ private buildEndpoint(segments: string[]): string { + return segments + .map(segment => encodeURIComponent(segment)) + .join('/'); + } private async fetchChallengeContests<T extends ChallengeContestType>( contestType: T, round: ChallengeRoundMap[T], ): Promise<ContestsForImport> { // ... const results = await this.fetchApiWithConfig<AOJChallengeContestAPI>({ baseApiUrl: AOJ_API_BASE_URL, - endpoint: `challenges/cl/${contestType}/${round}`, + endpoint: this.buildEndpoint(['challenges', 'cl', contestType, round]), // ... });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/lib/clients/aizu_online_judge.ts(10 hunks)src/lib/utils/contest.ts(5 hunks)
🔇 Additional comments (5)
src/lib/utils/contest.ts (2)
197-197: LGTM! Priority assignment follows documented grouping
The priority assignment for AOJ_JAG (20) aligns with the documented grouping for external platforms (18-20).
Also applies to: 227-227
279-283: LGTM! Good refactoring for code reuse
The shared getAojChallengeLabel function effectively handles both PCK and JAG contest labels while preventing accidental modifications to translations through Readonly.
src/lib/clients/aizu_online_judge.ts (3)
Line range hint 33-109: Well-structured type definitions with proper type safety!
The implementation of ChallengeRoundMap type effectively ensures type safety between contest types and their corresponding rounds, preventing invalid combinations at compile time.
309-321: Similar improvements as getContests method apply here
Line range hint 398-440: Similar improvements as fetchChallengeContests method apply here
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/lib/utils/contest.ts (2)
279-283: Add validation for contest ID prefixWhile the code works correctly when the prefix matches, it's good practice to validate the prefix before processing. Consider adding validation:
if (contest_id.startsWith('PCK')) { + if (!/^PCK/.test(contest_id)) { + return contest_id.toUpperCase(); + } return getAojChallengeLabel(PCK_TRANSLATIONS, contest_id); } if (contest_id.startsWith('JAG')) { + if (!/^JAG/.test(contest_id)) { + return contest_id.toUpperCase(); + } return getAojChallengeLabel(JAG_TRANSLATIONS, contest_id); }
321-331: Enhance type safety and error handlingConsider these improvements to make the function more robust:
- Use a more specific type for translations:
- translations: Readonly<ContestLabelTranslations>, + translations: Readonly<Record<string, string>>,
- Add error handling for missing translations:
function getAojChallengeLabel( translations: Readonly<Record<string, string>>, contestId: string, ): string { let label = contestId; + const matchedTranslation = Object.entries(translations).find( + ([abbrEnglish]) => contestId.includes(abbrEnglish) + ); + + if (!matchedTranslation) { + return aojBaseLabel + contestId; + } - Object.entries(translations).forEach(([abbrEnglish, japanese]) => { - label = label.replace(abbrEnglish, japanese); - }); + const [abbrEnglish, japanese] = matchedTranslation; + label = label.replace(abbrEnglish, japanese); return aojBaseLabel + label; }src/lib/clients/aizu_online_judge.ts (3)
117-186: Consider moving cache to instance levelThe cache maps (
contestCacheandtaskCache) are currently defined at module level. This could lead to memory leaks in long-running applications as the cache would be shared across all instances ofAojApiClient.Consider moving the cache to instance level:
export class AojApiClient extends ContestSiteApiClient { + private readonly contestCache: Map<string, CacheEntry<ContestsForImport>> = new Map(); + private readonly taskCache: Map<string, CacheEntry<TasksForImport>> = new Map();
Line range hint
266-323: Enhance error logging for better debuggingWhile the implementation is solid, the error logging could be more specific to help with debugging.
Consider enhancing the error logging:
- console.error(`Failed to fetch from AOJ ${contestTypeLabel} ${round} contests:`, error); + console.error( + `Failed to fetch from AOJ ${contestTypeLabel} ${round} contests:`, + error instanceof Error ? error.message : error + );
326-334: Add input validation to buildEndpoint methodThe method should validate its input to prevent potential issues with undefined or null segments.
Consider adding input validation:
private buildEndpoint(segments: string[]): string { + if (!segments?.length) { + throw new Error('Endpoint segments array cannot be empty'); + } return segments.map((segment) => encodeURIComponent(segment)).join('/'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/lib/clients/aizu_online_judge.ts(10 hunks)src/lib/utils/contest.ts(5 hunks)
🔇 Additional comments (6)
src/lib/utils/contest.ts (2)
84-86: Previous review comment is still applicable
197-197: LGTM! Priority assignment follows the documented rationale
The AOJ_JAG priority (20) is correctly placed in the external platforms category (18-20) alongside other AOJ contests.
Also applies to: 227-227
src/lib/clients/aizu_online_judge.ts (4)
Line range hint 33-109: Well-structured type system implementation!
The type definitions and enums create a robust type-safe relationship between contest types and their corresponding rounds. The ChallengeRoundMap type effectively prevents mismatches between contest types and rounds.
200-220: LGTM! Efficient implementation with concurrent fetching
The method effectively uses Promise.all for concurrent fetching of different contest types, with clear and informative logging.
Line range hint 361-388: LGTM! Consistent implementation with getContests
The method follows the same efficient pattern as getContests, using concurrent fetching and providing clear logging.
Line range hint 452-509: Apply consistent error logging enhancement
Similar to the suggestion for fetchChallengeContests, the error logging here could be more specific.
Apply the same error logging enhancement as suggested for fetchChallengeContests.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/lib/clients/aizu_online_judge.ts (4)
117-168: Consider implementing a more robust cache invalidation strategy.While the current cache implementation with TTL is good, consider adding:
- A maximum cache size to prevent memory issues
- A method to manually invalidate the cache when needed
- Error handling for cache operations
+const MAX_CACHE_SIZE = 1000; // Adjust based on your needs +class Cache<T> { + private cache: Map<string, CacheEntry<T>> = new Map(); + + set(key: string, data: T): void { + if (this.cache.size >= MAX_CACHE_SIZE) { + const oldestKey = this.findOldestEntry(); + if (oldestKey) this.cache.delete(oldestKey); + } + this.cache.set(key, { data, timestamp: Date.now() }); + } + + private findOldestEntry(): string | undefined { + let oldestKey: string | undefined; + let oldestTime = Infinity; + + for (const [key, entry] of this.cache.entries()) { + if (entry.timestamp < oldestTime) { + oldestTime = entry.timestamp; + oldestKey = key; + } + } + + return oldestKey; + } +}
207-220: Consider adding retry logic for failed API requests.The concurrent fetching of contests is well implemented, but network failures could benefit from retry logic.
+const MAX_RETRIES = 3; +const RETRY_DELAY = 1000; // ms +private async withRetry<T>( + operation: () => Promise<T>, + retries = MAX_RETRIES +): Promise<T> { + try { + return await operation(); + } catch (error) { + if (retries > 0) { + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); + return this.withRetry(operation, retries - 1); + } + throw error; + } +}
473-478: Consider adding cache warmup mechanism.The cache implementation could benefit from a warmup mechanism to pre-populate frequently accessed data.
+interface CacheConfig { + warmupEnabled: boolean; + warmupKeys: string[]; +} + +private async warmupCache(): Promise<void> { + const warmupKeys = [ + `${ChallengeContestType.PCK}_${PckRound.PRELIM}`, + `${ChallengeContestType.PCK}_${PckRound.FINAL}`, + `${ChallengeContestType.JAG}_${JagRound.PRELIM}`, + `${ChallengeContestType.JAG}_${JagRound.REGIONAL}` + ]; + + await Promise.all( + warmupKeys.map(key => this.fetchChallengeTasks( + key.split('_')[0] as ChallengeContestType, + key.split('_')[1] as ChallengeRoundMap[ChallengeContestType] + )) + ); +}
486-487: Add rate limiting to prevent API abuse.Consider implementing rate limiting for API requests to avoid overwhelming the AOJ API.
+private rateLimiter = new RateLimiter({ + tokensPerInterval: 10, + interval: 'second', +}); private async fetchApiWithConfig<T>(...): Promise<T> { + await this.rateLimiter.removeTokens(1); // existing code }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/lib/clients/aizu_online_judge.ts(10 hunks)
🔇 Additional comments (2)
src/lib/clients/aizu_online_judge.ts (2)
33-39: LGTM! Well-structured enum for challenge contest types.
The enum provides a clear and type-safe way to distinguish between PCK and JAG contest types.
94-109: LGTM! Excellent type safety implementation.
The combination of JagRound enum and ChallengeRoundMap type ensures type safety between contest types and their corresponding rounds at compile time.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/lib/clients/aizu_online_judge.ts (3)
117-208: Consider making cache parameters configurableWhile the cache implementation is well-designed, consider making the
CACHE_TTLandMAX_CACHE_SIZEconfigurable through the constructor to enhance flexibility for different use cases.class Cache<T> { private cache: Map<string, CacheEntry<T>> = new Map(); + constructor( + private readonly ttl: number = CACHE_TTL, + private readonly maxSize: number = MAX_CACHE_SIZE + ) {} set(key: string, data: T): void { - if (this.cache.size >= MAX_CACHE_SIZE) { + if (this.cache.size >= this.maxSize) { const oldestKey = this.findOldestEntry(); if (oldestKey) { this.cache.delete(oldestKey); } } this.cache.set(key, { data, timestamp: Date.now() }); } get(key: string): T | undefined { const entry = this.cache.get(key); if (!entry) { return undefined; } - if (Date.now() - entry.timestamp > CACHE_TTL) { + if (Date.now() - entry.timestamp > this.ttl) { this.cache.delete(key); return undefined; } return entry.data; }
244-264: Enhance error handling granularityWhile the concurrent fetching is well-implemented, consider catching errors for each contest type separately to prevent a single failure from affecting all contest types.
-const [courses, pckPrelims, pckFinals, jagPrelims, jagRegionals] = await Promise.all([ - this.fetchCourseContests(), - this.fetchChallengeContests(ChallengeContestType.PCK, PckRound.PRELIM), - this.fetchChallengeContests(ChallengeContestType.PCK, PckRound.FINAL), - this.fetchChallengeContests(ChallengeContestType.JAG, JagRound.PRELIM), - this.fetchChallengeContests(ChallengeContestType.JAG, JagRound.REGIONAL), -]); +const results = await Promise.allSettled([ + { type: 'courses', promise: this.fetchCourseContests() }, + { type: 'pck_prelim', promise: this.fetchChallengeContests(ChallengeContestType.PCK, PckRound.PRELIM) }, + { type: 'pck_final', promise: this.fetchChallengeContests(ChallengeContestType.PCK, PckRound.FINAL) }, + { type: 'jag_prelim', promise: this.fetchChallengeContests(ChallengeContestType.JAG, JagRound.PRELIM) }, + { type: 'jag_regional', promise: this.fetchChallengeContests(ChallengeContestType.JAG, JagRound.REGIONAL) }, +].map(async ({ type, promise }) => { + try { + return await promise; + } catch (error) { + console.error(`Failed to fetch ${type} contests:`, error); + return []; + } +}));
Line range hint
420-447: Consider reducing code duplication with contest fetchingThe task fetching logic closely mirrors the contest fetching logic. Consider extracting the common pattern into a reusable utility function to reduce duplication and maintain consistency.
+private async fetchAllTypes<T>( + fetchFn: (type: ChallengeContestType, round: ChallengeRoundMap[ChallengeContestType]) => Promise<T[]>, + baseFn: () => Promise<T[]> +): Promise<T[]> { + const results = await Promise.allSettled([ + { type: 'courses', promise: baseFn() }, + { type: 'pck_prelim', promise: fetchFn(ChallengeContestType.PCK, PckRound.PRELIM) }, + { type: 'pck_final', promise: fetchFn(ChallengeContestType.PCK, PckRound.FINAL) }, + { type: 'jag_prelim', promise: fetchFn(ChallengeContestType.JAG, JagRound.PRELIM) }, + { type: 'jag_regional', promise: fetchFn(ChallengeContestType.JAG, JagRound.REGIONAL) }, + ].map(async ({ type, promise }) => { + try { + return await promise; + } catch (error) { + console.error(`Failed to fetch ${type}:`, error); + return []; + } + })); + return results.flat(); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/lib/clients/aizu_online_judge.ts(10 hunks)
🔇 Additional comments (2)
src/lib/clients/aizu_online_judge.ts (2)
Line range hint 33-109: Well-structured type definitions with proper type safety!
The implementation of ChallengeRoundMap effectively ensures type safety between contest types and their corresponding rounds, preventing invalid combinations at compile-time.
370-393: Robust URL endpoint builder with proper validation!
The implementation includes proper input validation and URL encoding, effectively preventing potential security issues with malformed URLs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/lib/clients/aizu_online_judge.ts (4)
117-219: Consider adding cache invalidation mechanism.While the cache implementation is solid with TTL and size limits, consider adding a method to manually invalidate the cache when needed (e.g., for testing or when data is known to be stale).
class Cache<T> { private cache: Map<string, CacheEntry<T>> = new Map(); + /** + * Clears all entries from the cache. + */ + clear(): void { + this.cache.clear(); + }
262-275: Consider adding error type information.The error logging could be more informative by including the error type and message.
- console.log( + console.info( `Found AOJ contests - Total: ${contests.length} ` + `(Courses: ${courses.length}, PCK: ${pckPrelims.length + pckFinals.length}, ` + `JAG: ${jagPrelims.length + jagRegionals.length})`, ); } catch (error) { - console.error(`Failed to fetch contests from AOJ API:`, error); + console.error(`Failed to fetch contests from AOJ API:`, { + error: error instanceof Error ? error.message : String(error), + type: error?.constructor?.name + });
387-404: Consider strengthening URL segment validation.While the current validation is good, consider adding length limits and additional checks for potential security issues.
private buildEndpoint(segments: string[]): string { if (!segments?.length) { throw new Error('Endpoint segments array cannot be empty'); } // Allow alphanumeric characters, hyphens, and underscores const validateSegment = (segment: string): boolean => { - return /^[a-zA-Z0-9-_]+$/.test(segment); + const MAX_SEGMENT_LENGTH = 100; + return segment.length <= MAX_SEGMENT_LENGTH && + /^[a-zA-Z0-9-_]+$/.test(segment) && + !segment.includes('..'); }; for (const segment of segments) { if (!validateSegment(segment)) { - throw new Error(`Invalid segment: ${segment}`); + throw new Error(`Invalid segment: ${segment}. Segments must be alphanumeric with hyphens and underscores, max length ${MAX_SEGMENT_LENGTH}`); } } return segments.map((segment) => encodeURIComponent(segment)).join('/'); }
446-458: Consider adding retry mechanism for API calls.The concurrent API calls could benefit from a retry mechanism to handle temporary failures.
Consider implementing a retry mechanism with exponential backoff for the Promise.all calls to handle temporary API failures gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/lib/clients/aizu_online_judge.ts(10 hunks)
🔇 Additional comments (5)
src/lib/clients/aizu_online_judge.ts (5)
33-39: LGTM! Well-structured enum definition.
The ChallengeContestType enum is well-defined and follows TypeScript best practices.
91-109: LGTM! Type-safe contest round mapping.
The combination of JagRound enum and ChallengeRoundMap type creates a type-safe relationship between contest types and their rounds, preventing invalid combinations at compile time.
230-250: LGTM! Well-documented cache properties.
The cache properties are well-documented with clear explanations of their purpose and types.
335-338: LGTM! Type-safe generic implementation.
The generic type parameter T extends ChallengeContestType with ChallengeRoundMap[T] ensures type safety between contest types and their corresponding rounds.
535-544: LGTM! Efficient caching implementation.
The caching implementation for challenge tasks is well-structured with proper type safety and cache key generation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/lib/clients/aizu_online_judge.ts (4)
117-226: Consider enhancing cache error handlingWhile the cache implementation is robust, consider adding error handling for edge cases:
- Handle potential race conditions in concurrent operations
- Add error handling for invalid TTL or maxSize values in constructor
- Consider adding a method to check cache health/status
class Cache<T> { constructor( private readonly ttl: number = CACHE_TTL, private readonly maxSize: number = MAX_CACHE_SIZE, - ) {} + ) { + if (ttl <= 0) throw new Error('TTL must be positive'); + if (maxSize <= 0) throw new Error('Max size must be positive'); + } + public get size(): number { + return this.cache.size; + } + public get health(): { size: number; oldestEntry: number } { + const oldestEntry = Math.min(...Array.from(this.cache.values()).map(e => e.timestamp)); + return { size: this.cache.size, oldestEntry }; + }
Line range hint
262-385: Improve error handling in concurrent fetchingWhile using
Promise.allfor concurrent fetching is efficient, if any single request fails, it will cause the entire operation to fail. Consider usingPromise.allSettledto handle partial failures gracefully.- const [courses, pckPrelims, pckFinals, jagPrelims, jagRegionals] = await Promise.all([ + const results = await Promise.allSettled([ this.fetchCourseContests(), this.fetchChallengeContests(ChallengeContestType.PCK, PckRound.PRELIM), this.fetchChallengeContests(ChallengeContestType.PCK, PckRound.FINAL), this.fetchChallengeContests(ChallengeContestType.JAG, JagRound.PRELIM), this.fetchChallengeContests(ChallengeContestType.JAG, JagRound.REGIONAL), - ]); + ]); + + const [courses, pckPrelims, pckFinals, jagPrelims, jagRegionals] = results.map(result => + result.status === 'fulfilled' ? result.value : [] + );
388-418: Enhance error message clarityThe error message could be more specific about the validation rules. Consider creating a constant for the validation pattern and including it in the error message.
+ private static readonly SEGMENT_PATTERN = '^[a-zA-Z0-9-_]+$'; + private static readonly SEGMENT_PATTERN_DESC = 'alphanumeric characters, hyphens, and underscores'; private buildEndpoint(segments: string[]): string { // ... const validateSegment = (segment: string): boolean => { return ( segment.length <= MAX_SEGMENT_LENGTH && - /^[a-zA-Z0-9-_]+$/.test(segment) && + new RegExp(AojApiClient.SEGMENT_PATTERN).test(segment) && !segment.includes('..') ); }; // ... throw new Error( - `Invalid segment: ${segment}. Segments must be alphanumeric with hyphens and underscores, max length ${MAX_SEGMENT_LENGTH}`, + `Invalid segment: ${segment}. Segments must contain only ${AojApiClient.SEGMENT_PATTERN_DESC}, ` + + `be ${MAX_SEGMENT_LENGTH} characters or less, and not include path traversal sequences`, );
Line range hint
445-472: Reduce code duplication in logging messagesConsider extracting the logging logic into a helper method to reduce duplication and make it easier to maintain consistent logging format across the codebase.
+ private logEntityCount( + entity: 'tasks' | 'contests', + counts: { courses: number; pck: number; jag: number } + ): void { + console.info( + `Found AOJ ${entity} - Total: ${counts.courses + counts.pck + counts.jag} ` + + `(Courses: ${counts.courses}, PCK: ${counts.pck}, JAG: ${counts.jag})` + ); + } async getTasks(): Promise<TasksForImport> { try { // ... fetch tasks ... - console.info( - `Found AOJ tasks - Total: ${tasks.length} ` + - `(Courses: ${courses.length}, PCK: ${pckPrelims.length + pckFinals.length}, ` + - `JAG: ${jagPrelims.length + jagRegionals.length})`, - ); + this.logEntityCount('tasks', { + courses: courses.length, + pck: pckPrelims.length + pckFinals.length, + jag: jagPrelims.length + jagRegionals.length + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/lib/clients/aizu_online_judge.ts(10 hunks)
🔇 Additional comments (1)
src/lib/clients/aizu_online_judge.ts (1)
Line range hint 33-109: Well-structured type definitions with proper type safety!
The implementation of ChallengeContestType, JagRound, and ChallengeRoundMap provides excellent type safety and maintainability. The mapped type pattern in ChallengeRoundMap ensures that only valid combinations of contest types and rounds are possible.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/lib/clients/aizu_online_judge.ts (4)
117-122: Consider making cache configuration more flexible.The cache configuration values are hardcoded. Consider making them configurable through constructor parameters.
-const CACHE_TTL = 60 * 60 * 1000; // 1 hour in milliseconds -const MAX_CACHE_SIZE = 50; +const DEFAULT_CACHE_TTL = 60 * 60 * 1000; // 1 hour in milliseconds +const DEFAULT_MAX_CACHE_SIZE = 50;
380-386: Consider implementing cache key normalization.The cache key construction could benefit from normalization to prevent potential collisions.
- const cacheKey = `${contestType}_${round}`; + const cacheKey = `${contestType.toLowerCase()}_${round.toLowerCase()}`;Also applies to: 611-617
435-438: Consider using structured logging.The current logging implementation uses string concatenation. Consider using structured logging for better log aggregation and analysis.
- console.info( - `Found AOJ ${entity} - Total: ${counts.courses + counts.pck + counts.jag} ` + - `(Courses: ${counts.courses}, PCK: ${counts.pck}, JAG: ${counts.jag})`, - ); + console.info('Found AOJ entities', { + entity, + total: counts.courses + counts.pck + counts.jag, + breakdown: { + courses: counts.courses, + pck: counts.pck, + jag: counts.jag, + }, + });
Line range hint
594-650: Consider implementing retry mechanism.The fetch operations could benefit from a retry mechanism to handle temporary network issues.
Consider implementing an exponential backoff retry mechanism for API calls. Would you like me to provide an implementation example?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/lib/clients/aizu_online_judge.ts(10 hunks)
🔇 Additional comments (3)
src/lib/clients/aizu_online_judge.ts (3)
33-39: LGTM! Well-structured enums for contest types and rounds.
The enums are well-defined and follow TypeScript best practices with clear naming conventions.
Also applies to: 91-97
99-109: LGTM! Type-safe mapping between contest types and rounds.
The ChallengeRoundMap type effectively ensures type safety between contest types and their corresponding rounds.
267-288: LGTM! Well-implemented caching mechanism.
The cache implementation with TTL and size limits is well-designed and properly documented.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/lib/clients/aizu_online_judge.ts (4)
117-256: Consider implementing periodic cache cleanup.While the cache implementation is solid, it could benefit from a periodic cleanup mechanism to prevent potential memory leaks from expired entries that haven't been accessed.
class Cache<T> { private cache: Map<string, CacheEntry<T>> = new Map(); + private cleanupInterval: NodeJS.Timeout; constructor( private readonly timeToLive: number = DEFAULT_CACHE_TTL, private readonly maxSize: number = DEFAULT_MAX_CACHE_SIZE, ) { if (timeToLive <= 0) { throw new Error('TTL must be positive'); } if (maxSize <= 0) { throw new Error('Max size must be positive'); } + this.cleanupInterval = setInterval(() => this.cleanup(), timeToLive); } + private cleanup(): void { + const now = Date.now(); + for (const [key, entry] of this.cache.entries()) { + if (now - entry.timestamp > this.timeToLive) { + this.cache.delete(key); + } + } + } + + dispose(): void { + clearInterval(this.cleanupInterval); + this.cache.clear(); + }
299-321: Consider adding retry mechanism for failed fetches.The concurrent fetching implementation handles errors well by continuing with partial results, but critical endpoints might benefit from retry logic.
+const MAX_RETRIES = 3; +const RETRY_DELAY = 1000; // ms + +private async withRetry<T>( + operation: () => Promise<T>, + retries = MAX_RETRIES, + delay = RETRY_DELAY +): Promise<T> { + try { + return await operation(); + } catch (error) { + if (retries > 0) { + await new Promise(resolve => setTimeout(resolve, delay)); + return this.withRetry(operation, retries - 1, delay * 2); + } + throw error; + } +}
446-476: Enhance URL segment validation.While the current validation is good, consider adding more restrictions to prevent potential security issues.
const validateSegment = (segment: string): boolean => { return ( segment.length <= MAX_SEGMENT_LENGTH && - /^[a-zA-Z0-9][-_a-zA-Z0-9]*$/.test(segment) && + /^[a-zA-Z][a-zA-Z0-9][-_a-zA-Z0-9]*$/.test(segment) && !segment.includes('..') ); };
518-540: Consider implementing rate limiting.To prevent overwhelming the AOJ API, consider adding rate limiting for concurrent requests.
+import pLimit from 'p-limit'; + +const MAX_CONCURRENT_REQUESTS = 3; +const limit = pLimit(MAX_CONCURRENT_REQUESTS); + async getTasks(): Promise<TasksForImport> { try { const results = await Promise.allSettled([ - this.fetchCourseTasks(), - this.fetchChallengeTasks(ChallengeContestType.PCK, PckRound.PRELIM), - this.fetchChallengeTasks(ChallengeContestType.PCK, PckRound.FINAL), - this.fetchChallengeTasks(ChallengeContestType.JAG, JagRound.PRELIM), - this.fetchChallengeTasks(ChallengeContestType.JAG, JagRound.REGIONAL), + limit(() => this.fetchCourseTasks()), + limit(() => this.fetchChallengeTasks(ChallengeContestType.PCK, PckRound.PRELIM)), + limit(() => this.fetchChallengeTasks(ChallengeContestType.PCK, PckRound.FINAL)), + limit(() => this.fetchChallengeTasks(ChallengeContestType.JAG, JagRound.PRELIM)), + limit(() => this.fetchChallengeTasks(ChallengeContestType.JAG, JagRound.REGIONAL)), ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/lib/clients/aizu_online_judge.ts(10 hunks)
🔇 Additional comments (1)
src/lib/clients/aizu_online_judge.ts (1)
Line range hint 33-109: LGTM! Well-structured type definitions.
The new enums and type definitions are well-documented and provide good type safety for contest types and rounds.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/lib/clients/aizu_online_judge.ts (4)
33-39: LGTM! Consider enhancing enum documentation.The implementation of
ChallengeContestType,JagRound, andChallengeRoundMapprovides excellent type safety. The type mapping ensures that only valid combinations of contest types and rounds are possible.Consider adding
@deprecatedor@experimentalJSDoc tags to indicate the stability status of these new enums.Also applies to: 91-97, 99-109
117-280: Enhance cache cleanup mechanism.The cache implementation is robust with TTL and size limits. However, there are a few potential improvements:
- Consider using
WeakMapfor better memory management:- private cache: Map<string, CacheEntry<T>> = new Map(); + private cache: WeakMap<string, CacheEntry<T>> = new WeakMap();
- Add a maximum age limit to prevent stale data:
constructor( private readonly timeToLive: number = DEFAULT_CACHE_TTL, private readonly maxSize: number = DEFAULT_MAX_CACHE_SIZE, + private readonly maxAge: number = DEFAULT_MAX_AGE, ) {
Line range hint
332-477: Consider adding retry mechanism for failed requests.The error handling with
Promise.allSettledis good, but network requests can fail transiently.Consider implementing a retry mechanism:
private async withRetry<T>( operation: () => Promise<T>, retries = 3, delay = 1000 ): Promise<T> { try { return await operation(); } catch (error) { if (retries > 0) { await new Promise(resolve => setTimeout(resolve, delay)); return this.withRetry(operation, retries - 1, delay * 2); } throw error; } }
Line range hint
551-693: Add rate limiting for API requests.Multiple concurrent requests could potentially overwhelm the AOJ API.
Consider implementing a rate limiter:
class RateLimiter { private queue: Array<() => Promise<void>> = []; private processing = false; constructor(private rateLimit: number, private interval: number) {} async schedule<T>(task: () => Promise<T>): Promise<T> { return new Promise((resolve, reject) => { this.queue.push(async () => { try { resolve(await task()); } catch (error) { reject(error); } }); this.process(); }); } private async process(): Promise<void> { if (this.processing) return; this.processing = true; while (this.queue.length > 0) { const batch = this.queue.splice(0, this.rateLimit); await Promise.all(batch.map(task => task())); await new Promise(resolve => setTimeout(resolve, this.interval)); } this.processing = false; } }
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/lib/clients/aizu_online_judge.ts (3)
36-39: Consider using string literal types for better type safety.The enum values could be defined as string literal types to provide better type safety and prevent potential runtime errors from invalid string values.
-enum ChallengeContestType { - PCK = 'pck', - JAG = 'jag', -} +type ChallengeContestType = 'pck' | 'jag'; -enum JagRound { - PRELIM = 'prelim', - REGIONAL = 'regional', -} +type JagRound = 'prelim' | 'regional';Also applies to: 94-97
291-312: Consider adding cache configuration options.The cache instances are created with default values. Consider allowing these to be configured through constructor parameters.
+interface AojApiClientConfig { + contestCacheTTL?: number; + contestCacheSize?: number; + taskCacheTTL?: number; + taskCacheSize?: number; +} export class AojApiClient extends ContestSiteApiClient { + constructor(config?: AojApiClientConfig) { + super(); + this.contestCache = new Cache<ContestsForImport>( + config?.contestCacheTTL, + config?.contestCacheSize + ); + this.taskCache = new Cache<TasksForImport>( + config?.taskCacheTTL, + config?.taskCacheSize + ); + }
340-347: Consider implementing retry mechanism for failed requests.The error handling is good, but for improved reliability, consider implementing a retry mechanism for failed requests.
+private async retryOperation<T>( + operation: () => Promise<T>, + maxRetries: number = 3, + delay: number = 1000 +): Promise<T> { + for (let i = 0; i < maxRetries; i++) { + try { + return await operation(); + } catch (error) { + if (i === maxRetries - 1) throw error; + await new Promise(resolve => setTimeout(resolve, delay * (i + 1))); + } + } + throw new Error('Max retries reached'); +} const [courses, pckPrelims, pckFinals, jagPrelims, jagRegionals] = results.map((result) => { if (result.status === 'rejected') { - console.error(`Failed to fetch contests from AOJ API:`, result.reason); + console.error(`Failed to fetch contests from AOJ API after retries:`, result.reason); return []; } return result.value; });
close #1514
Summary by CodeRabbit
New Features
AOJ_JAG, to enhance contest categorization.PCKandJAGtypes.Tests
AOJ_JAGcontests across various functions to ensure accurate classification and URL generation.Documentation
JAGcontest type in the AIZU ONLINE JUDGE section.