Hydro v5 #951
Hydro v5 #951
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| return function check(this: Handler<C>) { | ||
| checker(); | ||
| if (perm) this.checkPerm(perm); |
Check warning
Code scanning / CodeQL
Useless conditional Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 2 days ago
To fix the problem, we need to ensure that the condition if (perm) is only checked when perm has a meaningful value. This can be achieved by checking if perm is defined and is of type bigint before calling this.checkPerm(perm). This will ensure that the condition is not always false and only evaluates to true when perm has a valid value.
| @@ -682,3 +682,3 @@ | ||
| checker(); | ||
| if (perm) this.checkPerm(perm); | ||
| if (typeof perm !== 'undefined' && typeof perm === 'bigint') this.checkPerm(perm); | ||
| if (priv) this.checkPriv(priv); |
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant L as Loader
participant W as Worker
participant S as Server
participant A as API
L->>L: async preload() injects Loader plugin & dependencies
W->>S: require('server').apply(ctx) for server dependency
S-->>W: returns updated context (ctx)
W->>A: require('api').apply(ctx) for API initialization
A-->>Global: Application context fully established
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/hydrooj/src/interface.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "@hydrooj/eslint-config" to extend from. Please check that the name of the config is correct. The config "@hydrooj/eslint-config" was referenced from the config file in "/.eslintrc.yaml". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. build/prepare.jsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "@hydrooj/eslint-config" to extend from. Please check that the name of the config is correct. The config "@hydrooj/eslint-config" was referenced from the config file in "/.eslintrc.yaml". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. packages/ui-default/pages/setting.page.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "@hydrooj/eslint-config" to extend from. Please check that the name of the config is correct. The config "@hydrooj/eslint-config" was referenced from the config file in "/packages/ui-default/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
🪧 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: 3
🧹 Nitpick comments (15)
framework/framework/serializer.ts (1)
3-3: Enhanced type safety with generic parameter.Adding the generic type parameter
<T>to the serializer function improves type safety by allowing the HandlerCommon parameter to be properly typed. This ensures better type checking and IDE support when working with different handler types.Consider adding a JSDoc comment to explain the purpose of the generic parameter:
+ /** + * Creates a serializer function for JSON.stringify with support for specialized handlers + * @template T Type parameter for the handler context + * @param ignoreSerializeFunction Whether to ignore serialize functions on objects + * @param h Optional handler common instance with type context + * @returns A replacer function for JSON.stringify + */ export default function serializer<T>(ignoreSerializeFunction = false, h?: HandlerCommon<T>) {packages/hydrooj/package.json (1)
19-21:❓ Verification inconclusive
Update to new @cordisjs modularity architecture
The PR introduces dependencies on alpha versions of the modular
@cordisjspackages (core,logger, andplugin-timer), replacing the previous monolithiccordispackage. This aligns with the broader codebase update to use the namespaced packages.Since these are alpha versions, let's verify their stability and compatibility:
🏁 Script executed:
#!/bin/bash # Check the latest versions of these cordis packages and any relevant release notes echo "Checking the latest versions of cordis packages:" npm view @cordisjs/core versions --json | jq '.[-5:]' npm view @cordisjs/logger versions --json | jq '.[-5:]' npm view @cordisjs/plugin-timer versions --json | jq '.[-5:]' # Check if there are any github repos with release notes echo "Looking for related GitHub repositories:" gh search repos --owner cordisjs --limit 3 --json name,urlLength of output: 612
Modular @cordisjs Dependencies Verified
The PR now correctly adopts the new modular architecture by replacing the monolithic package with the namespaced packages. Verification via
npm viewconfirms that:
- @cordisjs/core is updated to "4.0.0-alpha.1" (with earlier releases up to "4.0.0-alpha.0").
- @cordisjs/logger is updated to "1.0.0-alpha.1" (following the previous 0.3.x cycle).
- @cordisjs/plugin-timer remains at "1.0.0-alpha.0" (the only available version).
Since all these are alpha releases, additional stability and compatibility testing is advised before wider adoption.
framework/framework/decorators.ts (1)
38-40: Enhanced type safety with generic ContextThe function signature now uses generics with
<T extends Context>, allowing for more specific typing of the handler context. This is a significant improvement to type safety across the framework.Consider adding a type guard or documentation clarifying how these generic handlers should be implemented. This would be helpful for developers creating custom handlers.
packages/hydrooj/src/loader.ts (1)
142-150: Enhanced asynchronous context initializationThe
preloadfunction has been updated to be asynchronous, ensuring proper initialization of the application context with required plugins before continuing execution. This is a good improvement for handling asynchronous operations.There's a missing semicolon at the end of line 150, which was flagged by static analysis. Fix it to maintain code consistency:
- }) + });🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 150-150: Semicolon insertion
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).packages/hydrooj/src/context.ts (1)
12-13: Fix the TODO comment grammar.
There's a minor typo in the comment: "this is an broken declaration" should be "this is a broken declaration."-// TODO: this is an broken declaration +// TODO: this is a broken declarationframework/framework/server.ts (10)
186-188: Remove usage ofanyfor server renderers.
Casting(this.ctx as any).server.rendererscan be replaced with proper type definitions for your server instance to ensure type safety.-const renderers = Object.values((this.ctx as any).server.renderers as Record<string, Renderer>) +const renderers = Object.values(this.ctx.server.renderers) .filter((r) => r.accept.includes(templateName) || r.asFallback);
246-246: Ensure consistent generics.
ConnectionHandler<C>extendsHandlerCommon<C>similarly. If you anticipate a narrower generic type than anyC, consider a more constrained approach to avoid confusion in usage sites.
481-483: Address repeated type casting.
You’re using(this.ctx.parallel as any)with a// FIXME: should pass type check. Recommend updating the type definitions to remove reliance onany.
535-536: Similar pattern withthis.ctx.serial.
Likewise, you are castingthis.ctx.serialasany. Consider refining your context or plugin types for improved type safety.
551-552: Parallel usage in WS handling.
Again,(this.ctx.parallel as any)is used when creating a connection. Try to unify type definitions to avoid these repeated workarounds.
594-594: Emit usage withas any.
Emitting'connection/close'with(this.ctx.emit as any)also bypasses type checks. Suggest properly defining or augmenting your event emitter.
705-705: Event registration withas any.
this.ctx.on(…)is again cast toanyfor the callback. Type definitions for the event system can remove the need for such casts.
709-710: Refine handler parameters.
A TODO comment notes this should betypeof Handler<Context>instead ofany. Updating it would enhance type safety.
715-715: Ensure typed connection handlers.
Similar to theRoutemethod, switch fromanyto a well-defined handler type once your framework interface is stable.
720-724: Type the registered layers.
Storing all layers asanyis flexible but loses type safety. Consider refining the signature ofregisterLayerto clarify the shape of each layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.gitattributes(1 hunks)framework/framework/decorators.ts(3 hunks)framework/framework/interface.ts(1 hunks)framework/framework/package.json(1 hunks)framework/framework/serializer.ts(1 hunks)framework/framework/server.ts(21 hunks)package.json(1 hunks)packages/hydrooj/package.json(1 hunks)packages/hydrooj/src/context.ts(4 hunks)packages/hydrooj/src/entry/worker.ts(1 hunks)packages/hydrooj/src/handler/judge.ts(1 hunks)packages/hydrooj/src/loader.ts(3 hunks)packages/hydrooj/src/model/contest.ts(1 hunks)packages/hydrooj/src/model/discussion.ts(1 hunks)packages/hydrooj/src/model/document.ts(1 hunks)packages/hydrooj/src/model/domain.ts(1 hunks)packages/hydrooj/src/model/message.ts(1 hunks)packages/hydrooj/src/model/oplog.ts(1 hunks)packages/hydrooj/src/model/problem.ts(1 hunks)packages/hydrooj/src/model/setting.ts(2 hunks)packages/hydrooj/src/model/solution.ts(1 hunks)packages/hydrooj/src/model/system.ts(1 hunks)packages/hydrooj/src/model/task.ts(1 hunks)packages/hydrooj/src/model/token.ts(1 hunks)packages/hydrooj/src/model/user.ts(1 hunks)packages/hydrooj/src/service/bus.ts(4 hunks)packages/hydrooj/src/service/db.ts(1 hunks)packages/hydrooj/src/service/hmr.ts(2 hunks)packages/hydrooj/src/service/monitor.ts(1 hunks)packages/hydrooj/src/service/server.ts(7 hunks)packages/hydrooj/src/ui.ts(1 hunks)packages/onsite-toolkit/index.ts(1 hunks)packages/scoreboard-xcpcio/index.ts(1 hunks)packages/ui-default/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (9)
- packages/hydrooj/src/model/discussion.ts
- packages/scoreboard-xcpcio/index.ts
- packages/hydrooj/src/model/domain.ts
- packages/ui-default/package.json
- packages/hydrooj/src/model/system.ts
- packages/onsite-toolkit/index.ts
- .gitattributes
- packages/hydrooj/src/model/setting.ts
- packages/hydrooj/src/service/monitor.ts
🧰 Additional context used
🪛 GitHub Check: CodeQL
packages/hydrooj/src/loader.ts
[notice] 150-150: Semicolon insertion
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
framework/framework/server.ts
[warning] 676-676: Useless conditional
This use of variable 'perm' always evaluates to false.
🪛 Biome (1.9.4)
packages/hydrooj/src/service/server.ts
[error] 79-79: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
[error] 119-119: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
framework/framework/server.ts
[error] 128-128: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 129-129: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (49)
package.json (1)
23-24: Expanded linting coverage to include the framework directory.The linting script now includes the framework directory, ensuring that all TypeScript and JSX files there are also subject to the same linting rules as the rest of the project. This makes sense given that "framework/*" is already part of the project's workspaces (line 6).
framework/framework/package.json (1)
7-7:✅ Verification successful
Package dependency updated from "cordis" to "@cordisjs/core".
The peer dependency has been changed from a non-namespaced package to a namespaced one. This change is likely part of a broader migration to using the namespaced version of the package throughout the project.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining imports or dependencies for the old "cordis" package echo "Checking for remaining references to the old 'cordis' package..." grep -r "import.*from.*cordis['\"]" --include="*.ts" --include="*.tsx" --include="*.js" --include="*.jsx" . grep -r "\"cordis\":" --include="package.json" .Length of output: 303
I'll run an additional, deeper search to ensure no lingering references to the old "cordis" package exist. This new script searches broadly for any occurrence of the string "cordis" (excluding common vendored directories) to provide a higher-confidence result.
Please run this script and share the output so we can proceed with a high-confidence verification of the update from "cordis" to "@cordisjs/core."
🏁 Script executed:
#!/bin/bash echo "Performing deep search for any occurrence of 'cordis' (case-insensitive) excluding node_modules, dist, and build directories..." rg -i "cordis" --glob "!node_modules/*" --glob "!dist/*" --glob "!build/*" .Length of output: 3784
Dependency migration verified – Namespaced package "@cordisjs/core" correctly in use
The deep search confirms that the dependency update in
framework/framework/package.jsonis correctly applied. We see that all usages now reference@cordisjs/core, and no lingering references to the old non-namespaced "cordis" remain in package dependencies. Minor mentions of "cordis" in comments and documentation are acceptable as they do not affect functionality.
- Location verified:
framework/framework/package.jsonnow correctly includes"@cordisjs/core": "*"- Other code references: All module imports across the codebase consistently use the namespaced package.
packages/hydrooj/src/service/db.ts (1)
10-10:✅ Verification successful
Import style updated from namespace to default import.
The import statement has been changed from a namespace import (
import * as bus from './bus') to a default import (import bus from './bus'). This change suggests that the bus module now exports a default value, or has been refactored to do so. This change is consistent with similar changes in other files mentioned in the summary.
🏁 Script executed:
#!/bin/bash # Check the export style of the bus module cat packages/hydrooj/src/service/bus.ts | grep -E "export default|module.exports ="Length of output: 105
Import update verified:
The import change inpackages/hydrooj/src/service/db.tsfrom a namespace import (import * as bus from './bus') to a default import (import bus from './bus') is correct. The module atpackages/hydrooj/src/service/bus.tsindeed exports a default value (export default app;), confirming the intended refactor. There are no additional issues to address.packages/hydrooj/src/model/token.ts (1)
3-3: Import style updated from namespace to default import.The import statement has been changed from a namespace import (
import * as bus from '../service/bus') to a default import (import bus from '../service/bus'). This change is consistent with the project-wide refactoring to use default imports for the bus module.packages/hydrooj/src/entry/worker.ts (1)
45-49: Context handling enhancement with asynchronous injectionThe introduction of an asynchronous context injection mechanism ensures that the server dependency is fully integrated into the context before proceeding with subsequent operations. This is a good practice for proper dependency management.
This change addresses potential race conditions where
ctxmight not have the fully initialized server dependency when needed by code executed after line 44. The Promise-based approach ensures everything proceeds in the correct order.packages/hydrooj/src/ui.ts (1)
3-3: Updated import style for bus moduleThe change from namespace import (
import * as bus) to default import (import bus) indicates a refactoring of the module export strategy in the bus service.This change aligns with modern JavaScript module practices and likely part of a broader effort to standardize import styles across the codebase, as similar changes appear in multiple files.
packages/hydrooj/src/model/contest.ts (1)
12-12: Updated import style for bus moduleChanging from namespace import (
import * as bus) to default import (import bus) aligns with the project-wide standardization of how the bus service is imported.This is consistent with similar changes in other files, indicating a systematic refactoring of import statements across the codebase.
packages/hydrooj/src/model/solution.ts (1)
3-3: Updated import style for bus moduleThe import statement has been changed from namespace import to default import, consistent with other files in the project.
This change is part of a broader refactoring effort to standardize how the bus service is imported throughout the codebase, improving consistency.
packages/hydrooj/src/model/task.ts (1)
9-9: Import change from namespace to default is appropriate.The change from
import * as bus from '../service/bus'toimport bus from '../service/bus'aligns with the broader refactoring effort across the codebase to standardize import patterns.packages/hydrooj/src/model/document.ts (1)
12-12: Import change from namespace to default is appropriate.The change from
import * as bus from '../service/bus'toimport bus from '../service/bus'aligns with the broader refactoring effort across the codebase to standardize import patterns.packages/hydrooj/src/model/message.ts (1)
3-3: Import change from namespace to default is appropriate.The change from
import * as bus from '../service/bus'toimport bus from '../service/bus'aligns with the broader refactoring effort across the codebase to standardize import patterns.packages/hydrooj/src/model/oplog.ts (1)
4-4:✅ Verification successful
Import style updated for the bus module.
The import has been updated from a namespace import (
import * as bus) to a default import (import bus). This change is consistent with the same pattern applied across multiple files in the codebase.
🏁 Script executed:
#!/bin/bash # Check how bus is exported in the source file cat packages/hydrooj/src/service/bus.ts | grep -n "export default" || echo "No default export found in bus.ts"Length of output: 92
Verification complete: The import change for the bus module is correct.
The default import (
import bus from '../service/bus';) is now consistent with the export inpackages/hydrooj/src/service/bus.ts, where the default export is defined (export default app;). No further changes are needed.packages/hydrooj/src/model/user.ts (1)
12-12:✅ Verification successful
Import style updated for the bus module.
The import statement has been changed from a namespace import (
import * as bus) to a default import (import bus). This is consistent with the changes made across other files in the codebase and indicates that the bus module is now using a default export pattern.
🏁 Script executed:
#!/bin/bash # Check if bus is consistently imported as default across the codebase echo "Files using default import for bus:" rg "import bus from '../service/bus'" packages/hydrooj/src/ echo -e "\nFiles still using namespace import for bus:" rg "import \* as bus from '../service/bus'" packages/hydrooj/src/Length of output: 1254
Import style update verified across the codebase
All files, including
packages/hydrooj/src/model/user.ts, now consistently use the default import for the bus module; no namespace import remains.packages/hydrooj/src/handler/judge.ts (1)
26-26:✅ Verification successful
Import style updated for the bus module.
The import has been standardized from namespace import (
import * as bus) to default import (import bus). This is part of a broader refactoring pattern across the codebase.
🏁 Script executed:
#!/bin/bash # Check how the bus module is structured and what it exports cat packages/hydrooj/src/service/bus.ts | head -20Length of output: 732
I'll run an additional check to verify that the
bus.tsmodule indeed provides a default export as expected.
🏁 Script executed:
rg "export default" packages/hydrooj/src/service/bus.tsLength of output: 77
Import statement verified: Default export found and used correctly.
The code change from a namespace import to a default import is correct since
packages/hydrooj/src/service/bus.tsexports a default export (export default app;). No further modifications are necessary for this update.packages/hydrooj/src/model/problem.ts (1)
19-19:✅ Verification successful
Import style updated for the bus module.
The import statement has been updated from a namespace import to a default import. This change is consistent with the same pattern applied across the entire codebase for the bus module.
🏁 Script executed:
#!/bin/bash # Examine how bus is used in this file to ensure the import change is appropriate rg -A 1 "bus\.(parallel|emit|broadcast|on)" packages/hydrooj/src/model/problem.tsLength of output: 1427
Import style update for the bus module verified.
The default import from../service/busis being used consistently (e.g.,bus.parallelandbus.emit) in the filepackages/hydrooj/src/model/problem.tsas confirmed by the grep output. This change aligns with the overall codebase pattern, and no issues remain.framework/framework/decorators.ts (2)
1-1: Import from new namespaced @cordisjs packageThe import has been updated to use the new modular
@cordisjs/corepackage, which is consistent with the dependency changes in package.json.
48-48: Propagated generic type to validate functionThe nested
validatefunction properly maintains the generic type parameter from the outer function, ensuring consistent typing throughout the descriptor.packages/hydrooj/src/loader.ts (2)
169-170: Proper async handling in load functionThe
loadfunction now properly awaits the completion ofpreload(), ensuring that all dependencies are initialized before proceeding with the application startup.
207-209: Consistent async handling in CLI loaderThe
loadClifunction now properly awaits the completion ofpreload(), matching the pattern used in the mainloadfunction. This ensures consistent behavior across different entry points.packages/hydrooj/src/service/hmr.ts (4)
151-151: Simplified plugin tracking in HMRThe pending map type has been changed from
Map<string, Plugin.Runtime<Context>>toMap<string, Plugin>, focusing on storing the plugin objects directly rather than their runtime instances. This aligns with the broader changes in the codebase.
164-165: Direct plugin storage and side effect checkingThe code now properly stores the plugin object directly and checks its
sideEffectproperty. This is a cleaner approach than handling the runtime.
181-184: Improved reloads map associationThe reloads map now correctly associates the plugin with its filename and runtime, matching the updated structure of the pending map.
168-173:❓ Verification inconclusive
Refactored plugin validation loop
The loop now correctly iterates over plugins and retrieves the runtime separately. The side effect check has been moved to use the plugin object directly.
Let's verify that sideEffect access is consistent throughout the codebase:
🏁 Script executed:
#!/bin/bash # Check for consistent access patterns to the sideEffect property on plugins echo "Checking sideEffect property access patterns across the codebase:" rg --type typescript "sideEffect" -A 1 -B 1Length of output: 219
Verify SideEffect Access Consistency
The refactored plugin validation loop correctly retrieves the runtime and checks the plugin’s sideEffect property using bracket notation. However, our initial verification script failed due to an unrecognized file type error. Please manually verify that the sideEffect property is accessed consistently across the codebase by running the revised script above.
🏁 Script executed:
#!/bin/bash echo "Searching sideEffect in TypeScript files (.ts and .tsx):" rg -g '*.ts' -g '*.tsx' "sideEffect" -A 1 -B 1Length of output: 854
Plugin Validation Loop Refactor & SideEffect Consistency Check
The refactored plugin validation loop inpackages/hydrooj/src/service/hmr.ts(lines 168–173) now iterates over plugins and retrieves the runtime separately while using the simpler check (if (!plugin['sideEffect'])) as intended.However, our codebase search also revealed another instance in the same file using a more defensive pattern (
if (!plugin || !('sideEffect' in plugin) || !plugin['sideEffect'])). Please verify whether this discrepancy is intentional or if the conditions should be unified for consistency.
- Refactored loop (lines 168–173) correctly uses
if (!plugin['sideEffect']).- An earlier block in the file uses the additional existence check on
pluginand itssideEffect.packages/hydrooj/src/service/api.tsexportssideEffectconsistently.packages/hydrooj/src/service/bus.ts (4)
5-5: Import usage looks good.
Using a type import from the framework is a clean approach, ensuring we don't bring in unnecessary code.
17-18: Interface definition clarity.
DeclaringEventMapin a single interface consolidates event definitions and streamlines event-related logic. Ensure that any dynamic or external events are also typed or otherwise handled gracefully to avoid runtime errors.
88-88: Explicit handler type is consistent.
UsingHandler<Context>for theoplog/logevent is consistent with the generics introduced elsewhere, improving type clarity for event subscribers.
33-33:❓ Verification inconclusive
Confirm external references to broadcast events.
Switching fromkeyof ServerEventstokeyof EventMaprestricts the valid events that can be broadcast. Please verify and update any external code referencing events outside ofEventMap.
🏁 Script executed:
#!/bin/bash # Verify references to the old 'bus/broadcast' usage to ensure they match the new signature rg -A 3 "bus/broadcast"Length of output: 1879
Action: Validate ‘bus/broadcast’ Event Keys Against the Updated EventMap
The change from
keyof ServerEventstokeyof EventMapnow restricts broadcast events to only those defined inEventMap. Our repository search found the following usages:
- packages/prom-client/metrics.ts – The event is received as
namefor incrementing counters.- packages/hydrooj/src/model/task.ts – The event is handled with
(event, payload, trace).- packages/hydrooj/src/context.ts – The event is emitted via
ctx.emit.Please confirm that each of these usages only references event keys present in
EventMap. If any external code relies on events outside of this mapping, update either the event references or extendEventMapaccordingly.framework/framework/interface.ts (6)
1-5: Modular approach to known handlers.
DefiningKnownHandlerswith specific handler entries, such as'NotFound': NotFoundHandler, simplifies maintenance and ensures type safety. Good job.
7-14: Flexible mapped event types.
Combining template literal types with yourMapHandlerEventsandKnownHandlerEventsprovides powerful typing for your events. Just be sure that any downstream usage is thoroughly tested to catch any subtle type mismatches.
[approve]
15-17: Generic handler events.
Extending event signatures with<C>allows context-specific event hooks. This pattern is beneficial for large systems requiring specialized context handling.
21-23: Restricting hook types.
UsingExclude<HookType, 'before-operation'>forHookWithMethodis a neat technique to disallow certain hook combinations. This approach prevents accidental usage of hooks in unsupported scenarios.
24-34: Expanded ServerEvents interface.
The extendedServerEvents<C>captures both known handler events and generic handler events, covering a wide range of scenarios. This provides a robust event system while maintaining strong typing.
36-38: Extending third-party modules with generic events.
DeclaringEvents<C>to extendServerEvents<C>in@cordisjs/coreis a solid pattern. Confirm in your build/compilation steps that the augmentation merges properly and doesn't conflict with other type definitions in the same module.packages/hydrooj/src/service/server.ts (3)
8-10: Generic imports from the framework.
Switching toHandlerOriginal<Context>andConnectionHandlerOriginal<Context>aligns with the new typed approach introduced throughout the codebase. This ensures context-aware handlers for both HTTP and WebSocket connections.
33-33: Ensuring generics are fully utilized.
DeclaringHandlerCommon<C>is a good step. Confirm that all usage sites either specify<Context>or additional custom contexts as needed, to avoid unintentionalanyresolution.
135-135: Framework plugin usage.
Replacing the direct import ofrequire('@hydrooj/framework')withWebServiceis a clean, modular approach. This clarified import fosters better maintainability and avoids bundling the entire framework when only the web service is needed.packages/hydrooj/src/context.ts (5)
1-3: Imports look good.
These imports from@cordisjs/core,@cordisjs/logger, and@cordisjs/plugin-timerappear consistent with the usage ofthis.plugin(TimerService);andthis.plugin(Logger);. No issues detected here.
29-29: Confirm usage of re-exported entities.
You’re re-exportingDisposable,Plugin, andScopeStatusfrom@cordisjs/core. Ensure they’re actually used in this file or other files importing from here, otherwise they might be extraneous.
31-32: Validate the property signature.
Using[Context.events]: EventMap & cordis.Events<Context>is unusual. Verify thatContext.eventsis indeed a valid property or symbol on thecordis.Context. If it's meant to be a Symbol, ensure you have the correct reference to avoid type mismatches.
54-54: Class extension looks coherent.
DefiningApiMixinto extendcordis.Service<Context>is consistent with other services. No immediate issues found.
75-75: Logger plugin usage is fine.
Registering theLoggerplugin afterTimerServiceis valid. Ensure your logging configuration is properly initialized so logs appear in the expected format/output.framework/framework/server.ts (8)
5-5: Imports updated correctly.
ImportingContext as CordisContextandServicefrom@cordisjs/corealigns with the new context structures and type usage.
138-146: Question the usage of.extend({}).
Inside the constructor,this.ctxis being re-assigned to(ctx as any).extend({}). Verify that yourctxis indeed an object or class with anextendmethod. Otherwise, this can trigger runtime errors ifctxdoes not meet that contract.
200-200: Class extension is consistent.
Handler<C>extendsHandlerCommon<C>with additional methods. Recommend specifying constraints onCif you rely on specific properties from context. Otherwise, good to proceed.
283-283: ExplicitCordisContexttype.
Usingextends Handler<CordisContext>is a valid approach for theNotFoundHandlerif you specifically only handle aCordisContext. Looks consistent.
322-322: GenericWebServicedefinition is sound.
Providing<C extends CordisContext = CordisContext>is a flexible default. This should help with type safety across extended contexts.
337-338: Initialize with config and context.
The constructor properly callssuper(ctx, 'server')and mixes in server features. No immediate issues detected.
655-680: Useless conditional onperm
Static analysis indicates thatif (perm)may always evaluate to false under certain usage becausepermappears not to be set by typical flows. This is the same concern flagged in previous reviews or scanning tools.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 676-676: Useless conditional
This use of variable 'perm' always evaluates to false.
766-771: Module augmentation for@cordisjs/core.
Augmentinginterface Contextwithserver,Route, and others is a standard TypeScript approach. Make sure each property is set or at least defined at runtime to avoid undefined references.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
framework/framework/server.ts (2)
128-129:⚠️ Potential issueFix unsafe declaration merging
The empty interface
HandlerCommon<C>followed by a class with the same name causes unsafe declaration merging. The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.-export interface HandlerCommon<C> { } // eslint-disable-line @typescript-eslint/no-unused-vars +// Use a type alias instead of an empty interface export class HandlerCommon<C> {🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 129-129: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
655-679:⚠️ Potential issueFix conditional logic in Checker function
The
Checkerfunction has been refactored to properly handle permission and privilege checking, but there's an issue in the returned checking function.The condition on line 676 is flagged by static analysis as always evaluating to false. This is because
permis initialized asundefinedand doesn't get properly assigned in some code paths.return function check(this: Handler<C>) { checker(); - if (perm) this.checkPerm(perm); + if (perm !== undefined) this.checkPerm(perm); if (priv) this.checkPriv(priv); };🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 676-676: Useless conditional
This use of variable 'perm' always evaluates to false.
🧹 Nitpick comments (5)
packages/hydrooj/src/plugin-api.ts (1)
56-58: Use explicit side-effect import instead of empty export.
While this approach can load side effects from'./handler/contest', it's more conventional to use a side-effect import statement for clarity.Use this diff to switch to a side-effect import:
- // to load ctx.scoreboard - export { } from './handler/contest'; + // to load ctx.scoreboard + import './handler/contest';packages/hydrooj/src/model/user.ts (1)
468-486: Consider adding error handling or logging for index creation.
Although creating these indexes in parallel is fine, you might want to handle or log possible failures to aid debugging.Example of adding minimal error handling:
export async function apply() { await Promise.all([ db.ensureIndexes( coll, { key: { unameLower: 1 }, name: 'uname', unique: true }, { key: { mailLower: 1 }, name: 'mail', unique: true }, ).catch(err => console.error('Error ensuring coll indexes:', err)), db.ensureIndexes( collV, { key: { unameLower: 1 }, name: 'uname', unique: true }, { key: { mailLower: 1 }, name: 'mail', unique: true }, ).catch(err => console.error('Error ensuring collV indexes:', err)), db.ensureIndexes( collGroup, { key: { domainId: 1, name: 1 }, name: 'name', unique: true }, { key: { domainId: 1, uids: 1 }, name: 'uid' }, ).catch(err => console.error('Error ensuring collGroup indexes:', err)), ]); }packages/hydrooj/src/model/domain.ts (1)
288-308: Add optional logging to the new apply function.
Invokingctx.on('domain/delete-cache', ...)is a good pattern. For improved observability, consider logging on domain cache deletion or whenever index creation completes.export async function apply(ctx: Context) { ctx.on('domain/delete-cache', async (domainId: string) => { const ddoc = await DomainModel.get(domainId); if (!ddoc) return; for (const host of ddoc.hosts || []) { cache.delete(`host::${host}`); } cache.delete(`id::${domainId}`); + console.info(`[domain/apply] Cache entries cleared for domain: ${domainId}`); }); await Promise.all([ db.ensureIndexes( coll, { key: { lower: 1 }, name: 'lower', unique: true }, ), db.ensureIndexes( collUser, { key: { domainId: 1, uid: 1 }, name: 'uid', unique: true }, { key: { domainId: 1, rp: -1, uid: 1 }, name: 'rp', sparse: true }, ), ]); + console.info('[domain/apply] Domain indexes ensured successfully'); }framework/framework/server.ts (2)
138-146: Proper generic type implementation in HandlerCommon constructorThe constructor now accepts a generic context type parameter, improving type safety. However, there's an unsafe type cast on line 146 that could be improved with a proper type definition.
- this.ctx = (ctx as any).extend({}); + // Consider using a more specific type instead of 'any' + this.ctx = ctx.extend({});
708-710: Fix TypeScript type annotation for Route methodThe
Routemethod currently has a FIXME comment indicating it should use a more specific type rather thananyfor theRouteHandlerparameter.- // FIXME: should be typeof Handler<Context> instead of any - // eslint-disable-next-line @typescript-eslint/naming-convention - public Route(name: string, path: string, RouteHandler: any, ...permPrivChecker) { + // eslint-disable-next-line @typescript-eslint/naming-convention + public Route(name: string, path: string, RouteHandler: typeof Handler<C>, ...permPrivChecker) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
framework/framework/server.ts(21 hunks)packages/hydrooj/src/handler/judge.ts(2 hunks)packages/hydrooj/src/loader.ts(3 hunks)packages/hydrooj/src/model/domain.ts(2 hunks)packages/hydrooj/src/model/user.ts(2 hunks)packages/hydrooj/src/plugin-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hydrooj/src/handler/judge.ts
🧰 Additional context used
🪛 Biome (1.9.4)
framework/framework/server.ts
[error] 128-128: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 129-129: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
🪛 GitHub Check: CodeQL
framework/framework/server.ts
[warning] 676-676: Useless conditional
This use of variable 'perm' always evaluates to false.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (9)
packages/hydrooj/src/model/domain.ts (2)
4-4: Importing Context is valid.
This new import correctly aligns with the addedapplyfunction’s signature.
6-6: Default import from bus is consistent with other refactors.
Switching from a namespace import to a default import is in line with the broader shift in this update.packages/hydrooj/src/loader.ts (3)
142-150: Good use of async/await for application initializationThe conversion of
preload()to an async function using a Promise-based initialization pattern is a significant improvement. This ensures proper sequencing of the initialization steps and prevents potential race conditions when setting up the application context.
169-170: Properly awaiting preload for sequential executionConverting the
loadfunction to async and awaiting thepreload()function ensures the application is properly initialized before proceeding with additional setup steps.
207-209: Appropriate async CLI initializationThe
loadClifunction correctly uses async/await pattern to ensure proper initialization sequence, matching the changes in theloadfunction.framework/framework/server.ts (4)
5-5: Updated import to use new package structureThe change from importing
Contextto importingContext as CordisContextfrom@cordisjs/corealigns with the package structure changes mentioned in the PR summary.
200-200: Good use of generic defaults in Handler classUsing
<C = CordisContext>provides a sensible default while allowing for custom context types when needed, enhancing flexibility without sacrificing type safety.
246-246: Generic type parameter added to ConnectionHandlerThe
ConnectionHandlerclass now properly accepts a generic context type, aligning with the other handler classes for type consistency.
766-772: Well-structured module declaration augmentationThe module declaration extends the
Contextinterface to include the server-related properties with proper generic type handling, ensuring type safety when accessing these properties from a context instance.
| // FIXME: should pass type check | ||
| await (this.ctx.parallel as any)('handler/create', h, 'http'); | ||
| await (this.ctx.parallel as any)('handler/create/http', h); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address type checking TODOs for event emission
Several type casts to any have been added with FIXME comments. These should be properly typed to maintain type safety throughout the codebase.
Consider creating proper type definitions for the event handlers:
// Add proper typings for the event handlers
interface HandlerEvents<T> {
'handler/create': [handler: T, type: 'http' | 'ws'];
'handler/create/http': [handler: T];
// Add other event types
}
// Then update the Context interface to include these typed events
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/hydrooj/src/service/server.ts (2)
79-79:⚠️ Potential issueAddress unsafe interface-class merge [noUnsafeDeclarationMerging].
The interface
Handler(line 76) and classHandler(line 79) share the same name, potentially causing issues with uninitialized interface fields. To avoid confusion and ensure full type checking, consider renaming the interface or incorporating its fields into the class.-export interface Handler { - domain: DomainDoc; -} -export class Handler extends HandlerOriginal<Context> { +interface IHandler { + domain: DomainDoc; +} +export class Handler extends HandlerOriginal<Context> implements IHandler { constructor(_, ctx) { super(_, ctx); ...🧰 Tools
🪛 Biome (1.9.4)
[error] 79-79: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
119-119:⚠️ Potential issueLikewise address interface-class merge.
Same issue as above for
ConnectionHandler. Interface and class share the same name, leading to potential confusion and partial type checks. Rename or integrate the interface into the class.-export interface ConnectionHandler { - domain: DomainDoc; -} -export class ConnectionHandler extends ConnectionHandlerOriginal<Context> { +interface IConnectionHandler { + domain: DomainDoc; +} +export class ConnectionHandler extends ConnectionHandlerOriginal<Context> implements IConnectionHandler { constructor(_, ctx) { super(_, ctx); ...🧰 Tools
🪛 Biome (1.9.4)
[error] 119-119: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
🧹 Nitpick comments (1)
packages/hydrooj/src/context.ts (1)
75-75: Added LoggerService to the Context constructor.The addition of LoggerService to the Context constructor ensures that logging capabilities are available throughout the application. This is a good enhancement for observability.
Consider documenting what features the LoggerService provides and how it should be used by developers working on the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/hydrooj/src/context.ts(4 hunks)packages/hydrooj/src/service/server.ts(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/hydrooj/src/service/server.ts
[error] 79-79: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
[error] 119-119: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (12)
packages/hydrooj/src/service/server.ts (7)
8-10: Import statement updated to support generic typed framework components.The imports have been updated to use the original Handler and ConnectionHandler with generic typing, and WebService has been added as a new direct import. This supports the generic type improvements throughout the codebase.
33-33: Good type enhancement to the HandlerCommon interface.Adding the generic type parameter
<C>to the HandlerCommon interface improves type safety by making the interface more flexible and allowing it to work with different context types.
80-80: Good simplification of constructor signature.The constructor signature has been simplified by removing explicit type declarations, as TypeScript can now infer the types from the generic parameter
<Context>provided to the parent class.
120-120: Good simplification of constructor signature.Similar to the Handler class, the constructor signature has been simplified by removing explicit type declarations which are now inferred from the generic parameter.
135-135: Updated to use WebService directly instead of requiring the framework.The change from
require('@hydrooj/framework')to directly using the importedWebServiceis a good practice as it makes the dependency more explicit and can help with tree-shaking during builds.
222-222: Minor improvement to router URL call.This change appears to be a slight modification to the method call but maintains the same functionality. It's a minor improvement to the code style.
283-296: Enhanced error handling with new event handlers for HTTP and WebSocket connections.The addition of event handlers for 'handler/create/http' and 'handler/create/ws' with error checking logic is a good improvement. It ensures that any pending errors are properly thrown and handled early in the request lifecycle.
The implementation properly checks for
pendingErrorin the context and throws it if present, which will trigger the appropriate error handling flow.packages/hydrooj/src/context.ts (5)
1-3: Updated imports to use dedicated packages instead of a monolithic import.The import changes from 'cordis' to '@cordisjs/core' and addition of LoggerService and TimerService from their respective packages indicate a move towards a more modular architecture. This is a good practice that can lead to better tree-shaking and more explicit dependencies.
12-13: TODO needs to be addressed for broken declaration.The comment acknowledges a broken declaration, and the code exports EventMap as Events. This should be prioritized for fixing since it could lead to type-related bugs.
Could you clarify what makes this declaration "broken"? Is it related to:
- EventMap compatibility with the rest of the system?
- A transition period during refactoring?
Consider either fixing the issue or adding more details about the problem in the TODO comment to help future developers.
29-29: Updated exports to match new import sources.The exports have been updated to match the new import path from '@cordisjs/core' instead of 'cordis', which is consistent with the import changes at the top of the file.
31-32: Improved type definition for Context interface.The Context interface has been simplified and improved by:
- Removing the Pick dependency
- Using EventMap with cordis.Events to properly type the events
This change enhances type safety while making the code more maintainable.
54-54: Enhanced type safety with generic parameter on ApiMixin.Adding the generic type parameter
<Context>to the ApiMixin class improves type safety by ensuring that the service correctly works with the Context type.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/hydrooj/src/service/server.ts (1)
76-78: 🛠️ Refactor suggestionAddress unsafe interface-class merge.
The code has an unsafe interface-class merge issue where interfaces and classes share the same name. This pattern is flagged by TypeScript's linter as it may lead to uninitialized interface fields not being checked by the compiler.
Consider implementing the interface in the class definition instead:
-export class Handler extends HandlerOriginal<Context> { - domain: DomainDoc; -} +export class Handler extends HandlerOriginal<Context> implements { domain: DomainDoc } { + // The domain property is already implemented through inheritance +} -export class ConnectionHandler extends ConnectionHandlerOriginal<Context> { - domain: DomainDoc; -} +export class ConnectionHandler extends ConnectionHandlerOriginal<Context> implements { domain: DomainDoc } { + // The domain property is already implemented through inheritance +}This approach makes it explicit that the class is implementing these properties and avoids the interface-class merge warning.
Also applies to: 79-81
framework/framework/server.ts (2)
128-129: 🛠️ Refactor suggestionResolve unsafe declaration merging.
There's an unsafe declaration merging issue with an empty interface and class sharing the same name. This pattern is flagged by TypeScript's linter as potentially problematic.
Replace the empty interface with a type alias or remove it completely:
-export interface HandlerCommon<C> { } // eslint-disable-line @typescript-eslint/no-unused-vars -export class HandlerCommon<C> { +export class HandlerCommon<C> {Since the interface is empty, there's no need to keep it. If the intention was to allow for module augmentation, consider using a different approach or documentation to indicate this.
🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 129-129: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
481-483: 🛠️ Refactor suggestionAddress type-checking TODOs/FIXMEs.
Several TODOs/FIXMEs have been added related to type checking issues, particularly with event emission. These should be properly addressed to maintain type safety throughout the codebase.
Instead of using type assertions to
any, consider creating proper type definitions for the event handlers:// Define an interface for events interface ServerEvents<T> { 'handler/create': [handler: T, type: 'http' | 'ws']; 'handler/create/http': [handler: T]; 'handler/create/ws': [handler: T]; 'handler/error': [handler: T, error: Error]; 'handler/error/${string}': [handler: T, error: Error]; 'connection/create': [handler: T]; 'connection/active': [handler: T]; 'connection/close': [handler: T]; } // Update the Context interface interface Context { // Update methods to use typed events parallel<K extends keyof ServerEvents<T>, T>(event: K, ...args: ServerEvents<T>[K]): Promise<void>; serial<K extends keyof ServerEvents<T>, T>(event: K, ...args: ServerEvents<T>[K]): Promise<void>; }This would allow for proper type checking of event emissions without resorting to
anycasts.Also applies to: 535-536, 550-551, 579-582, 593-594, 632-633, 704-705
🧹 Nitpick comments (4)
packages/utils/lib/common.ts (1)
214-214: Improved type safety in sortFiles function signature.The addition of a generic type parameter
Tthat extends{ _id: string }enhances type safety by preserving the full type of the input objects when they're returned. This change ensures that consumers of this function retain all type information about their objects, rather than getting back only objects with the minimal{ _id: string }structure.Consider making the key parameter more type-safe as well by using a mapped type to ensure it's a key of T:
-export function sortFiles<T extends { _id: string }>(files: T[], key?: '_id'): T[]; +export function sortFiles<T extends { _id: string }>(files: T[], key?: keyof T & '_id'): T[];This would ensure that when a key is provided, it exists on the type T and is specifically the '_id' property.
packages/migrate/index.ts (1)
10-16: Improved function signature to accept and forward multiple argumentsThis change enhances the flexibility of the
checkLockfunction by using rest parameters and argument spreading, allowing it to accept an arbitrary number of arguments. The modification improves the function's adaptability for use with various script runners that might require different parameter signatures.function checkLock(innerCall) { - return async (args) => { + return async (...args) => { const cur = await SystemModel.get('migrate.lock'); if (cur) throw new SystemError(`migrate lock already exists: ${cur}, possible another migration is running`); - return innerCall(args); + return innerCall(...args); }; }packages/hydrooj/src/entry/cli.ts (1)
126-130: Introduced asynchronous context modification with server dependency injectionThis change adds an important asynchronous step in the application initialization flow, ensuring the server dependency is properly injected into the context before proceeding with the remainder of the startup process. The context is reassigned after the dependency injection to ensure subsequent code uses the updated context.
The pattern used here (creating a new Promise and resolving it with the updated context) is appropriate for ensuring proper sequencing in the initialization process.
While this approach works, consider examining whether this initialization pattern could be standardized across other similar dependency injection flows in the application for consistency. You might also benefit from a more formalized dependency resolution system if similar patterns appear elsewhere.
packages/hydrooj/src/entry/common.ts (1)
45-45: Simplified plugin loading API by removing redundant context parameterThe call to
ctx.loader.reloadPluginhas been updated to remove thectxparameter, which was redundant since the context is already available through the method's object. This change is part of a broader refactoring to simplify the plugin loading mechanism.The code now passes fewer parameters, making it cleaner, but verify that this pattern is consistently applied across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
README.md(1 hunks)framework/eslint-config/package.json(1 hunks)framework/framework/package.json(2 hunks)framework/framework/server.ts(20 hunks)package.json(4 hunks)packages/elastic/package.json(1 hunks)packages/hydrojudge/package.json(1 hunks)packages/hydrooj/package.json(3 hunks)packages/hydrooj/src/entry/cli.ts(1 hunks)packages/hydrooj/src/entry/common.ts(2 hunks)packages/hydrooj/src/entry/worker.ts(2 hunks)packages/hydrooj/src/loader.ts(6 hunks)packages/hydrooj/src/service/server.ts(6 hunks)packages/migrate/index.ts(1 hunks)packages/ui-default/package.json(4 hunks)packages/utils/lib/common.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/elastic/package.json
- packages/hydrojudge/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/hydrooj/src/entry/worker.ts
- package.json
- framework/framework/package.json
- packages/hydrooj/src/loader.ts
- packages/ui-default/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
framework/framework/server.ts
[error] 128-128: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 129-129: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
🪛 GitHub Check: CodeQL
framework/framework/server.ts
[warning] 676-676: Useless conditional
This use of variable 'perm' always evaluates to false.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (27)
packages/migrate/index.ts (5)
32-32: Consistent use of rest parameter patternThis migration script runner is correctly updated to use the spread operator, aligning with the change in the
checkLockfunction.
46-46: Consistent use of rest parameter patternThis migration script runner is correctly updated to use the spread operator, matching the new implementation of the
checkLockfunction.
57-57: Consistent use of rest parameter patternThis migration script runner is correctly updated to use the spread operator, matching the new implementation of the
checkLockfunction.
70-70: Consistent use of rest parameter patternThis migration script runner is correctly updated to use the spread operator, matching the new implementation of the
checkLockfunction.
85-85: Consistent use of rest parameter patternThis migration script runner is correctly updated to use the spread operator, matching the new implementation of the
checkLockfunction.packages/hydrooj/src/entry/cli.ts (1)
134-134:✅ Verification successful
Updated plugin reload method signature
The call to
reloadPluginhas been modified to remove thectxparameter, consistent with changes in the plugin loading mechanism across the codebase. This suggests the method now relies on an internally stored context rather than requiring it to be passed explicitly.
🏁 Script executed:
#!/bin/bash # Verify how reloadPlugin is defined and used elsewhere in the codebase # Check the definition of reloadPlugin method to confirm it no longer expects the context parameter rg -A 5 "reloadPlugin.*?\(" --glob "*.ts" | grep -v "ctx, " || echo "No instances of old-style reloadPlugin calls found" # Check for any remaining instances of the old method signature that might need updating rg "reloadPlugin.*?\(ctx," --glob "*.ts" || echo "No remaining instances of old-style method calls found"Length of output: 2959
Plugin Reload Method Signature Verified as Updated
The usage in
packages/hydrooj/src/entry/cli.tsis now aligned with the new method signature—only the plugin key, configuration, and an optional name are passed. Verification across the codebase confirmed that all invocations (incli.ts,common.ts, andworker.ts) have removed the extraneous context parameter, and thereloadPlugindefinition inpackages/hydrooj/src/loader.tsreflects this update.
- The call in
cli.tsnow correctly uses:ctx.loader.reloadPlugin(path.resolve(scriptDir, h), {}, `hydrooj/script/${h.split('.')[0]}`);- No occurrences of the old pattern (passing
ctxas an argument) were found.framework/eslint-config/package.json (4)
11-11: Updated De Morgan plugin to latest versionThe eslint-plugin-de-morgan has been updated to version 1.2.0, which may include improvements to logical expression handling.
16-16: Updated React Hooks plugin to latest versionThe eslint-plugin-react-hooks has been updated to version 5.2.0, which likely includes improvements for detecting React Hooks issues and supporting newer React features.
22-25:✅ Verification successful
Added TypeScript as an optional dependency
TypeScript has been added as an optional dependency with version ^5.8.2. This is a good practice as it ensures a compatible TypeScript version is available for the ESLint plugins without making it a strict requirement.
🌐 Web query:
Is TypeScript 5.8.2 the latest version, and what are its key features?💡 Result:
TypeScript 5.8 is the latest stable version as of March 2025, with TypeScript 5.8.2 likely representing a minor patch within this release. This update introduces significant improvements to type safety, module interoperability, and developer tooling. Below are the key features and enhancements:
1. Granular Return Expression Checks
TypeScript 5.8 adds stricter checks for conditional expressions in return statements. Each branch of a return expression is now validated against the function's declared return type, catching errors where mismatched types might otherwise go unnoticed. For example:
declare const cache: Map<any, any>; function getURL(url: string): URL { return cache.has(url) ? cache.get(url) : url; // Error: 'string' not assignable to 'URL'[1][4][10]2. Improved CommonJS/ESM Interoperability
With Node.js 22+ support under the
--module nodenextflag, CommonJS modules can nowrequire()ECMAScript modules (ESM), simplifying library development and eliminating the need for dual publishing. This resolves long-standing compatibility issues between module systems[1][6][10].3.
--erasableSyntaxOnlyFlagThis new compiler option enforces syntax that can be fully erased during compilation, ensuring compatibility with Node.js’ experimental TypeScript execution mode. It disallows runtime-impacting TypeScript features like enums, namespaces, and parameter properties in classes[1][5][10].
4. Performance Optimizations
- Faster program loads/updates: Reduced path normalization overhead and avoided redundant configuration checks (e.g.,
tsconfig.jsonvalidation) improve responsiveness in large projects[8][10].- Watch mode/editor efficiency: Updates to files now skip unnecessary revalidation steps when project structure remains unchanged[8][10].
5. Declaration File Improvements
Computed property names in classes are now preserved in declaration files instead of being rewritten as index signatures. This enhances predictability for consumers of
.d.tsfiles[5][10].6. Module and Library Enhancements
--libReplacementflag: Disables automatic lookup for@typescript/lib-*packages, reducing overhead for projects not using custom type libraries[5][10].- Import assertion restrictions: The
assertkeyword for imports is deprecated in favor ofwithunder--module nodenext, aligning with Node.js 22+ standards[10].7. Deferred Features
A planned feature for conditional return type checking was moved to TypeScript 5.9 due to implementation complexities. However, preparatory work enabled the granular return checks in 5.8[1][4][10].
TypeScript 5.8 focuses on refining type safety, modernizing module workflows, and optimizing tooling performance. Developers can install it via
npm install typescriptor explore nightly builds for upcoming 5.9 features[1][10].Citations:
- 1: https://www.infoworld.com/article/3836957/typescript-5-8-reaches-general-availability.html
- 2: https://www.youtube.com/watch?v=vcVoyLQMCxU
- 3: https://javascript.plainenglish.io/typescript-5-8-smarter-faster-and-more-efficient-30b5ea678ede
- 4: https://visualstudiomagazine.com/Articles/2025/03/04/TypeScript-5-8-Improves-Type-Checking-Conditional-Feature-Delayed-to-5-9.aspx
- 5: https://devblogs.microsoft.com/typescript/announcing-typescript-5-8-beta/
- 6: https://www.heise.de/en/news/TypeScript-5-8-improves-interoperability-between-CommonJS-and-ECMAScript-10301871.html
- 7: https://angular.dev/reference/versions
- 8: https://visualstudiomagazine.com/Articles/2025/01/29/TypeScript-5-8-Beta-Speeds-Program-Loads-Updates.aspx
- 9: https://en.wikipedia.org/wiki/TypeScript
- 10: https://devblogs.microsoft.com/typescript/announcing-typescript-5-8/
Approved: TypeScript ^5.8.2 is Correctly Configured
The
optionalDependenciesupdate inframework/eslint-config/package.jsonnow correctly specifies TypeScript version ^5.8.2. Our verification confirms that TypeScript 5.8 (with patch 5.8.2) is the latest stable release as of March 2025, and it brings useful improvements in type safety, module interoperability, and tooling performance. This optional dependency ensures that ESLint plugins have access to a compatible TypeScript without enforcing a strict requirement.
- Location:
framework/eslint-config/package.json, lines 22-25- Snippet:
}, "optionalDependencies": { "typescript": "^5.8.2" }
8-9:✅ Verification successful
Updated TypeScript ESLint dependencies to latest versions
The TypeScript ESLint plugin and parser have been updated to version 8.26.0, which likely includes bug fixes, new rule sets, and improved TypeScript language support.
🌐 Web query:
What are the major changes in @typescript-eslint/eslint-plugin version 8.26.0 compared to previous versions?💡 Result:
The @typescript-eslint/eslint-plugin version 8.26.0 introduces several significant changes and enhancements. Here's a breakdown of the major updates:
🚀 Key Features and Enhancements
TypeScript 5.8 Support
- Full compatibility with TypeScript 5.8, ensuring accurate linting for new syntax and type-checking features[1][2].
Project Service Stabilization
- The experimental
projectServiceoption is now stable, replacingparserOptions.projectin many cases. It simplifies configuration and improves performance by leveraging TypeScript's editor-like project management[2][3].// Example configuration parserOptions: { projectService: true, // Automatically finds tsconfig.json tsconfigRootDir: import.meta.dirname, }New Rules and Rule Updates
- Added Rules:
no-empty-object-type: Bans the{}type in ambiguous contexts[2].no-require-imports: Replaces the deprecatedno-var-requiresrule[2][3].no-unsafe-function-type: Flags unsafeFunctiontype usage[2].no-wrapper-object-types: BansObject,Number, etc., in favor of primitives[2].- Enhanced Rules:
no-unnecessary-type-parameters: Improved handling of tuples and array parameter locations[1].no-unused-var: Now detects unused variables in declaration files with implicit exports[1].
🩹 Fixes and Optimizations
- Dependency Updates: Compatibility with ESLint v9 and other tooling improvements[1][4].
- Scope Manager: Reduced redundant
Object.entriescalls for type libs, improving performance[1].- Rule Fixes:
no-invalid-void-type: Reports invalidvoidtypes inaccessorproperties[1].no-unnecessary-type-assertion: Handlesunknowntypes correctly[1].unified-signatures: Ignores overloads with differing JSDoc comments[1].
⚠️ Breaking Changes and Deprecations
- Deprecated Rules:
ban-typeswas split into more targeted rules (no-empty-object-type,no-unsafe-function-type, etc.)[2][3].no-throw-literalreplaced byonly-throw-error[3].- Configuration Presets Updated:
eslint:recommendedandrecommended-type-checkednow include stricter defaults[2][3].- Removed rules:
ban-types,no-loss-of-precision,no-var-requires[2].
🛠️ Migration Steps
- Update Dependencies:
npm install @typescript-eslint/[email protected] @typescript-eslint/[email protected] --save-dev- Adopt
projectService: Replaceproject: truewithprojectService: truefor faster, more reliable type-aware linting[2][3].- Review Rule Changes: Disable new rules if necessary or adjust configurations for stricter defaults.
For a full list of changes, see the v8.26.0 release notes[1].
Citations:
- 1: https://github.com/typescript-eslint/typescript-eslint/releases
- 2: https://typescript-eslint.io/blog/announcing-typescript-eslint-v8/
- 3: https://typescript-eslint.io/blog/announcing-typescript-eslint-v8-beta/
- 4: https://www.npmjs.com/package/eslint/v/8.26.0
- 5: https://forge.libre.sh/libre.sh/keycloak-scim/commit/ec54a065f7d719e838652d5d3d2bb1a312bc40ed
- 6: https://eslint.org/docs/latest/use/migrating-to-5.0.0
- 7: https://typescript-eslint.io/packages/eslint-plugin/
- 8: https://typescript-eslint.io/users/releases/
- 9: Bug: can not find flat file config eslint/eslint#16485
- 10: https://experienceleaguecommunities.adobe.com/t5/adobe-experience-manager/aem-cloud-typescript-dependencies/m-p/657665
Updated TypeScript ESLint Dependencies to v8.26.0 – Leveraging TypeScript 5.8 and Enhanced Linting Capabilities
- The dependency upgrade in
framework/eslint-config/package.jsonnow sets both the ESLint plugin and parser to version 8.26.0.- This version brings full support for TypeScript 5.8, stable
projectServiceconfiguration (which simplifiestsconfig.jsonmanagement), and introduces several new and enhanced rules (e.g., improved handling of empty object types, unsafe function types, and more).- The changes ensure better performance, improved configuration defaults, and overall more robust TypeScript linting without any visible removal or replacement in the diff snippet.
packages/hydrooj/src/entry/common.ts (1)
72-72: Simplified plugin loading API by removing redundant context parameterSimilar to the change in the getLoader function, this call to
ctx.loader.reloadPluginhas been updated to remove the redundantctxparameter. This maintains consistency with the plugin loading mechanism changes throughout the codebase.README.md (2)
125-125: Documentation improvement for installation warning.Good addition of a warning about shared memory (shm) space size during installation. This is important information for users, especially when deploying in containerized environments where shared memory limitations can cause issues.
127-134: Good documentation of major architectural changes.The documentation clearly outlines important architectural changes:
- Upgrade to cordis@4
- Support for multiple renderers
- Separation of HMR and Watcher components
- Enhanced i18n with interface options
- New checker support
- Removal of legacy bus calls
- Enforced dependency declarations
These notes will be helpful for users upgrading from previous versions.
packages/hydrooj/package.json (2)
19-21: Migration to namespaced cordis packages.Good transition from the monolithic
cordispackage to the modular@cordisjs/*packages. This follows modern JavaScript packaging practices and should provide better maintainability and potentially reduced bundle sizes through more granular imports.
22-22: Dependency updates to newer versions.Keeping dependencies updated is good practice for security and performance. Make sure to test the application thoroughly with these new versions to ensure compatibility.
Also applies to: 28-28, 46-46, 52-52, 67-67
packages/hydrooj/src/service/server.ts (7)
8-10: Improved imports with type-safety.Good improvement on imports using type imports and adding
WebServicefrom the framework. This makes the dependencies clearer and will provide better type checking.
33-33: Enhanced type-safety with generic interface.Converting
HandlerCommonto a generic interface with type parameter<C>improves type safety and flexibility. This allows the interface to adapt to different context types throughout the application.
84-93: Improved plugin architecture.Great improvement to use
WebServicedirectly as a plugin instead of requiring the entire framework module. This follows better dependency injection practices and makes the code more maintainable.
222-245: Well-structured error handling for HTTP handlers.The addition of the
httpHandlerMixinwith detailed error handling logic is a good improvement. It properly handles different error types, provides appropriate responses, and includes useful logging.The special handling for unauthenticated users encountering permission errors by redirecting to the login page is a nice UX touch.
247-255: Consistent error handling for WebSocket connections.Good addition of error handling for WebSocket connections. The implementation properly logs errors while excluding common errors like NotFoundError when the user is not authenticated, which helps keep logs clean.
262-268: Enhanced template rendering with domain support.The implementation of domain-specific template rendering is a good improvement. It attempts to load a domain-specific template first (
${s[0]}.${args.domainId}.${s[1]}) and falls back to the generic template if not found.
270-283: Improved error handling in handler creation.The addition of error checking for pending errors in the handler creation process is a good safety feature. This ensures that any errors that occurred during earlier stages of request processing are properly propagated.
framework/framework/server.ts (5)
5-5: Migration to @cordisjs/core.Good update to import
Context as CordisContextfrom@cordisjs/coreinstead of using a local Context. This aligns with the package.json changes and moves toward using the namespaced cordis packages.
200-200: Enhanced type safety with generics.Good improvement to make the
Handler,ConnectionHandler,NotFoundHandler, andWebServiceclasses generic. This allows for better type checking and more flexible composition across the application.Also applies to: 246-246, 283-283, 322-322
655-679: Improved permission and privilege checking logic.The reworked
Checkerfunction is a significant improvement. It properly handles different types of inputs (objects, numbers, bigints) and returns a function that checks permissions and privileges based on the provided arguments. This makes the code more maintainable and reduces duplication.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 676-676: Useless conditional
This use of variable 'perm' always evaluates to false.
749-763: Well-structured mixin implementation with effect hooks.The implementation of the mixin methods using
ctx.effect()is excellent. This approach:
- Properly registers and unregisters the mixins when components are activated/deactivated
- Separates concerns between HTTP and WebSocket handlers
- Provides clean warnings when attempting to override existing methods
- Uses type-safe generics to ensure compatibility with the target classes
This implementation follows modern React-like patterns for managing side effects and cleanup.
Also applies to: 765-779, 781-794
807-814: Enhanced type declarations for framework integration.The module augmentation for
@cordisjs/coreis well implemented. It ensures that theserver,Route,Connection, andwithHandlerClassproperties are properly typed in the extendedContextinterface, providing proper type checking for users of the framework.
| cursor[p] ||= {}; | ||
| cursor = cursor[p]; | ||
| } | ||
| cursor[t] = value; |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 19 hours ago
To fix the prototype pollution issue, we need to ensure that the key parameter cannot contain dangerous properties like __proto__, prototype, or constructor at any level of nesting. We can achieve this by using a Map object instead of a plain object for the root and cursor variables. This approach will prevent prototype pollution because Map objects do not have a prototype chain that can be polluted.
| @@ -44,10 +44,10 @@ | ||
| const t = path.pop(); | ||
| const root = JSON.parse(JSON.stringify(source)); | ||
| const root = new Map(Object.entries(source)); | ||
| let cursor = root; | ||
| for (const p of path) { | ||
| cursor[p] ??= {}; | ||
| cursor = cursor[p]; | ||
| if (!cursor.has(p)) cursor.set(p, new Map()); | ||
| cursor = cursor.get(p); | ||
| } | ||
| cursor[t] = value; | ||
| return root; | ||
| cursor.set(t, value); | ||
| return Object.fromEntries(root); | ||
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (17)
packages/migrate/scripts/hustoj.ts (2)
291-296: Improved numeric handling with BigInt for large datasetsThe change to use BigInt for pagination logic is a good improvement that prevents potential numeric overflow when dealing with large datasets in migration scenarios.
However, be cautious about the conversion between BigInt and Number types in the progress reporting code.
Consider adding a comment explaining why BigInt is used here to help future maintainers understand the rationale:
- const [{ 'count(*)': _rcount }] = await query('SELECT count(*) FROM `solution`'); - const rcount = BigInt(_rcount); + // Using BigInt to handle potentially large solution counts that exceed Number.MAX_SAFE_INTEGER + const [{ 'count(*)': _rcount }] = await query('SELECT count(*) FROM `solution`'); + const rcount = BigInt(_rcount);
332-337: Ensure safe conversion from BigInt to NumberWhen reporting progress, you're converting BigInt values to Number which could lead to precision loss with extremely large values.
Consider adding a safety check before the conversion:
- if (pageId % 10n === 0n) { - const progress = Math.round(((Number(pageId) * step) / Number(rcount)) * 100); - report({ - message: `record finished ${Number(pageId * BigInt(step))} / ${Number(rcount)} (${progress}%)`, - }); - } + if (pageId % 10n === 0n) { + // Safe conversion - ensure values don't exceed MAX_SAFE_INTEGER + const pageIdNum = pageId <= Number.MAX_SAFE_INTEGER ? Number(pageId) : Infinity; + const rcountNum = rcount <= Number.MAX_SAFE_INTEGER ? Number(rcount) : Infinity; + const processedNum = (pageId * BigInt(step)) <= Number.MAX_SAFE_INTEGER ? + Number(pageId * BigInt(step)) : Infinity; + + const progress = Math.round(((pageIdNum * step) / rcountNum) * 100); + report({ + message: `record finished ${processedNum} / ${rcountNum} (${progress}%)`, + }); + }packages/ui-default/index.ts (1)
77-77: Configuration source updated to use context-specific settingsThis change moves from using a static configuration source to a context-specific one, aligning with the broader refactoring toward using the new ConfigService.
Add error handling to ensure the code doesn't break if the config settings are missing:
- const schema = convert(Schema.intersect(this.ctx.config.settings) as any, true); + const settings = this.ctx.config?.settings || {}; + const schema = convert(Schema.intersect(settings) as any, true);packages/import-qduoj/index.ts (1)
31-37: Improved type safety for optional SPJ fieldThe change to make
spja union type withSchema.never()increases type safety by explicitly modeling the absence of SPJ information. This is better than just making the field optional.Consider adding a comment to explain this pattern for clarity:
spj: Schema.union([ + // Using Schema.never() to explicitly represent cases where SPJ doesn't exist, + // providing better type safety than a simple optional field Schema.never(), Schema.object({ language: Schema.string().required(), code: Schema.string().required(), }), ]),packages/hydrooj/src/entry/worker.ts (2)
41-45: Avoid reassigningctx.
Reassigningctxinside a promise may cause confusion. Considering a different variable name can improve clarity.- ctx = await new Promise((resolve) => { + const updatedCtx = await new Promise((resolve) => {
50-54: Same overshadowing concern.
Similarly, reassigningctxin another promise can hamper readability. A distinct variable name is preferable.packages/hydrooj/src/service/db.ts (1)
188-193: Deprecated fallback proxy.
Thisdbproxy depends on a globalappreference and could create unexpected behavior ifappis unavailable. Consider removing or gating usage more explicitly.packages/hydrooj/src/service/storage.ts (1)
391-415: Security check for proxy route.
The rewrite logic and proxy usage are fine. However, consider adding security or rate-limiting to avoid exposing files unintentionally.packages/hydrooj/src/loader.ts (4)
50-50: Consider specifying a more descriptive type forstate
UsingRecord<string, EffectScope>is acceptable. However, static analysis warns against using{}(observed in the subsequent line forpublic config). If feasible, define a more robust interface or at least aRecord<string, unknown>to improve clarity and enforce type safety.
58-62: Handle interval cleanup
You are establishing a repeating check withctx.setInterval(...)to log plugin states. Consider clearing this interval if the service is disposed or if the app shuts down, to avoid potential memory leaks or calls after the context is no longer valid.
100-108: Returning an empty object if no schema
The logic to return{}for plugins without a schema is convenient but may hide silent misconfigurations. Consider logging a warning or providing a default config to improve debuggability.
147-152: Assigningglobal.app
Usingglobal.appmight introduce maintainability issues. If your codebase standardizes around it, that's acceptable, but stay mindful if multiple modules attempt to modifyglobal.appconcurrently.packages/hydrooj/src/settings.ts (4)
17-20: Type definitions for configurations
UsingSchema[]andanymight limit type safety. If possible, refinesystemConfigto a structured type instead ofanyto reduce potential runtime errors.
29-39: Error handling for config loading
This function parses YAML, logs on failure, and emits an event on success. Consider providing a fallback to default settings if parsing fails, to prevent partial system functionality.
77-83: Configuration getter
Dot-notation traversal is handy but can fail silently if any subpath is undefined. Consider optional safety checks or default return values for more robust behavior.
85-119: Dynamic config proxy
Providing aProxyfor config updates is powerful but can be harder to debug and can add overhead. Still, this pattern fosters a neat, reactive approach to configuration management.packages/hydrooj/src/model/setting.ts (1)
82-119: Well-structured schema conversion functionThe
schemaToSettingsfunction is well implemented with proper error handling for unsupported structures. It correctly processes schema metadata and maps schema types to corresponding setting types.Consider adding a type guard or assertion for the return value of
schemaToSettingsto ensure type safety when used in the settings functions:-function schemaToSettings(schema: Schema<any>) { +function schemaToSettings(schema: Schema<any>): _Setting[] {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
README.md(1 hunks)packages/hydrooj/src/entry/cli.ts(3 hunks)packages/hydrooj/src/entry/common.ts(2 hunks)packages/hydrooj/src/entry/worker.ts(4 hunks)packages/hydrooj/src/handler/manage.ts(1 hunks)packages/hydrooj/src/handler/misc.ts(1 hunks)packages/hydrooj/src/interface.ts(0 hunks)packages/hydrooj/src/loader.ts(7 hunks)packages/hydrooj/src/model/setting.ts(9 hunks)packages/hydrooj/src/service/db.ts(3 hunks)packages/hydrooj/src/service/server.ts(6 hunks)packages/hydrooj/src/service/storage.ts(7 hunks)packages/hydrooj/src/settings.ts(1 hunks)packages/hydrooj/src/upgrade.ts(2 hunks)packages/hydrooj/src/utils.ts(2 hunks)packages/import-qduoj/index.ts(1 hunks)packages/migrate/scripts/hustoj.ts(2 hunks)packages/ui-default/index.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/hydrooj/src/interface.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hydrooj/src/entry/common.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/hydrooj/src/handler/manage.ts
[error] 198-198: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
packages/hydrooj/src/loader.ts
[error] 51-51: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🪛 GitHub Check: CodeQL
packages/hydrooj/src/settings.ts
[warning] 51-51: Prototype-polluting assignment
This assignment may alter Object.prototype if a malicious 'proto' string is injected from library input.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (76)
README.md (1)
125-139: Well-documented changes in the updateGood job documenting all the major changes in this update, especially architectural changes like the upgrade to cordis@4, the removal of global services, and the support for Schemastery in Settings.
These changes provide users with clear information about what to expect in this version.
packages/import-qduoj/index.ts (1)
108-115:✅ Verification successful
Verify SPJ language check
The current code only handles SPJ in C++. After making
spjoptional with the union type, ensure this conditional still behaves as expected.
🏁 Script executed:
#!/bin/bash # Find all places where spj.language is checked to verify consistency rg "spj.*language" -A 5 -B 5Length of output: 3257
SPJ language conditional verified
I've confirmed via a repository-wide search that the only check on
spj.languagein the qduoj import code is on line 108 ofpackages/import-qduoj/index.ts, where it evaluatesif (pdoc.spj?.language === 'C++'). This optional chaining ensures that ifspjis undefined (as allowed by the new union type), the condition safely returns false, thereby avoiding any unintended execution of the SPJ-related logic.Please double-check that supporting only the C++ SPJ is intentional. If additional SPJ languages should be supported in the future, adjustments to this conditional might be necessary.
packages/hydrooj/src/handler/misc.ts (1)
112-112: Improved configuration management approach.The change to retrieve the file secret from context configuration instead of a static import is a good improvement. This promotes better dependency injection and makes the code more testable and configurable.
packages/hydrooj/src/entry/cli.ts (5)
9-11: Updated imports for service-based architecture.Good restructuring to support the new service-based approach.
114-116: Improved service initialization with dependency injection.The refactoring from direct service calls to plugin-based initialization improves modularity and follows better dependency injection practices.
117-122: Enhanced dependency handling with Promise-based injection.This change ensures that the loader dependency is properly injected before proceeding. This is a good pattern for managing asynchronous dependencies.
133-137: Consistent implementation of Promise-based dependency injection.Similar to the loader injection, this ensures the server dependency is available before proceeding with addon operations.
141-142: Simplified plugin reloading.The removal of the ctx parameter from reloadPlugin call simplifies the function signature while maintaining functionality.
packages/hydrooj/src/upgrade.ts (2)
425-435: Centralized configuration management.The migration from
setBuiltinConfigtoapp.get('config').setConfigaligns with the project's move toward a service-based architecture for configuration management. This makes configuration handling more consistent and maintainable.
704-704: Version marker added.The comment indicating the start of Hydro v5 provides useful versioning information for future reference.
packages/hydrooj/src/utils.ts (2)
1-2: Added import for enhanced null checking.Good addition of the
isNullableutility from cosmokit for more robust null/undefined checks.
22-29: Improved module export handling logic.The refactored
unwrapExportsfunction is more readable and explicit in handling different cases. The updated structure with individual condition checks improves maintainability and the added comments about ESBuild interoperability provide helpful context.packages/hydrooj/src/entry/worker.ts (6)
10-11: Smooth import additions.
No issues found here; the new imports forMongoServiceandConfigServiceappear straightforward.
26-26: Verify null or empty string scenarios.
IfMongoService.getUrl()returns''orundefined, ensureif (!url)correctly handles that case.
37-38: Registration order looks fine.
Plugging inMongoServiceandConfigServiceis consistent with the new approach.
46-46: Plugin reload looks correct.
No problems found with reloading thestorageservice plugin.
69-69: Plugin reload usage.
Reloading the handler plugin is consistent with your pattern.
76-76: No issues found.
The script plugin reload mirrors the handler pattern with no evident problems.packages/hydrooj/src/handler/manage.ts (4)
186-186: Circumvent direct config usage.
Referencingthis.ctx.config.configSourceis in line with the newConfigService; no immediate concerns.
188-188: Verify schema intersection behavior.
Ensure partial or invalid YAML doesn't yield downstream errors, though the try/catch is likely sufficient.
191-191: Publicly exposing schema.
Generating a JSON representation of the intersected schema is a clean approach.
205-205: Saving config seamlessly.
The call tothis.ctx.config.saveConfig(config)fits the new config system well.packages/hydrooj/src/service/db.ts (9)
7-7: New import is consistent.
Bringing inContextandServicealigns well with the updated architecture.
11-11: No concerns withbusimport.
Using this default import is a standard approach.
29-33: Typed context extension.
Addingdb: MongoServicetoContextimproves type safety and clarity.
35-35: MongoService class hierarchy.
ExtendingServiceis a straightforward design to group DB logic.
39-41: Constructor pattern looks good.
super(ctx, 'db')is consistent with naming for the service.
43-49: Behavior in CI environment.
getUrl()is managing in-memory Mongo for CI. Verify that production builds never setCIinadvertently.
58-59: Service setup method.
Using[Service.setup]neatly triggers the initialization. Looks correct.
64-64: Periodic collection maintenance.
fixExpireAfter()each hour is generally fine, but watch out for performance overhead on large sets.
68-70: Custom collection mapping.
LeveragingcollectionMapis a clean solution for flexible naming.packages/hydrooj/src/service/storage.ts (10)
15-15: Introduction ofkoa-proxies.
Importingproxyis reasonable for the new approach to file routing.
17-17:nanoidusage is appropriate.
A short, unique secret is a solid approach for config defaults.
18-18: Schemastery import.
Ensures typed validation for storage configuration.
63-65: Check default path logic.
Usingprocess.env.CIplus a fallback is sensible. Confirm environment variables won't break references.
66-96: ComprehensiveFileSettingschema.
Good job unifying local and S3 storage definitions viaSchema.intersect().
98-99: ExportingConfig.
Re-exportingFileSettingunderConfigis clear enough.
110-111: RemoteStorageService constructor.
Injecting typed config fosters clarity for S3 usage.
307-308: LocalStorageService constructor.
Similarly adopting typed config for local storage is consistent.
383-389: Adaptive storage instantiation.
Falling back to local iftype === 'file'is logical. You may want to guard against unknown types.
419-425: Deprecated proxy retained.
Keeping a fallback export is fine but do encourage consumers to usectx.storage.packages/hydrooj/src/loader.ts (8)
17-19: Imports of context types are consistent.
These imports appear appropriate, and all the symbols (Context,Service,ScopeStatus,EffectScope) are indeed used in the file.
24-24: Schemastery import usage
Using theschemasteryimport for configuration schema handling is appropriate. Looks good.
56-56: Dependency injection array
This injection array (['config', 'timer', 'i18n', 'logger']) matches your usage. No issues found.
110-114:reloadPluginmethod reuses the plugin object
The method updates the plugin config and optionally overridesplugin.name. Ensure that overwritingplugin.namedoes not disrupt other references or logging flows. Overall, the approach for retrieving and applying the plugin config is sound.
121-121: Initialize plugin with resolved config
This is a straightforward call to initialize the plugin. Verify that each plugin can properly handle the passed-in config object to avoid unexpected runtime errors.
135-136: Verbose error logging
Logging errors when plugin resolution fails is helpful for debugging. Ensure thaterrdoes not contain sensitive information that might be exposed in logs.
172-172: Synchronous loading
Callingawait preload()ensures required dependencies and data are fully loaded before proceeding. This improves reliability of the loading process.
211-211: Ensuring CLI preload
Awaitingpreload()inloadCli()helps unify the initialization path for CLI usage, maintaining consistency with the mainload()method.packages/hydrooj/src/service/server.ts (6)
7-9: Modular import from@hydrooj/framework
ImportingConnectionHandlerOriginal,HandlerOriginal, and related types from the framework improves clarity and modular design.
31-31: Introducing generic parameter C
ExtendingHandlerCommonto accept a generic context type fosters greater type flexibility. Ensure that downstream references use the new parameter consistently.
74-74: Generic class extension forHandler
SwitchingHandlerto extendHandlerOriginal<Context>clarifies the expected context type, enhancing type safety.
77-77: Consistent usage of typedContextinConnectionHandler
UsingConnectionHandlerOriginal<Context>aligns with the approach inHandler, ensuring uniform type handling.
82-82: RegisteringWebService
Replacing direct framework usage with theWebServiceplugin helps maintain a modular structure. Verify that prior functionality remains accessible through this plugin integration.
259-260: Early error throw in WS handler
Immediately rejecting ifh.context.pendingErroris set prevents initiating any incomplete WebSocket operations. This “fail fast” method is beneficial for robust error handling.packages/hydrooj/src/settings.ts (9)
3-3: Context and Service import
Importing these for the new settings architecture appears consistent with the overall service approach.
8-10: Augmenting the Context interface
Addingconfig: ConfigServiceensures configuration logic is conveniently accessible across the application.
14-15: NewConfigServicewith DB injection
Deriving fromServicewhile injecting the DB dependency centralizes config logic. This is a cohesive design choice.
21-21: Constructor ensures naming
Usingsuper(ctx, 'config')clearly identifies this service. Maintains consistency with your injection pattern.
25-27: Service setup ensures config loading
Overriding[Service.setup]to invokeloadConfig()ensures early retrieval of settings, aligning with service lifecycle expectations.
55-63: Validation synergy
isPatchValidreusesapplyDeltaandSchema.intersectto verify patches. Ensure all update flows call this method to avoid bypassing checks.
65-71: Schema intersection before saving
PerformingSchema.intersect(this.settings)(config)ensures the config adheres to expected schemas before persistence. Nice defensive measure.
72-75: Granular config updates
UsingapplyDeltafor partial config changes is flexible. Validate that multi-level paths are tested, especially if keys or sub-keys are missing.
120-121: No additional functional changes
These lines appear to close out the class with no further modifications.packages/hydrooj/src/model/setting.ts (12)
3-3: Package dependency update from 'cordis' to '@cordisjs/core'This change aligns with the broader project update to use the namespaced
@cordisjs/corepackage instead of the originalcordispackage.
7-7: Added Schema import from 'schemastery'The addition of Schema import is necessary to support the new schema-based settings functionality in this file.
74-80: Good implementation of Schemastery namespace declarationThe global declaration for Schemastery namespace properly defines the Meta interface with a family property, which will be used for schema metadata.
121-122: Improved settings function to support Schema objectsThe
PreferenceSettingfunction now accepts both _Setting arrays and Schema objects, using the newschemaToSettingsfunction to convert Schema objects to _Setting arrays when needed.
141-142: Improved AccountSetting function to support Schema objectsSimilar to PreferenceSetting, the AccountSetting function now supports Schema objects as input.
161-162: Improved DomainUserSetting function to support Schema objectsDomainUserSetting now accepts Schema objects and converts them to _Setting objects appropriately.
177-178: Improved DomainSetting function to support Schema objectsDomainSetting function now handles Schema objects, consistent with the other setting functions.
193-194: Improved SystemSetting function to support Schema objectsSystemSetting now supports Schema objects, completing the consistent pattern across all setting functions.
351-351: Updated module declaration to use '@cordisjs/core'This change is consistent with the import update at the top of the file, ensuring type compatibility with the new package.
380-380: Added dependency injection specificationThe addition of the 'inject' export specifies that the 'db' service is a dependency for this module, making dependencies explicit.
388-388: Improved database access with context-aware approachChanged from using the global database service to using the context-bound database service, improving modularity and testability.
412-413: Updated model export to include the inject propertyThis ensures that the dependency requirements are properly exposed in the global model object.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
framework/framework/server.ts (3)
128-129:⚠️ Potential issueFix unsafe declaration merging and empty interface
The empty interface followed by a class with the same name creates unsafe declaration merging in TypeScript. The compiler won't check if properties defined in the interface are initialized in the class.
-export interface HandlerCommon<C> { } // eslint-disable-line @typescript-eslint/no-unused-vars +// Use a type alias instead if needed, or remove entirely as the class already defines the structure export class HandlerCommon<C> {🧰 Tools
🪛 Biome (1.9.4)
[error] 128-128: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 129-129: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
481-483: 🛠️ Refactor suggestionCreate proper type definitions for event handlers
Multiple type casts to
anywith FIXME comments indicate a need for proper typing of event handlers.Consider creating a proper event definition interface:
interface HandlerEvents<T> { 'handler/create': [handler: T, type: 'http' | 'ws']; 'handler/create/http': [handler: T]; // Define other event types similarly } // Then extend the Context interface to include typed events
676-676:⚠️ Potential issueFix condition that always evaluates to false
The condition
if (perm)is flagged by static analysis as always evaluating to false.The issue might be that
permis initialized asundefinedand the type checking logic doesn't properly assign it. Consider checking ifpermis being assigned correctly in the loop above:- if (perm) this.checkPerm(perm); + if (perm !== undefined) this.checkPerm(perm);Alternatively, initialize
permwith a falsy value that's not undefined:- let perm: bigint; + let perm: bigint | undefined;🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 676-676: Useless conditional
This use of variable 'perm' always evaluates to false.
🧹 Nitpick comments (3)
framework/framework/server.ts (3)
146-146: Remove explicit any type assertionType assertions to
anydefeat TypeScript's type checking. Consider creating a proper type definition for the context extension method.- this.ctx = (ctx as any).extend({}); + this.ctx = ctx.extend({});If the
extendmethod isn't properly typed in the context type, consider enhancing the type definition instead of usingany.
187-188: Avoid type assertions to any for renderersInstead of using
any, properly typethis.ctx.server.renderers.- const renderers = Object.values((this.ctx as any).server.renderers as Record<string, Renderer>) + const renderers = Object.values(this.ctx.server.renderers as Record<string, Renderer>)
708-710: Address FIXME comment with proper type definitionThe FIXME comment indicates a need for proper typing of the
Routemethod parameter.Define a proper type for RouteHandler instead of using
any:- // FIXME: should be typeof Handler<Context> instead of any - // eslint-disable-next-line @typescript-eslint/naming-convention - public Route(name: string, path: string, RouteHandler: any, ...permPrivChecker) { + // eslint-disable-next-line @typescript-eslint/naming-convention + public Route<T extends CordisContext = C>(name: string, path: string, RouteHandler: typeof Handler<T>, ...permPrivChecker) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
framework/framework/server.ts(21 hunks)packages/hydrooj/src/entry/cli.ts(3 hunks)packages/hydrooj/src/entry/worker.ts(4 hunks)packages/hydrooj/src/model/setting.ts(10 hunks)packages/hydrooj/src/model/user.ts(3 hunks)packages/hydrooj/src/plugin-api.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hydrooj/src/plugin-api.ts
🧰 Additional context used
🪛 Biome (1.9.4)
framework/framework/server.ts
[error] 128-128: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 129-129: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
🪛 GitHub Check: CodeQL
framework/framework/server.ts
[warning] 676-676: Useless conditional
This use of variable 'perm' always evaluates to false.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (31)
packages/hydrooj/src/model/user.ts (3)
12-12: Updated bus import patternThe import statement for bus has been changed from namespace import to default import, which aligns with the broader pattern change across the codebase mentioned in the PR summary.
212-214: Added static references to utility functionsAdding static references to existing functions enhances the class interface by making these utilities accessible through the UserModel class directly. This improves code organization and follows good OOP principles.
471-489: Introduced async initialization patternThe new
applyfunction replaces the previous event listener approach with a more explicit initialization pattern. This is a good architectural improvement that:
- Makes the initialization sequence more deterministic
- Uses Promise.all for efficient parallel database index creation
- Aligns with the asynchronous initialization pattern mentioned in the PR summary
This approach provides better control over the application startup sequence.
packages/hydrooj/src/entry/cli.ts (5)
9-11: Updated imports to support the new service architecture.The imports have been updated to align with the new architecture, introducing
loadOptions,MongoService, andConfigService. This is part of the broader refactoring to improve dependency management.
114-115: Services now initialized as plugins rather than direct calls.The direct calls to database initialization and config loading have been replaced with a more modular plugin-based approach. This improves testability and allows for better dependency management.
117-122: Added async context injection for the loader.This Promise-based pattern ensures that the loader dependency is available before proceeding, which is a more robust approach to handling dependencies. This aligns with the async refactoring across the codebase.
133-137: Added async context injection for the server component.Similar to the loader injection, this ensures the server component is fully initialized before proceeding with addon loading and other operations that might depend on it.
141-142: Simplified plugin reloading call.The plugin reloading call has been simplified by removing the explicit ctx parameter, which is now handled internally. This is a cleaner approach that reduces parameter passing.
packages/hydrooj/src/entry/worker.ts (7)
10-11: Updated imports for the new service architecture.Similar to cli.ts, imports have been updated to support the new plugin-based service architecture with
MongoServiceandConfigService.
26-30: Improved database URL retrieval with async pattern.Now uses an async approach to retrieve the MongoDB URL before initializing the setup process if needed. This provides better flow control and error handling.
37-38: Services now initialized as plugins.Consistent with the changes in cli.ts, database and configuration services are now initialized as plugins, enhancing modularity.
41-48: Added async context injection and improved plugin initialization.The code now ensures the loader is fully initialized before proceeding and introduces better plugin management. The addition of HMR service with a watch option enhances development capabilities.
50-54: Added async context injection for the server component.Similar to the loader injection, this ensures proper initialization sequencing for components that depend on the server.
69-70: Simplified plugin reloading for handlers.Removed unnecessary ctx parameter in the reloadPlugin call, consistent with other similar changes in the codebase.
76-77: Simplified plugin reloading for scripts.Similar simplification as with handlers, maintaining consistency in the plugin loading approach across the codebase.
packages/hydrooj/src/model/setting.ts (13)
3-8: Updated imports to use namespaced package and added Schema support.The import from 'cordis' has been updated to '@cordisjs/core', and Schema from 'schemastery' has been added to support the new declarative settings approach.
74-80: Added global declaration for Schemastery namespace.This extends the global type definitions to include the Schemastery namespace with a Meta interface that includes a 'family' property. This enables type safety when working with schema metadata.
82-119: Added schemaToSettings function for converting Schema to Setting objects.This utility function enables a more declarative approach to defining settings using Schema objects. It correctly handles various schema types and includes appropriate error handling for unsupported structures.
121-123: Updated PreferenceSetting to support Schema objects.The function now accepts both _Setting arrays and Schema objects, with conversion handled through the new schemaToSettings function. This provides more flexibility in how settings are defined.
141-143: Updated AccountSetting to support Schema objects.Similar to PreferenceSetting, this function now supports both traditional setting arrays and schema-based definitions.
161-163: Updated DomainUserSetting to support Schema objects.Consistent with other setting functions, this now supports schema-based definitions.
177-179: Updated DomainSetting to support Schema objects.Consistent with other setting functions, this now supports schema-based definitions.
193-195: Updated SystemSetting to support Schema objects.Consistent with other setting functions, this now supports schema-based definitions.
290-317: Added Schema-based setting definition example.This showcases the new declarative approach to defining settings using Schema objects. The approach provides better type safety and structure compared to the previous array-based approach.
359-363: Updated module declaration to reference @cordisjs/core.This aligns with the import change at the top of the file, maintaining consistency in the module references.
388-388: Added inject property to specify database dependency.This explicitly declares the database dependency, improving clarity around required services.
396-397: Updated database access to use context.Changed from using the global Hydro service to ctx.db, which is more aligned with the context-based approach used throughout the codebase.
420-421: Added inject property to global model.Ensures the dependency information is available to the global model, maintaining consistency with the service-based approach.
framework/framework/server.ts (3)
719-725: Nice improvement with effect-based registrationUsing
ctx.effect()for layer registration is a good pattern as it automatically handles cleanup when the context is disposed.
749-763: Good implementation of handler mixins with proper cleanupThe implementation of mixins using
ctx.effect()ensures proper registration and cleanup of prototype modifications, enhancing the maintainability of the codebase.
807-813: Good use of module augmentation for type safetyThe module declaration enhances type safety by properly typing the context extensions, ensuring correct types are used throughout the application.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
framework/framework/server.ts (4)
129-130: 🛠️ Refactor suggestionFix unsafe declaration merging.
The empty interface
HandlerCommon<C>followed by a class with the same name creates an unsafe declaration merging pattern in TypeScript. The linter also flags this as problematic.Consider using a type alias instead of an empty interface:
-export interface HandlerCommon<C> { } // eslint-disable-line @typescript-eslint/no-unused-vars +// Use a type alias if you need to augment the class type +export type HandlerCommonType<C> = HandlerCommon<C>; export class HandlerCommon<C> {🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 130-130: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
487-489: 🛠️ Refactor suggestionAddress type checking TODOs for event emission.
Several type casts to
anyhave been added withFIXMEcomments for event-related methods.Create proper type definitions for the event handlers to maintain type safety throughout the codebase:
// Add to your interface file interface HandlerEvents<T> { 'handler/create': [handler: T, type: 'http' | 'ws']; 'handler/create/http': [handler: T]; // Add other event types } // Then update the Context interface to include these typed events
662-686:⚠️ Potential issueFix condition that always evaluates to false.
In the
Checkerfunction, thepermvariable is being initialized incorrectly, causing theif (perm)condition at line 683 to always evaluate to false.The issue is that you're using assignment without updating the variable correctly:
const Checker = (args) => { - let perm: bigint; - let priv: number; + let perm: bigint | undefined; + let priv: number | undefined; let checker = () => { }; for (const item of args) { if (typeof item === 'object') { if (typeof item.call !== 'undefined') { checker = item; } else if (typeof item[0] === 'number') { - priv = item; + priv = item[0]; } else if (typeof item[0] === 'bigint') { - perm = item; + perm = item[0]; } } else if (typeof item === 'number') { priv = item; } else if (typeof item === 'bigint') { perm = item; } } return function check(this: Handler<C>) { checker(); if (perm) this.checkPerm(perm); if (priv) this.checkPriv(priv); }; };🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 683-683: Useless conditional
This use of variable 'perm' always evaluates to false.
794-796:⚠️ Potential issueFix wsHandlerMixin implementation.
The
wsHandlerMixinmethod warns about existing properties but doesn't actually assign the new properties, making it ineffective.All the other mixin methods use
_applyMixinwhich properly assigns properties, but this method doesn't:public wsHandlerMixin(MixinClass: Partial<ConnectionHandler<C>>) { - return this._applyMixin(ConnectionHandler, MixinClass); + this._applyMixin(ConnectionHandler, MixinClass); + return; }
🧹 Nitpick comments (1)
framework/framework/server.ts (1)
147-147: Avoid type casting when extending context.The use of
as anywhen calling.extend()on the context circumvents type checking, which contradicts the PR's goal of improving type safety.Consider creating a proper type for the context's extend method:
-this.ctx = (ctx as any).extend({}); +this.ctx = ctx.extend({});Add proper type definitions for the extend method in the CordisContext interface if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
build/prepare.js(1 hunks)framework/framework/server.ts(21 hunks)
✅ Files skipped from review due to trivial changes (1)
- build/prepare.js
🧰 Additional context used
🪛 Biome (1.9.4)
framework/framework/server.ts
[error] 129-129: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 130-130: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
🪛 GitHub Check: CodeQL
framework/framework/server.ts
[warning] 683-683: Useless conditional
This use of variable 'perm' always evaluates to false.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (3)
framework/framework/server.ts (3)
809-816: Good use of TypeScript module augmentation.The module declaration correctly extends the
@cordisjs/coreContext interface with the WebService methods using thethistype to maintain type relationships.This is a good pattern for extending external types while preserving type safety and self-referential relationships.
756-778: Great implementation of the _applyMixin method.The new
_applyMixinmethod is well-designed, featuring:
- Type checking for the target
- Clean setup and teardown through the effect pattern
- Proper warning messages for existing properties
- Restoration of original values during cleanup
This represents a significant improvement over the previous implementation.
201-203: Good use of generic defaults.The Handler class now properly uses generics with default types, improving both flexibility and backward compatibility.
export class Handler<C = CordisContext> extends HandlerCommon<C> {This approach allows existing code to work without changes while enabling new code to specify custom context types.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/ui-next/package.json (2)
1-4: Metadata and Versioning Check
The package metadata is clearly defined with a scoped name, module type, and an initial version of "0.0.0". Using "0.0.0" clearly signals a development or pre-release version, so ensure you update it to a proper semantic version before a production release.
16-28: DevDependencies Package Review
The devDependencies are well-chosen to support linting, TypeScript, and Vite. A couple of points to verify:
- The package
"typescript-eslint": "^8.26.0"is included; however, commonly the TypeScript ESLint setup involves packages like@typescript-eslint/parserand@typescript-eslint/eslint-plugin. Please confirm that"typescript-eslint"is the correct package name as intended.- There is a potential overlap between
"@vitejs/plugin-react"in dependencies and"@vitejs/plugin-react-swc"in devDependencies. Review whether both are needed or if one is preferred over the other for your build process.packages/ui-next/index.ts (5)
7-11: Remove unused imports.There are multiple imports declared but never used in the code. This creates unnecessary dependencies and clutters the codebase.
Consider removing the following unused imports:
-import { } from '@hydrooj/framework'; -import { - BadRequestError, Context, db, DocumentModel, Handler, ObjectId, param, ProblemModel, RecordModel, SettingModel, - StorageModel, -} from 'hydrooj'; +import { Context } from 'hydrooj';🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 8-11: Unused variable, import, function or class
Unused imports BadRequestError, DocumentModel, Handler, ObjectId, ProblemModel, RecordModel, SettingModel, StorageModel, db, param.
25-25: Consider makingallowedHostsconfigurable.The allowed hosts are currently hardcoded to
['beta.hydro.ac'], which may limit the flexibility of this module when used in different environments.Consider making this configurable through environment variables or configuration parameters:
- allowedHosts: ['beta.hydro.ac'], + allowedHosts: process.env.VITE_ALLOWED_HOSTS ? process.env.VITE_ALLOWED_HOSTS.split(',') : undefined,
34-35: Avoid using@ts-ignorecomments.The use of
@ts-ignorebypasses TypeScript's type checking, which can lead to runtime errors.Instead of ignoring type errors, consider properly typing the import or plugin:
- // @ts-ignore - importMetaUrlPlugin, + importMetaUrlPlugin as any,Alternatively, create a proper type definition for the plugin if it's a regular dependency.
13-62: Add error handling for Vite server creation.The function lacks error handling for the Vite server creation process, which could lead to unhandled promise rejections.
Consider wrapping the server creation in a try-catch block:
export async function apply(ctx: Context) { if (process.env.HYDRO_CLI) return; + let vite; + try { const vite = await createServer({ // ... server configuration }); + } catch (error) { + console.error('Failed to initialize Vite server:', error); + return; + } // Rest of the function... ctx.on('dispose', async () => { await vite.close().catch((e) => console.error(e)); }); }
19-19: Consider making HMR port configurable.The HMR port is hardcoded to 3010, which might conflict with other services or be unavailable in certain environments.
Consider using an environment variable with a fallback:
- port: 3010, + port: parseInt(process.env.VITE_HMR_PORT || '3010', 10),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.yarn/releases/yarn-4.0.2.cjsis excluded by!**/.yarn/**.yarn/releases/yarn-4.7.0.cjsis excluded by!**/.yarn/**
📒 Files selected for processing (12)
.yarnrc.yml(1 hunks)build/prepare.js(3 hunks)package.json(4 hunks)packages/hydrooj/package.json(3 hunks)packages/ui-default/backendlib/markdown-it-katex.ts(1 hunks)packages/ui-default/package.json(4 hunks)packages/ui-next/index.html(1 hunks)packages/ui-next/index.ts(1 hunks)packages/ui-next/package.json(1 hunks)packages/ui-next/src/App.tsx(1 hunks)packages/ui-next/src/main.tsx(1 hunks)packages/ui-next/src/vite-env.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/ui-next/src/App.tsx
- packages/ui-next/src/vite-env.d.ts
- packages/ui-next/src/main.tsx
- packages/ui-next/index.html
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- packages/hydrooj/package.json
- build/prepare.js
- packages/ui-default/package.json
🧰 Additional context used
🪛 GitHub Check: CodeQL
packages/ui-next/index.ts
[notice] 8-11: Unused variable, import, function or class
Unused imports BadRequestError, DocumentModel, Handler, ObjectId, ProblemModel, RecordModel, SettingModel, StorageModel, db, param.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (6)
packages/ui-default/backendlib/markdown-it-katex.ts (1)
114-114: Type assertion improves type safetyThe change from
strict: 'ignore'tostrict: 'ignore' as 'ignore'improves type safety by explicitly asserting the string literal type. This ensures TypeScript treats this as a specific literal value rather than a general string, which is beneficial if KaTeX expects specific literals for this option.packages/ui-next/package.json (2)
5-10: Scripts Configuration Validation
The scripts defined for development, building, linting, and previewing (using Vite) are appropriate for a modern TypeScript + React project. The command"tsc -b && vite build"effectively builds both TypeScript and the Vite bundles. Consider adding additional scripts (e.g., a watch mode) if required by your development workflow, though the current set is sufficient for a standard setup.
11-15: Dependency Versions Verification
The dependencies section includes core packages like React and React DOM. However, specifying"react": "^19.0.0"and"react-dom": "^19.0.0"is unusual because the latest stable React versions are commonly in the 18.x range. Please verify that version 19.x is intentional—if it’s experimental or a pre-release, consider adding appropriate tags in the documentation or double-check for typos..yarnrc.yml (3)
1-2: Disable Telemetry for Enhanced Privacy
SettingenableTelemetry: falseensures that Yarn does not collect telemetry data. This aligns with our privacy and performance goals, especially important for consistent behavior across environments.
9-10: Optimize Dependency Management with 'preferReuse'
AddingpreferReuse: truepromotes the reuse of cached dependencies, which can lead to faster installs and more predictable builds.
13-14: Upgrade Yarn Version for Improved Tooling
Updating theyarnPathto.yarn/releases/yarn-4.7.0.cjsupgrades Yarn to the newer version. Please verify that this version is compatible with all project tooling and that no regressions occur in dependency resolution.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/hydrooj/src/service/storage.ts (1)
382-382: Minor: Consider using a more descriptive variable nameThe variable
serviceis quite generic. Consider using a more descriptive name likestorageServiceoractiveStorageServiceto improve code readability.-let service; +let storageService;packages/ui-default/pages/setting.page.tsx (1)
114-114: Consider usingstructuredClone.
For modern browsers,structuredClone(initial)can be used instead ofJSON.parse(JSON.stringify(initial)).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/hydrooj/src/handler/manage.ts(2 hunks)packages/hydrooj/src/service/storage.ts(7 hunks)packages/ui-default/components/monaco/index.ts(1 hunks)packages/ui-default/components/monaco/loader.ts(1 hunks)packages/ui-default/index.ts(2 hunks)packages/ui-default/package.json(5 hunks)packages/ui-default/pages/setting.page.tsx(1 hunks)packages/ui-default/templates/manage_config.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui-default/index.ts
- packages/ui-default/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui-default/pages/setting.page.tsx
[error] 35-35: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
packages/hydrooj/src/handler/manage.ts
[error] 198-198: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (26)
packages/hydrooj/src/service/storage.ts (6)
63-96: Configuration schema implementation looks goodThe new configuration schema using
schemasteryprovides a clear structure for both local and S3 storage providers. The schema includes proper defaults, internationalization support, and appropriate typing.
110-111: Good type safety improvementUpdating the constructor to accept a strongly typed configuration parameter improves type safety and makes the dependency explicit.
307-308: Good type safety improvementSimilar to the RemoteStorageService, this constructor update improves type safety and dependency management.
384-417: Well-structured service initialization with context integrationThe new
applyfunction properly initializes the storage service based on configuration and registers it with the context. The proxy middleware setup for handling file system requests is well-implemented.
419-425: Good deprecation noticeProperly marking the direct service export as deprecated while providing guidance to use the context-based approach aligns with the architectural improvements in this PR.
363-363: Updated secret source correctlyThe change from using a global config to the instance-specific configuration is correctly implemented.
packages/hydrooj/src/handler/manage.ts (3)
198-198: Remove the empty object pattern.Static analysis flags
{ }as an unexpected empty pattern. Consider removing or renaming it.- async post({ }, value: string) { + async post(value: string) {🧰 Tools
🪛 Biome (1.9.4)
[error] 198-198: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
184-206: Good dependency injection implementation.Using
this.ctx.get('config')to access configuration methods and properties decouples the code from direct imports, improving testability and maintainability.🧰 Tools
🪛 Biome (1.9.4)
[error] 198-198: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
324-324: Proper dependency declaration.The
injectarray properly declares that this module requires the 'config' service to be injected, which complements the dependency injection pattern used in the code.packages/ui-default/components/monaco/index.ts (1)
11-11: Clean export of renderMarkdown function.Good addition that makes the
renderMarkdownfunction from monaco-editor accessible to module consumers.packages/ui-default/components/monaco/loader.ts (1)
88-93: Properly expose renderMarkdown in loader API.This change correctly exposes the newly exported
renderMarkdownfunction through the loader's return object, maintaining consistency with the module's API design.packages/ui-default/templates/manage_config.html (1)
3-9: UI modernization for configuration management.The template has been refactored from a form-based UI to what appears to be a JavaScript-driven approach (with the
appdiv). This change aligns with the backend dependency injection pattern changes and likely provides a more dynamic user experience.packages/ui-default/pages/setting.page.tsx (14)
1-2: Imports look good.
These imports help ensure the required styling is available.
5-6: React imports confirmed.
No issues found; the usage ofReactandReactDOMappears standard.
8-8: Hybrid integration approach looks fine.
Applying Vue within React viaveauryis a neat approach.
11-11: Utility imports confirmed.
No concerns; these utilities appear to be used for network requests and localization.
13-16: Localization mapping is valid.
Defining language codes is straightforward; consider adding a default or fallback if needed, else this is fine.
19-24: Code-splitting strategy is noted.
Awaiting multiple imports in parallel is efficient.
25-26: Reactive form bridging looks clean.
Bridging a Vue-based KForm into React is implemented correctly.
37-49: Vue app setup is good to go.
Registeringform,i18n,ElementPlus, andk-markdownlooks well-organized.
55-59: Monaco container definition is coherent.
All references and state management foreditorare valid at a glance.
68-112: Monaco YAML handling is properly structured.
Error handling logic inonDidChangeModelContentis nice.
115-154: Refine logging and endpoint usage.
• Replace manual console logs with a centralized logger if logs are expected in production.
• Confirm that an empty string endpoint''is intentional forrequest.post().
[nitpick, verify]
156-156: ReactDOM usage is correct.
UsingcreateRootis the recommended approach for modern React apps.
159-159: Page export is straightforward.
No issues; the default export aligns with your page definition.
51-53:✅ Verification successful
Verify
UiContextavailability.
Ensure thatUiContextis properly declared and accessible here.
🏁 Script executed:
#!/bin/bash # Searching for UiContext definition to confirm it's globally accessible or imported. rg 'UiContext'Length of output: 20026
UiContext is declared and accessible globally.
The search confirms thatUiContextis defined inpackages/ui-default/api.tsand is consistently used across multiple files—including thepages/setting.page.tsxfile in question. No changes are needed here.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
packages/ui-next/index.ts (1)
46-46: 🛠️ Refactor suggestionReplace synchronous file reading with asynchronous version.
Using synchronous file reading can block the event loop and affect application performance.
Convert to async file reading:
- const html = fs.readFileSync(path.join(__dirname, 'index.html'), 'utf-8'); + const html = await fs.promises.readFile(path.join(__dirname, 'index.html'), 'utf-8');framework/framework/server.ts (3)
129-130: 🛠️ Refactor suggestionRemove unsafe declaration merging.
The empty interface
HandlerCommon<C>followed by a class with the same name creates an unsafe declaration merge, which can lead to TypeScript not properly checking property initializations.-export interface HandlerCommon<C> { } // eslint-disable-line @typescript-eslint/no-unused-vars +// Use a type alias instead if properties need to be added +// type HandlerCommonProps<C> = {} export class HandlerCommon<C> {🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 130-130: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
487-490: 🛠️ Refactor suggestionAddress type casting issues in event emissions.
Multiple
as anytype assertions with FIXME comments have been introduced for event emissions, which compromises type safety.Consider creating proper type definitions for the event system to maintain type safety:
// Define event types interface HandlerEvents { 'handler/create': [handler: Handler<CordisContext>, type: 'http' | 'ws']; 'handler/create/http': [handler: Handler<CordisContext>]; // Add other event types } // Then update the parallel/serial methods to use these typed events
662-686:⚠️ Potential issueFix condition that may evaluate incorrectly.
The
Checkerfunction's implementation of permission and privilege checking has been refactored, but the condition on line 683 might still evaluate incorrectly in some cases.The issue could be in how permissions are extracted from the arguments. Verify that the assignments to
permare correctly handling all possible input formats.packages/hydrooj/src/settings.ts (1)
41-53:⚠️ Potential issuePersistent risk of prototype pollution.
There is a partial safeguard against['__proto__', 'prototype', 'constructor']. Note that malicious actors can still exploit advanced paths to modify object prototypes. Refer to specialized libraries (e.g., safe-merge) or implement a deeper check to fully mitigate this.I can provide a more robust solution or a recommended library if you wish—would you like me to open an issue for that?
🧹 Nitpick comments (12)
packages/ui-next/index.ts (3)
32-34: Remove or explain the@ts-ignorecomment.Using
@ts-ignorebypasses TypeScript's type checking, which can hide potential issues. Either fix the underlying type issue or provide a detailed comment explaining why the ignore is necessary.If the plugin doesn't have proper typings, consider adding a more descriptive comment:
- // @ts-ignore + // @ts-ignore importMetaUrlPlugin lacks TypeScript definitions but is required for proper import.meta.url handling importMetaUrlPlugin,
23-23: Consider making allowedHosts configurable.The allowedHosts is currently hardcoded to ['beta.hydro.ac'], which might limit deployment flexibility.
Make this configurable through the Context or environment variables:
- allowedHosts: ['beta.hydro.ac'], + allowedHosts: ctx.config.allowedHosts || ['beta.hydro.ac'],
56-58: Improve error handling in disposal function.The error in the catch block is only logged to console, but no further action is taken.
Consider notifying the context of the error:
ctx.on('dispose', async () => { - await vite.close().catch((e) => console.error(e)); + await vite.close().catch((e) => { + console.error('Error closing Vite server:', e); + ctx.logger.error('Failed to close Vite server', { error: e }); + }); });packages/ui-default/pages/setting.page.tsx (4)
147-150: Internationalize notification messages.The success message is hardcoded in Chinese while the rest of the UI appears to use i18n.
Use i18n consistently for all user-facing messages:
- Notification.success('保存成功'); + Notification.success(i18n('Save successful')); - Notification.error('保存失败:', e.message); + Notification.error(`${i18n('Save failed')}: ${e.message}`);
89-89: Add error handling for Monaco editor registration.The
registerActionfunction is called without any error handling, which could lead to unhandled exceptions.Add try/catch to handle potential errors:
- registerAction(e, model, editorRef.current); + try { + registerAction(e, model, editorRef.current); + } catch (err) { + console.error('Failed to register editor actions:', err); + setError('Editor initialization error: ' + err.message); + }
69-102: Refactor editor initialization into a separate useEffect.The editor initialization effect is quite long and complex. Breaking it down would improve readability and maintainability.
Consider splitting the effect into smaller, more focused effects or extracting the Monaco initialization logic into a custom hook:
// Example of how this could be structured function useMonacoEditor(config, setValue, setError) { const [editor, setEditor] = React.useState(null); const [loading, setLoading] = React.useState(false); const initEditor = React.useCallback((element) => { if (!element || loading) return; setLoading(true); // Editor initialization logic... }, [config, loading, setValue, setError]); return { editor, initEditor }; } // Then in the component: const { editor, initEditor } = useMonacoEditor(config, setValue, setError); // ... React.useEffect(() => { if (editorRef.current) initEditor(editorRef.current); }, [editorRef.current, initEditor]);
115-115: Avoid unnecessary JSON serialization/deserialization.Using
JSON.parse(JSON.stringify(initial))for cloning is inefficient and can cause data loss for some JavaScript types.Use a more direct cloning approach appropriate for your data:
- const initialClone = JSON.parse(JSON.stringify(initial)); + const initialClone = yaml.load(yaml.dump(initial)); // For YAML dataOr consider using a dedicated deep clone utility:
- const initialClone = JSON.parse(JSON.stringify(initial)); + const initialClone = _.cloneDeep(initial); // Using lodashpackages/hydrooj/src/settings.ts (5)
14-23: Consider stricter type definitions for config fields.
DefiningsystemConfigasanymight mask potential issues. Consider introducing a typed interface or using a parameterized schema to increase type safety.
25-26: Redundantreturn await.
Usingreturn await this.loadConfig();inside an async function is slightly less efficient thanreturn this.loadConfig();. Consider removing the extraawaitif no additional logic is needed.- return await this.loadConfig(); + return this.loadConfig();
29-39: Graceful fallback for missing or malformed config.
Ifconfigis not found in the database or contains malformed YAML, the code logs an error but doesn’t initialize a default. Consider handling the undefined case more explicitly to ensure a valid in-memorysystemConfig.
65-70: Extra read after saving.
await this.loadConfig();forces a re-read from the database that might be redundant in some cases. Evaluate whether the in-memory state can be updated directly to avoid additional overhead.
77-83: Potential runtime errors for invalid paths.
If the path does not exist,root[p]might becomeundefinedand cause a crash on subsequent lookups. Consider short-circuiting when an intermediate property is missing.-def get(path: string) { - let root = this.systemConfig; - for (const p of path.split('.')) { - root = root[p]; - } - return root; +get(path: string) { + let root = this.systemConfig; + for (const p of path.split('.')) { + if (root == null) return undefined; + root = root[p]; + } + return root; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
framework/framework/server.ts(21 hunks)packages/hydrooj/src/settings.ts(1 hunks)packages/ui-default/pages/setting.page.tsx(1 hunks)packages/ui-next/index.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui-default/pages/setting.page.tsx
[error] 36-36: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
framework/framework/server.ts
[error] 129-129: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 130-130: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (9)
framework/framework/server.ts (5)
755-777: Well implemented mixin helper method.The
_applyMixinmethod is a significant improvement, properly handling:
- Property assignment to target prototypes
- Warning for existing properties
- Cleanup through the effect system
- Restoration of original property values when unmounting
This addresses previous issues with mixin implementations.
716-717: Improved type safety for Route handler.The
Routemethod now properly usestypeof Handler<C>instead ofanyfor theRouteHandlerparameter, which improves type safety.This properly addresses a previous review comment about improving the type signature.
721-722: Improved type safety for Connection handler.The
Connectionmethod now properly usestypeof ConnectionHandler<C>instead ofanyfor theRouteHandlerparameter, which improves type safety.This properly addresses a previous review comment about adding proper typing.
794-795: Fixed wsHandlerMixin implementation.The
wsHandlerMixinmethod now correctly applies the provided mixin to theConnectionHandlerprototype through the new_applyMixinhelper.This addresses a previous issue where the method would warn about existing properties but wouldn't actually assign the new ones.
808-814: Proper module augmentation for Context.The module declaration for
@cordisjs/corenow correctly types the mixins added to theContextinterface using the genericthistype, ensuring type safety when accessing server methods.packages/hydrooj/src/settings.ts (4)
3-3: Use explicit type imports for clarity.
BringingContextandServicewithin the same import statement is fine, but consider explicitly importing types (e.g.,type Context) if you prefer to highlight them as types or keep them separate for clarity.
8-10: Context augmentation looks good.
Augmenting theContextinterface with theconfigproperty is a clean way to integrate the config service.
55-63: Schema validation approach is solid.
Validating a temporary copy of the config helps prevent partial corruption. This layered check (withSchema.intersect) is a good practice for safety.
72-75: Asynchronous set calls.
setConfigitself is asynchronous, but callers or the Proxy layer might not wait on the result. If a save fails, the error is lost. Consider exposing a promise or using a callback to handle errors at call sites.
| console.log('update.form', newDump); | ||
| setStringConfig(newDump); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log statements from production code.
Debug statements should be removed before deploying to production.
Remove console logs in the update functions:
const updateFromForm = React.useCallback((v) => {
const newDump = yaml.dump(v);
if (newDump === stringConfig) return;
- console.log('update.form', newDump);
setStringConfig(newDump);
setValue(v);
}, [stringConfig]);
const updateFromMonaco = React.useCallback((v) => {
if (v === stringConfig) return;
- console.log('update.monaco', v);
setStringConfig(v);
setValue(yaml.load(v));
}, [stringConfig]);Also applies to: 130-131
packages/hydrooj/src/settings.ts
Outdated
| requestConfig<T, S>(s: Schema<T, S>): ReturnType<Schema<T, S>> { | ||
| this.ctx.effect(() => { | ||
| this.settings.push(s); | ||
| return () => { | ||
| this.settings = this.settings.filter((v) => v !== s); | ||
| }; | ||
| }); | ||
| let curValue = s(this.systemConfig); | ||
| this.ctx.on('system/setting', () => { | ||
| try { | ||
| curValue = s(this.systemConfig); | ||
| } catch (e) { | ||
| logger.warn('Cannot read config: ', e.message); | ||
| curValue = null; | ||
| } | ||
| }); | ||
| const that = this; | ||
| const getAccess = (path: (string | symbol)[]) => { | ||
| let currentValue = curValue; | ||
| for (const p of path) { | ||
| currentValue = currentValue[p]; | ||
| } | ||
| if (typeof currentValue !== 'object') return curValue; | ||
| return new Proxy(currentValue, { | ||
| get(self, key: string) { | ||
| return getAccess(path.concat(key)); | ||
| }, | ||
| set(self, p: string | symbol, newValue: any) { | ||
| that.setConfig(path.concat(p).join(','), newValue); | ||
| return true; | ||
| }, | ||
| }); | ||
| }; | ||
| return getAccess([]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fire-and-forget in Proxy setter may swallow DB write errors.
The set trap does not await that.setConfig(...), which can result in silent failures if the DB call rejects. Consider making the proxy setter async or otherwise surfacing the error handling logic.
- set(self, p: string | symbol, newValue: any) {
- that.setConfig(path.concat(p).join(','), newValue);
- return true;
- },
+ async set(self, p: string | symbol, newValue: any) {
+ try {
+ await that.setConfig(path.concat(p).join(','), newValue);
+ } catch (err) {
+ logger.error('Failed to set config property:', err.message);
+ return false;
+ }
+ return true;
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| requestConfig<T, S>(s: Schema<T, S>): ReturnType<Schema<T, S>> { | |
| this.ctx.effect(() => { | |
| this.settings.push(s); | |
| return () => { | |
| this.settings = this.settings.filter((v) => v !== s); | |
| }; | |
| }); | |
| let curValue = s(this.systemConfig); | |
| this.ctx.on('system/setting', () => { | |
| try { | |
| curValue = s(this.systemConfig); | |
| } catch (e) { | |
| logger.warn('Cannot read config: ', e.message); | |
| curValue = null; | |
| } | |
| }); | |
| const that = this; | |
| const getAccess = (path: (string | symbol)[]) => { | |
| let currentValue = curValue; | |
| for (const p of path) { | |
| currentValue = currentValue[p]; | |
| } | |
| if (typeof currentValue !== 'object') return curValue; | |
| return new Proxy(currentValue, { | |
| get(self, key: string) { | |
| return getAccess(path.concat(key)); | |
| }, | |
| set(self, p: string | symbol, newValue: any) { | |
| that.setConfig(path.concat(p).join(','), newValue); | |
| return true; | |
| }, | |
| }); | |
| }; | |
| return getAccess([]); | |
| } | |
| } | |
| requestConfig<T, S>(s: Schema<T, S>): ReturnType<Schema<T, S>> { | |
| this.ctx.effect(() => { | |
| this.settings.push(s); | |
| return () => { | |
| this.settings = this.settings.filter((v) => v !== s); | |
| }; | |
| }); | |
| let curValue = s(this.systemConfig); | |
| this.ctx.on('system/setting', () => { | |
| try { | |
| curValue = s(this.systemConfig); | |
| } catch (e) { | |
| logger.warn('Cannot read config: ', e.message); | |
| curValue = null; | |
| } | |
| }); | |
| const that = this; | |
| const getAccess = (path: (string | symbol)[]) => { | |
| let currentValue = curValue; | |
| for (const p of path) { | |
| currentValue = currentValue[p]; | |
| } | |
| if (typeof currentValue !== 'object') return curValue; | |
| return new Proxy(currentValue, { | |
| get(self, key: string) { | |
| return getAccess(path.concat(key)); | |
| }, | |
| + async set(self, p: string | symbol, newValue: any) { | |
| + try { | |
| + await that.setConfig(path.concat(p).join(','), newValue); | |
| + } catch (err) { | |
| + logger.error('Failed to set config property:', err.message); | |
| + return false; | |
| + } | |
| + return true; | |
| + }, | |
| - set(self, p: string | symbol, newValue: any) { | |
| - that.setConfig(path.concat(p).join(','), newValue); | |
| - return true; | |
| - }, | |
| }); | |
| }; | |
| return getAccess([]); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/hydrooj/src/handler/manage.ts (1)
198-198:⚠️ Potential issueRemove the empty object pattern.
Static analysis flags
{ }as an unexpected empty pattern. Consider removing it as it doesn't serve any purpose.- async post({ }, value: string) { + async post(value: string) {🧰 Tools
🪛 Biome (1.9.4)
[error] 198-198: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/hydrooj/src/entry/cli.ts(3 hunks)packages/hydrooj/src/entry/worker.ts(4 hunks)packages/hydrooj/src/handler/manage.ts(2 hunks)packages/ui-default/index.ts(5 hunks)packages/ui-next/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui-next/index.ts
- packages/hydrooj/src/entry/worker.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/hydrooj/src/handler/manage.ts
[error] 198-198: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
packages/ui-default/index.ts
[error] 56-56: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (12)
packages/hydrooj/src/entry/cli.ts (5)
9-11: Improved service architecture with plugin systemThe shift from direct service initialization to a plugin-based approach using
MongoServiceandConfigServiceenhances modularity and separation of concerns. This architectural change makes the system more testable and easier to extend.Also applies to: 114-115
117-121: Good use of dependency injection patternThe Promise-based approach for ensuring dependencies are available before proceeding is a robust pattern for asynchronous initialization. This prevents potential race conditions where code might execute before required services are fully initialized.
122-122: Simplified plugin reloadingRemoving the redundant
ctxparameter from thereloadPlugincall simplifies the API. The loader already has access to the context, so this change reduces unnecessary parameter passing.
133-137: Consistent dependency resolution patternUsing the same Promise-based injection pattern for the server dependency maintains consistency with the earlier pattern. This systematic approach to dependency management makes the codebase more predictable and easier to understand.
141-141: Consistent plugin loading approachThe script loading follows the same simplified pattern as line 122, maintaining consistency throughout the codebase in how plugins are loaded and initialized.
packages/hydrooj/src/handler/manage.ts (3)
186-192: Good refactoring to context-based configurationThe changes here effectively move away from static imports to context-based configuration management. This improves modularity and testability by centralizing configuration handling through the context object.
205-205: Consistent use of context-based config operationsUsing
this.ctx.config.saveConfig(config)aligns well with the approach used in the rest of the file. This improves consistency in how configuration data is managed.
324-324: Good practice: explicit dependency injectionAdding the
injectconstant explicitly declares the module's dependencies, which improves code maintainability and makes dependencies clear to other developers.packages/ui-default/index.ts (4)
4-5: Proper import cleanupRemoving the unused
SystemSettingsimport keeps the codebase clean by eliminating unnecessary dependencies.
25-25: Improved type safety with explicit type annotationAdding explicit type annotation to the
sectionsarray improves type safety and code readability by clearly documenting the expected structure.
77-77: Good refactoring to context-based configurationUsing
this.ctx.config.settingsinstead of the staticSystemSettingsis consistent with the architectural changes being made throughout the codebase. This improves modularity and testability.
119-119: Improved type safety with explicit type annotationAdding the explicit
any[]type annotation to theresvariable improves code readability and helps catch potential type-related issues earlier.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ui-default/index.ts (1)
57-57:⚠️ Potential issueRemove the empty object pattern.
The empty object pattern
{ }in the parameter list is flagged by static analysis as problematic. Since you're not using any properties from this object, you should remove it.- async get({ }, legacy = false, nohint = false) { + async get(legacy = false, nohint = false) {🧰 Tools
🪛 Biome (1.9.4)
[error] 57-57: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
🧹 Nitpick comments (3)
packages/hydrooj/src/service/server.ts (2)
200-224: Consider reviewing sensitive logging.Error handling is robust, but it logs user IDs, names, and error details. In production, ensure that sensitive fields (e.g., user credentials) aren’t exposed.
225-233: WebSocket error handling aligns with HTTP approach.You may likewise review log content to avoid potential user privacy issues in production.
packages/ui-default/backendlib/template.ts (1)
44-57: Optional chaining recommendation.Instead of
(obj || {})._original, consider using optional chaining for improved readability, e.g.obj?._original.- if ((obj || {})._original) obj = obj._original; + if (obj?._original) obj = obj._original;🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/hydrooj/src/entry/common.ts(2 hunks)packages/hydrooj/src/entry/worker.ts(3 hunks)packages/hydrooj/src/init.ts(1 hunks)packages/hydrooj/src/interface.ts(0 hunks)packages/hydrooj/src/service/bus.ts(5 hunks)packages/hydrooj/src/service/server.ts(6 hunks)packages/ui-default/backendlib/template.ts(3 hunks)packages/ui-default/index.ts(6 hunks)
💤 Files with no reviewable changes (1)
- packages/hydrooj/src/interface.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hydrooj/src/service/bus.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui-default/backendlib/template.ts
[error] 46-46: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 121-121: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
packages/ui-default/index.ts
[error] 57-57: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (28)
packages/hydrooj/src/init.ts (1)
23-23: Global Hydro UI structure simplified.The
uiproperty is now initialized as an empty object instead of having a nestedtemplateproperty. This change aligns with the broader architectural changes in the PR where template management has been moved to a dedicated service (as seen in the introduction ofTemplateServicein packages/ui-default/index.ts).packages/hydrooj/src/entry/worker.ts (6)
10-11: Improved service architecture with explicit imports.The imports have been updated to use named imports for specific services (
MongoServiceandConfigService), which improves code clarity and reflects the move toward a more service-oriented architecture.
26-30: Enhanced database initialization with asynchronous URL retrieval.The code now uses an asynchronous approach with
MongoService.getUrl()instead of synchronously loading the configuration. This improves the application's startup flow by properly handling asynchronous operations.
33-36: Improved service initialization with explicit configuration.The code now properly awaits the locale loading and explicitly configures the MongoDB and Config services using the plugin architecture. This creates a more maintainable and modular approach to service initialization.
38-42: Added proper dependency injection with Promise-based resolution.This code introduces a pattern for ensuring dependencies are properly loaded before proceeding. The Promise-based approach ensures that the context with the required services ('loader', 'config') is available before continuing execution.
43-48: Improved plugin loading with Promise-based approach.The code now loads plugins concurrently using
Promise.all, which can improve startup performance. The updatedreloadPluginmethod calls also reflect the new plugin loading mechanism that no longer requires passing the context explicitly.
49-53: Added server dependency injection with Promise-based resolution.Similar to the earlier dependency injection, this ensures the 'server' component is properly loaded before proceeding, maintaining a correct initialization sequence.
packages/hydrooj/src/entry/common.ts (2)
34-34: Updated reloadPlugin method call signature.The method call has been updated to remove the context parameter and change the empty object to an empty string. This simplifies the API and likely aligns with changes in the plugin loader implementation.
61-61: Consistent update to reloadPlugin method signature.Similar to the previous change, this maintains consistency with the updated plugin loading mechanism across the codebase.
packages/ui-default/index.ts (5)
4-5: Updated imports to align with service architecture.The code now imports
TemplateServicefrom a dedicated module and removesSystemSettingswhich is no longer needed. This reflects the architectural shift towards service-based components.Also applies to: 8-8
26-26: Improved type safety with explicit type annotation.Adding a specific type for the
sectionsarray improves code readability and type safety. This ensures the array elements have the expected structure withid,title, andcontentproperties.
78-78: Updated schema generation to use context configuration.The code now uses
this.ctx.config.settingsinstead of the previously importedSystemSettingsto generate the schema. This change follows the service-oriented approach where configuration is accessed through the context rather than direct imports.
120-120: Improved type safety with explicit type annotation for array.Adding an explicit type annotation for the
resarray improves code readability and type safety.
168-168: Added TemplateService as a plugin.The code now registers the
TemplateServiceas a plugin, which aligns with the architectural changes in the PR. This centralizes template handling in a dedicated service.packages/hydrooj/src/service/server.ts (9)
7-9: Imports look good.No issues found with the introduction of these additional imports.
31-31: Module augmentation approved.Using a generic interface in the module augmentation is a clean approach and looks correct.
74-74: Class declaration appears fine.Ensure that
domainis initialized or marked definite (domain!: DomainDoc;) if strict property initialization is enabled.
77-77: Consistent pattern for connection handler.Similar to the previous class, verify that
domainis properly assigned at runtime if strict initialization is required.
82-82: Plugin addition looks straightforward.The
WebServiceplugin configuration is clear and aligns with the retrieved settings.
149-149: Dynamic URL logic seems valid.Calling the router method with custom arguments is properly handled.
240-241: Context extension.Attaching
domainto the extended context is straightforward and consistent.
242-252: HTTP handler creation logic is sound.Throwing
h.context.pendingErrorearly helps fail fast and prevents partial operations.
253-255: WS handler pending error check.Similarly, the WebSocket flow checks for pending errors at creation. This is good for consistency.
packages/ui-default/backendlib/template.ts (5)
5-5: Expanded imports look fine.Introducing
Serviceand typed references appear consistent.
25-34: File gathering logic is efficient.The recursive approach for reading files is clear, and the path normalization is beneficial for cross-platform compatibility.
36-42: File location utility is straightforward.The loop checks existence and returns the first found result. No issues found.
59-160: Rich set of template filters.This block adds valuable filters for markdown, base64, and more. The approach looks well-structured.
🧰 Tools
🪛 Biome (1.9.4)
[error] 121-121: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
161-275: TemplateService design is well-structured.The service-oriented approach to register templates and render logic is maintainable. Initialization steps are clearly defined.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/hydrooj/src/settings.ts (3)
17-20: Consider stricter typing ofsystemConfig.Although using
anyis acceptable for rapid prototyping, a more specific type can improve maintainability and reduce potential runtime errors.
25-27: Minor await style nitpick.
return await this.loadConfig();could be simplified toreturn this.loadConfig();to avoid the extra promise wrapping.async [Service.setup]() { - return await this.loadConfig(); + return this.loadConfig(); }
65-70: Consider error handling during save.Though the schema check and subsequent YAML dump are fine, consider try/catch around the database call to handle DB write failures more robustly.
packages/hydrooj/src/handler/misc.ts (2)
111-113: Avoid empty object destructuring.Using an empty destructuring pattern is flagged by linters. Consider removing the parameter or renaming it to
_domainIdif you need to keep the signature.-async get({ }, target: string, filename = '', expire: number, secret: string) { +async get(target: string, filename = '', expire: number, secret: string) { ... }🧰 Tools
🪛 Biome (1.9.4)
[error] 111-111: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
125-125: Remove the unused destructuring.Similarly, replace
{ }with a placeholder variable (e.g.,_,_domainId) or remove it altogether.🧰 Tools
🪛 Biome (1.9.4)
[error] 125-125: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/hydrooj/src/handler/misc.ts(3 hunks)packages/hydrooj/src/service/storage.ts(8 hunks)packages/hydrooj/src/settings.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/hydrooj/src/handler/misc.ts
[error] 111-111: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 125-125: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (27)
packages/hydrooj/src/settings.ts (11)
3-3: Looks good.The new import for
ContextandServicecorrectly aligns with the subsequent usage in this file.
8-10: Module augmentation is appropriate.Extending the
Contextinterface with aconfigproperty helps centralize configuration logic.
14-16: Logical service injection.Defining
static inject = ['db']ensures theConfigServiceclass accesses the database from the context.
21-23: No issues with the constructor.The constructor properly initializes the service with a designated name.
29-39: Graceful config loading and event emission.Excellent use of a try/catch block for YAML parsing, with informative logging and event emission.
41-53: Prototype pollution check still flagged.Your defense against
__proto__,prototype,constructoris partially mitigating the risk. However, as previously noted, a specialized library or more robust whitelist-based approach may be required to eliminate all attack vectors.
55-63: Schema intersection validation looks solid.Validating the patched config with all registered schemas helps maintain consistency and prevent invalid updates.
72-75: Single-key update is straightforward.Delegating to
saveConfigcentralizes logic and maintains schema safety.
77-96: Dynamic proxy approach is workable.Allowing components to request schema-based config with dynamic updates is a neat design. However, beware that the
settrap does not awaitthat.setConfig(...), which can silently swallow errors.
98-114: Proxy-based nested property access is well-structured.Similar to the previous note, the
setoperation would benefit from async handling or error feedback.
116-116: No concerns here.The class closing brace is straightforward.
packages/hydrooj/src/service/storage.ts (15)
15-19: New imports align with the added functionality.Imports for
koa-proxies,nanoid, andschemasteryare properly introduced, along with the existing MIME utilities.
63-96: Well-defined storage schema.Defining defaults and using
Schema.intersectwith local or S3 options streamlines reusability and ensures type safety.
98-98: Exporting the unified config enhances modularity.This straightforward export fosters consistency across the codebase.
110-111: Remote storage constructor injection.Passing a typed config object via constructor is a clean way to ensure the service remains flexible and testable.
265-267:isLinkValidplaceholder may need an implementation.Always returning
falseindicates no validation. Confirm whether remote storage link validation is intentionally disabled or if future logic is planned.
311-312: Local storage constructor injection.Similarly, using
FileSettingensures clarity for local storage configurations.
315-316: Local storage initialization is concise.Correctly logging and creating directories before use is a good practice.
319-320: Alternative endpoint configuration.Good usage of
parseAlternativeEndpointUrlfor user/judge endpoints.
367-367: Signed link generation.MD5 hashing the secret is a standard approach to create a short, unique signature for the download link.
372-376: Link validation logic.Verifying with a simple path-based split is effective. Ensure you handle unexpected additional segments if needed.
392-393: Globalservicereference introduced.Minimal overhead. Just ensure concurrency safety if usage is multi-threaded or multi-process.
394-399: Conditional construction of storage service.Dynamically picking a remote or local service is a good pattern.
401-427: Proxy middleware injection.Neatly integrating
koa-proxiesto handle/fsroutes. This approach is flexible and straightforward.
429-433: Context extension for storage.Adding a
storageproperty onContextis consistent with the new injection pattern.
435-441: Deprecated proxy.Encouraging
ctx.storageusage is cleaner. Removing the proxy eventually can reduce confusion.packages/hydrooj/src/handler/misc.ts (1)
18-18: ImportingsortFilesis appropriate.This utility usage is consistent with existing file listing logic.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/ui-default/pages/setting.page.tsx (1)
25-33:⚠️ Potential issueSecurity risk:
dangerouslySetInnerHTMLwithout sanitizationWhile the comment states the Markdown comes from trusted backend code, it's still a security best practice to sanitize HTML before injecting it into the DOM to prevent XSS attacks.
Consider adding DOMPurify for sanitization:
import React from 'react'; import ReactDOM from 'react-dom/client'; +import DOMPurify from 'dompurify'; import Schema from 'schemastery';// Markdown snippet come from trusted backend code, no need to sanitize here - return <div dangerouslySetInnerHTML={{ __html: rendered }} />; + // Even with trusted backend code, sanitizing prevents accidental XSS + return <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(rendered) }} />;🧰 Tools
🪛 Biome (1.9.4)
[error] 33-33: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🧹 Nitpick comments (5)
packages/utils/lib/utils.ts (1)
32-33: Enhanced type safety with explicit Logger type definitionThe exported Logger type now combines Reggol with a constructor signature, providing better type safety while maintaining backward compatibility through the type casting (
as any as Logger).However, the use of
anyin the type casting might bypass TypeScript's type checking. Consider if there's a more type-safe way to accomplish this in the future.-export const Logger = createLogger as any as Logger; +// Consider a more type-safe approach in the future +export const Logger = createLogger as unknown as Logger;packages/ui-default/pages/setting.page.tsx (4)
41-44: Add interface for MonacoContainer propsConsider adding a proper TypeScript interface for the component props to improve code maintainability and type safety.
+interface MonacoContainerProps { + config: string; + setValue: (value: string) => void; + setError: (error: string) => void; +} + -function MonacoContainer({ config, setValue, setError }) { +function MonacoContainer({ config, setValue, setError }: MonacoContainerProps) {
44-44: Improve type safety for editor stateThe editor state is using
anytype. Consider using a more specific type for better type safety.- const [editor, setEditor] = React.useState<any>(null); + const [editor, setEditor] = React.useState<monaco.editor.IStandaloneCodeEditor | null>(null);
69-69: Clarify fontLigatures settingThe empty string for fontLigatures might be unclear. If you intend to disable ligatures, consider using a more explicit value.
- fontLigatures: '', + fontLigatures: false, // Explicitly disable font ligatures
140-140: Add error handling for DOM element accessThe code assumes that an element with ID 'app' exists in the DOM. Add error handling to gracefully handle cases where the element is not found.
- ReactDOM.createRoot(document.getElementById('app')!).render(<App />); + const appElement = document.getElementById('app'); + if (appElement) { + ReactDOM.createRoot(appElement).render(<App />); + } else { + console.error('Element with ID "app" not found'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
build/prepare.js(3 hunks)framework/eslint-config/package.json(1 hunks)package.json(3 hunks)packages/hydrooj/package.json(3 hunks)packages/hydrooj/src/handler/record.ts(0 hunks)packages/hydrooj/src/interface.ts(2 hunks)packages/hydrooj/src/service/storage.ts(8 hunks)packages/ui-default/package.json(4 hunks)packages/ui-default/pages/setting.page.tsx(1 hunks)packages/ui-default/vue.d.ts(1 hunks)packages/ui-next/package.json(1 hunks)packages/utils/lib/utils.ts(1 hunks)packages/utils/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/hydrooj/src/handler/record.ts
✅ Files skipped from review due to trivial changes (1)
- packages/ui-default/vue.d.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/ui-next/package.json
- packages/hydrooj/src/service/storage.ts
- package.json
- packages/hydrooj/package.json
- build/prepare.js
- packages/ui-default/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/ui-default/pages/setting.page.tsx
[error] 33-33: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (12)
packages/hydrooj/src/interface.ts (2)
2-2: Added import to support updated type definition.The import of
ParsedAuthenticatorDatafrom@simplewebauthn/server/helpersis necessary to support the type change forauthenticatorExtensionResultsproperty on line 83.
83-83: Type enhancement for better type safety.Changing the type of
authenticatorExtensionResultsfromAuthenticationExtensionsAuthenticatorOutputstoParsedAuthenticatorData['extensionsData']improves type safety by using a more specific and appropriate type definition from the SimpleWebAuthn library.This change is part of the broader type safety improvements mentioned in the PR summary and aligns with the removal of the
HydroServiceinterface (not visible in this diff but mentioned in the AI summary).framework/eslint-config/package.json (6)
7-7: Updated Dependency Version for @stylistic/eslint-plugin
The version change to "^4.2.0" is a significant upgrade from the previous version. Please ensure that any breaking changes or updated rules in this plugin are compatible with your ESLint configuration.
8-9: Upgraded TypeScript ESLint Dependencies
Both@typescript-eslint/eslint-pluginand@typescript-eslint/parserhave been updated to "^8.26.0". Verify that these upgrades integrate well with your project's TypeScript configuration and that there are no regressions in linting behavior.
11-11: Updated Dependency Version for eslint-plugin-de-morgan
The dependency has been bumped to "^1.2.0", which may include bug fixes or minor improvements. Please run your linting tests to ensure that the updated rules do not introduce unexpected errors.
16-16: Updated Dependency Version for eslint-plugin-react-hooks
The version update to "^5.2.0" should bring enhancements and/or fixes. Confirm that this change does not affect existing hook rules or reporting in your codebase.
22-22: Peer Dependency Verification for ESLint
The peer dependency"eslint": ">= 8"continues to ensure that the consuming projects have a compatible ESLint version. Please double-check that this constraint satisfies all updated plugin requirements.
23-24: Addition of Optional Dependency for TypeScript
The new"optionalDependencies"section including"typescript": "^5.8.2"aligns with the broader type-safety improvements and provides flexibility for projects opting into TS usage. Verify that projects leveraging this configuration benefit from the enhanced type support without forcing an upgrade for all users.packages/utils/package.json (1)
17-17:❓ Verification inconclusive
Major version upgrade of reggol dependency
The update from reggol 1.7.1 to 2.0.3 represents a major version change which typically includes breaking changes according to semantic versioning. This appears to be related to the significant logger implementation changes in utils.ts.
🏁 Script executed:
#!/bin/bash # Check for reggol v2.0.3 changelog or breaking changes curl -s https://registry.npmjs.org/reggol | jq '.versions["2.0.3"]?.description, .versions["2.0.0"]?.description' # Find references to reggol in the codebase to verify all usage has been updated rg -A 2 -B 2 "reggol" --type ts --type jsLength of output: 589
Attention: Verify reggol Upgrade Impact on Logger Usage
The dependency upgrade from reggol 1.7.1 to 2.0.3 is a major version bump—which typically signals breaking changes. Our verification shows that both version 2.0.3 and 2.0.0 carry the description "Logger for professionals" and that the updated usage in
packages/utils/lib/utils.ts(importing the logger asReggol) appears to reflect the intended changes. However, the changelog details aren’t explicit about any breaking changes, so please confirm that the logger’s behavior aligns with the new reggol version.
- Action Item: Double-check that all logger usages (e.g., in
packages/utils/lib/utils.ts) conform to the updated reggol API.- Recommendation: Consult the official reggol documentation or changelog for details on breaking changes to ensure full compatibility.
packages/utils/lib/utils.ts (3)
10-10: Updated import for reggol libraryThe import has been modified to accommodate the new version (2.x) of the reggol library, importing specific components (Exporter, Factory, Logger as Reggol) that support the new Factory pattern implementation.
15-26: Improved logger configuration using Factory patternThe code now uses the Factory pattern from reggol 2.x to create and configure loggers. This approach is more modular and configurable than the previous direct manipulation of Logger properties.
The console exporter configuration includes helpful formatting settings like:
- Showing timestamps in "dd hh:mm:ss" format
- Right-aligned labels with specific width and margin
- Timestamp initialization
28-30: Clean abstraction with createLogger functionThe new createLogger function encapsulates the logger creation logic, making it easier to maintain and potentially extend with additional functionality in the future.
| Notification.success('保存成功'); | ||
| }).catch((e) => { | ||
| Notification.error('保存失败:', e.message); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use i18n for notification messages
The success message is hardcoded in Chinese while the button text uses i18n. For consistency and internationalization support, use i18n for all user-facing messages.
- Notification.success('保存成功');
+ Notification.success(i18n('Save successful'));
- Notification.error('保存失败:', e.message);
+ Notification.error(`${i18n('Save failed')}: ${e.message}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Notification.success('保存成功'); | |
| }).catch((e) => { | |
| Notification.error('保存失败:', e.message); | |
| }); | |
| Notification.success(i18n('Save successful')); | |
| }).catch((e) => { | |
| Notification.error(`${i18n('Save failed')}: ${e.message}`); | |
| }); |
| React.useEffect(() => { | ||
| if (!editorRef.current || loading) return; | ||
| setLoading(true); | ||
| const model = monaco.editor.createModel(config, 'yaml', monaco.Uri.parse('hydro://system/setting.yaml')); | ||
| const e = monaco.editor.create(editorRef.current, { | ||
| theme: 'vs-light', | ||
| lineNumbers: 'off', | ||
| glyphMargin: true, | ||
| lightbulb: { enabled: monaco.editor.ShowLightbulbIconMode.On }, | ||
| model, | ||
| minimap: { enabled: false }, | ||
| hideCursorInOverviewRuler: true, | ||
| overviewRulerLanes: 0, | ||
| overviewRulerBorder: false, | ||
| fontFamily: UserContext.codeFontFamily, | ||
| fontLigatures: '', | ||
| unicodeHighlight: { | ||
| ambiguousCharacters: true, | ||
| }, | ||
| }); | ||
| registerAction(e, model, editorRef.current); | ||
| e.onDidChangeModelContent(() => { | ||
| const val = e.getValue({ lineEnding: '\n', preserveBOM: false }); | ||
| try { | ||
| const loaded = yaml.load(val); | ||
| schema(loaded); | ||
| setValue(val); | ||
| setError(''); | ||
| } catch (err) { | ||
| setError(err.message); | ||
| } | ||
| }); | ||
| setEditor(e); | ||
| }, [editorRef.current]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Properly include dependencies in useEffect dependencies array
The useEffect hook for editor initialization doesn't include all dependencies in its dependency array. This may lead to stale closures.
React.useEffect(() => {
if (!editorRef.current || loading) return;
setLoading(true);
const model = monaco.editor.createModel(config, 'yaml', monaco.Uri.parse('hydro://system/setting.yaml'));
// ...rest of the code...
setEditor(e);
- }, [editorRef.current]);
+ }, [editorRef.current, loading, config, setValue, setError]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| React.useEffect(() => { | |
| if (!editorRef.current || loading) return; | |
| setLoading(true); | |
| const model = monaco.editor.createModel(config, 'yaml', monaco.Uri.parse('hydro://system/setting.yaml')); | |
| const e = monaco.editor.create(editorRef.current, { | |
| theme: 'vs-light', | |
| lineNumbers: 'off', | |
| glyphMargin: true, | |
| lightbulb: { enabled: monaco.editor.ShowLightbulbIconMode.On }, | |
| model, | |
| minimap: { enabled: false }, | |
| hideCursorInOverviewRuler: true, | |
| overviewRulerLanes: 0, | |
| overviewRulerBorder: false, | |
| fontFamily: UserContext.codeFontFamily, | |
| fontLigatures: '', | |
| unicodeHighlight: { | |
| ambiguousCharacters: true, | |
| }, | |
| }); | |
| registerAction(e, model, editorRef.current); | |
| e.onDidChangeModelContent(() => { | |
| const val = e.getValue({ lineEnding: '\n', preserveBOM: false }); | |
| try { | |
| const loaded = yaml.load(val); | |
| schema(loaded); | |
| setValue(val); | |
| setError(''); | |
| } catch (err) { | |
| setError(err.message); | |
| } | |
| }); | |
| setEditor(e); | |
| }, [editorRef.current]); | |
| React.useEffect(() => { | |
| if (!editorRef.current || loading) return; | |
| setLoading(true); | |
| const model = monaco.editor.createModel(config, 'yaml', monaco.Uri.parse('hydro://system/setting.yaml')); | |
| const e = monaco.editor.create(editorRef.current, { | |
| theme: 'vs-light', | |
| lineNumbers: 'off', | |
| glyphMargin: true, | |
| lightbulb: { enabled: monaco.editor.ShowLightbulbIconMode.On }, | |
| model, | |
| minimap: { enabled: false }, | |
| hideCursorInOverviewRuler: true, | |
| overviewRulerLanes: 0, | |
| overviewRulerBorder: false, | |
| fontFamily: UserContext.codeFontFamily, | |
| fontLigatures: '', | |
| unicodeHighlight: { | |
| ambiguousCharacters: true, | |
| }, | |
| }); | |
| registerAction(e, model, editorRef.current); | |
| e.onDidChangeModelContent(() => { | |
| const val = e.getValue({ lineEnding: '\n', preserveBOM: false }); | |
| try { | |
| const loaded = yaml.load(val); | |
| schema(loaded); | |
| setValue(val); | |
| setError(''); | |
| } catch (err) { | |
| setError(err.message); | |
| } | |
| }); | |
| setEditor(e); | |
| }, [editorRef.current, loading, config, setValue, setError]); |
Summary by CodeRabbit
Chores
Refactor
ConfigServiceclass.Bug Fixes
BigInt.New Features
ConfigServiceclass for better configuration management.Appcomponent for the React application.