Skip to content

Feat/config system integration#190

Merged
GeWuYou merged 5 commits into
mainfrom
feat/config-system-integration
Apr 6, 2026
Merged

Feat/config system integration#190
GeWuYou merged 5 commits into
mainfrom
feat/config-system-integration

Conversation

@GeWuYou

@GeWuYou GeWuYou commented Apr 6, 2026

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • New Features

    • Centralized config bootstrap exposing a registry and safer hot-reload controls.
    • Runtime and metadata support for exclusive numeric bounds, string regex patterns, and array min/max item constraints.
  • Tests

    • New and expanded tests covering bootstrap lifecycle, hot-reload, loader validation, and the new constraint behaviors.
  • Docs & Tools

    • Updated docs, tooling hints, localization, and generated snapshots to document and surface the new constraints.
  • CI

    • Added Node-based steps to run config-tooling tests.

GeWuYou added 2 commits April 6, 2026 18:41
- 添加 ArchitectureConfigIntegrationTests 验证架构初始化流程中配置加载
- 实现 GameConfigBootstrap 收敛配置注册、加载与热重载生命周期管理
- 提供 GameConfigBootstrapOptions 配置启动约定选项对象
- 添加 GameConfigBootstrapTests 验证启动帮助器功能完整性
- 更新中文文档详述配置系统接入模板和最佳实践
- 提供 Architecture 推荐接入模板简化框架集成步骤
- 实现热重载支持和错误诊断机制提升开发体验
- 新增游戏内容配置系统完整文档,涵盖 YAML 配置、JSON Schema 结构、目录组织
- 实现配置系统的运行时查询、类型生成、VS Code 插件集成等功能说明
- 添加 Schema 示例、YAML 示例和推荐接入模板
- 提供运行时读取、热重载、批处理编辑等功能的使用指南
- 实现配置校验行为、跨表引用、诊断对象等核心功能文档
- 集成开发期工具支持,包括表单编辑、批量更新、注释渲染等能力
- 添加架构接入模板和生产部署相关建议

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @GeWuYou, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented Apr 6, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Adds a centralized GameConfigBootstrap for initialization/hot-reload/disposal, extends JSON Schema support (exclusiveMinimum/exclusiveMaximum, pattern, minItems/maxItems) across validator, generator docs, tooling, UI, tests, and updates integration/tests to use the bootstrap and IConfigRegistry.

Changes

Cohort / File(s) Summary
Bootstrap Infrastructure
GFramework.Game/Config/GameConfigBootstrap.cs, GFramework.Game/Config/GameConfigBootstrapOptions.cs
Add GameConfigBootstrap (lifecycle: InitializeAsync, StartHotReload, StopHotReload, Dispose) and options class (RootPath, ConfigureLoader, Registry, EnableHotReload, HotReloadOptions).
Architecture Integration Tests
GFramework.Game.Tests/Config/ArchitectureConfigIntegrationTests.cs
Swap direct Yaml loader usage for GameConfigBootstrap; ConsumerArchitecture now exposes IConfigRegistry via bootstrap and disposes bootstrap in DestroyAsync; XML docs updated.
Bootstrap Tests
GFramework.Game.Tests/Config/GameConfigBootstrapTests.cs
New NUnit tests exercising bootstrap init, registry loading, hot-reload behaviour, constructor validation, concurrent-init rejection, failure/rollback scenarios, and helpers for file/event waits.
Schema Validator & Runtime
GFramework.Game/Config/YamlConfigSchemaValidator.cs
Extend schema parsing and runtime validation to support exclusiveMinimum/exclusiveMaximum, pattern (compiled Regex, culture-invariant), and array minItems/maxItems; add YamlConfigArrayConstraints.
Config Loader Tests
GFramework.Game.Tests/Config/YamlConfigLoaderTests.cs
Add tests for exclusive numeric bounds, string pattern mismatch/backreference acceptance, and array min/max items diagnostics.
Source Generator & Snapshots
GFramework.SourceGenerators/Config/SchemaConfigGenerator.cs, GFramework.SourceGenerators.Tests/Config/SchemaConfigGeneratorSnapshotTests.cs, GFramework.SourceGenerators.Tests/Config/snapshots/.../MonsterConfig.g.txt
Generator exposes constraint docs for exclusiveMinimum/exclusiveMaximum, pattern, minItems/maxItems; update snapshots to include new constraints.
Config Validation Tooling
tools/gframework-config-tool/src/configValidation.js, tools/gframework-config-tool/test/configValidation.test.js
Tooling parses/normalizes pattern, exclusive bounds, array limits; enforces them; tests added for parsing, validation diagnostics, and invalid-pattern regression.
Tool UI & Localization
tools/gframework-config-tool/src/extension.js, tools/gframework-config-tool/src/localization.js, tools/gframework-config-tool/src/localizationKeys.js
UI hint rendering shows exclusive bounds, patterns, and min/max item hints; add localized hint strings and new validation message keys.
Docs & CI
docs/zh-CN/game/config-system.md, .github/workflows/ci.yml
Docs updated for new constraints and bootstrap usage; CI now runs Node/Bun tooling tests for the config tool.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Options as GameConfigBootstrapOptions
    participant Bootstrap as GameConfigBootstrap
    participant Loader as YamlConfigLoader
    participant Registry as IConfigRegistry
    participant HotReload as HotReloadHandler

    Client->>Options: build options (ConfigureLoader, EnableHotReload?)
    Client->>Bootstrap: new(options)
    Bootstrap->>Registry: create or use provided Registry
    Client->>Bootstrap: InitializeAsync()
    activate Bootstrap
    Bootstrap->>Loader: create YamlConfigLoader
    Bootstrap->>Loader: ConfigureLoader(loader)
    Loader->>Loader: LoadAsync(Registry)
    Loader-->>Registry: populate tables (validated against schema constraints)
    alt EnableHotReload
        Bootstrap->>HotReload: create/register hot-reload handler
        HotReload-->>Bootstrap: handler registered
    end
    deactivate Bootstrap
    Client->>Bootstrap: StartHotReload() / StopHotReload()
    Client->>Bootstrap: Dispose()
    Bootstrap->>HotReload: unregister/dispose handler
    Bootstrap->>Bootstrap: release resources
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly Related PRs

Poem

🐰 I hopped through schemas late at night,
Patterns stitched and bounds set tight,
Bootstrap hummed the loader's tune,
Hot-reload twinkled like the moon,
Configs loaded — what a sight! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Feat/config system integration' is vague and overly generic. It uses non-descriptive terms that don't convey the specific changes made in this substantial changeset, which includes new schema validation constraints, bootstrap initialization patterns, and multiple test additions. Consider using a more specific title that highlights the primary changes, such as 'Add JSON Schema constraint validation and GameConfigBootstrap initialization helper' or similar.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 93.10% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/config-system-integration

Comment @coderabbitai help to get the list of available commands and usage tips.

@deepsource-io

deepsource-io Bot commented Apr 6, 2026

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in a76630a...d99af1c on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade   Security  

Reliability  

Complexity  

Hygiene  

Code Review Summary

Analyzer Status Updated (UTC) Details
C# Apr 6, 2026 2:39p.m. Review ↗
Secrets Apr 6, 2026 2:39p.m. Review ↗

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/zh-CN/game/config-system.md (1)

119-141: ⚠️ Potential issue | 🟠 Major

The documented code snippets won't compile as written.

With <ImplicitUsings>disable</ImplicitUsings> in the .csproj template, the subsequent code examples require explicit using declarations for System, System.Threading.Tasks, and related namespaces. Additionally, two different GameConfigRuntime classes are presented in the same namespace (lines 212–250 and 272–303) with conflicting names and incompatible shapes. A user following this guide verbatim will hit compilation errors.

Add missing using statements to all code snippets, and either unify the two GameConfigRuntime examples into one or rename one of them to avoid the namespace collision.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/zh-CN/game/config-system.md` around lines 119 - 141, The snippet's
.csproj disables implicit usings (ImplicitUsings) so code examples must include
explicit using directives (e.g., using System; using System.Threading.Tasks; and
any other referenced namespaces) — add these to every C# code block; also
resolve the duplicate type problem by unifying the two GameConfigRuntime
examples into a single consistent class shape or rename one of them (e.g.,
GameConfigRuntimeV2) so there is no namespace collision and their
members/signatures align (update references in examples to the chosen name).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@GFramework.Game.Tests/Config/YamlConfigLoaderTests.cs`:
- Around line 419-462: The test
LoadAsync_Should_Throw_When_Number_Violates_Exclusive_Minimum_Or_Exclusive_Maximum
currently only verifies the exclusiveMinimum case (hp: 10); add the missing
symmetric case(s) so exclusiveMaximum is exercised as well (e.g., set hp to a
value that violates exclusiveMaximum, assert ConfigLoadException with
Diagnostic.DisplayPath "hp" and appropriate message), and likewise
update/supplement the array-length test (the test around lines 566-616) to also
check the minItems violation (or parameterize both tests) so both lower and
upper bound failures are asserted; you can either convert these into
parameterized tests or add separate test methods but ensure they reference the
same schema files and assertions (ConfigLoadException, Diagnostic.FailureKind ==
ConstraintViolation, DisplayPath, RawValue and message content) as the existing
assertions.

In `@GFramework.Game/Config/GameConfigBootstrap.cs`:
- Around line 100-174: Add a dedicated lock object (e.g., private readonly
object _lifecycleLock = new()) and use it to make lifecycle transitions atomic:
in InitializeAsync, create and fully initialize a local YamlConfigLoader (call
_options.ConfigureLoader and await loader.LoadAsync) and only inside
lock(_lifecycleLock) check that _loader is still null and then assign _loader =
loader and start hot reload (or set up fields) so exceptions during load do not
publish partial state; in StartHotReload, acquire lock(_lifecycleLock) to verify
_loader != null and _hotReload == null, then create/enable the hot-reload handle
and assign it to _hotReload only after successful creation (or perform
EnableHotReload inside the lock to avoid races); likewise guard StopHotReload
and Dispose mutations with the same lock to ensure _hotReload/_disposed
transitions are consistent and never left partially applied.
- Around line 1-3: This file is missing explicit System namespace imports
required when ImplicitUsings is disabled; add the necessary usings (e.g.,
System, System.Threading, System.Threading.Tasks) at the top of
GameConfigBootstrap.cs so references to IDisposable, Task, CancellationToken,
ArgumentNullException, ArgumentException, InvalidOperationException, and
ObjectDisposedException resolve correctly in the GameConfigBootstrap
class/constructor/methods.

In `@GFramework.Game/Config/YamlConfigSchemaValidator.cs`:
- Around line 819-831: The code currently constructs Regex instances with
RegexOptions.ExplicitCapture which disables unnamed capturing groups and breaks
patterns with backreferences; update the Regex construction in the
YamlConfigScalarConstraints return (where pattern is passed into new Regex(...))
to remove RegexOptions.ExplicitCapture and use only
RegexOptions.CultureInvariant, and similarly adjust the pattern compilation in
the TryParsePatternConstraint method so it validates/compiles patterns using the
same RegexOptions.CultureInvariant flag (ensure both places reference the same
option set to keep C# validator behavior aligned with the JS tooling).

In `@tools/gframework-config-tool/src/configValidation.js`:
- Around line 389-399: normalizeSchemaPattern currently swallows invalid regexes
and returns undefined, and matchesSchemaPattern also silences regex errors;
change this to mirror the C# behavior by validating the pattern in
normalizeSchemaPattern and throwing a clear error when new RegExp(value) fails
(include the pattern in the message) so schema parsing fails visibly, and remove
or alter the try/catch in matchesSchemaPattern so it does not catch regex
construction errors and hide them—let the exception propagate (or convert to a
ConfigLoad-like error) instead of returning true, ensuring invalid pattern
declarations are surfaced to the user; reference the functions
normalizeSchemaPattern and matchesSchemaPattern when making these changes.

In `@tools/gframework-config-tool/test/configValidation.test.js`:
- Around line 234-272: The test "validateParsedConfig should report exclusive
bounds, pattern, and array item-count mismatches" currently only asserts
failures for pattern, exclusiveMinimum and minItems; extend it to also trigger
and assert the exclusiveMaximum and maxItems branches by providing YAML inputs
that violate those constraints and adding corresponding assertions against the
diagnostics array (use the same helpers parseSchemaContent, parseTopLevelYaml
and validateParsedConfig to produce diagnostics, then add assert.match checks
similar to the existing ones that look for messages indicating "less than 20" or
"小于 20" for exclusiveMaximum and "at most 3 items" or "至多包含 3 个元素" for maxItems,
making sure to reference diagnostics indices or sort/filter diagnostics by
message to reliably target the new expectations).

---

Outside diff comments:
In `@docs/zh-CN/game/config-system.md`:
- Around line 119-141: The snippet's .csproj disables implicit usings
(ImplicitUsings) so code examples must include explicit using directives (e.g.,
using System; using System.Threading.Tasks; and any other referenced namespaces)
— add these to every C# code block; also resolve the duplicate type problem by
unifying the two GameConfigRuntime examples into a single consistent class shape
or rename one of them (e.g., GameConfigRuntimeV2) so there is no namespace
collision and their members/signatures align (update references in examples to
the chosen name).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ea848abc-4050-409b-8ff6-4d899c6393e1

📥 Commits

Reviewing files that changed from the base of the PR and between a76630a and c732285.

📒 Files selected for processing (15)
  • GFramework.Game.Tests/Config/ArchitectureConfigIntegrationTests.cs
  • GFramework.Game.Tests/Config/GameConfigBootstrapTests.cs
  • GFramework.Game.Tests/Config/YamlConfigLoaderTests.cs
  • GFramework.Game/Config/GameConfigBootstrap.cs
  • GFramework.Game/Config/GameConfigBootstrapOptions.cs
  • GFramework.Game/Config/YamlConfigSchemaValidator.cs
  • GFramework.SourceGenerators.Tests/Config/SchemaConfigGeneratorSnapshotTests.cs
  • GFramework.SourceGenerators.Tests/Config/snapshots/SchemaConfigGenerator/MonsterConfig.g.txt
  • GFramework.SourceGenerators/Config/SchemaConfigGenerator.cs
  • docs/zh-CN/game/config-system.md
  • tools/gframework-config-tool/src/configValidation.js
  • tools/gframework-config-tool/src/extension.js
  • tools/gframework-config-tool/src/localization.js
  • tools/gframework-config-tool/src/localizationKeys.js
  • tools/gframework-config-tool/test/configValidation.test.js

Comment thread GFramework.Game.Tests/Config/YamlConfigLoaderTests.cs Outdated
Comment thread GFramework.Game/Config/GameConfigBootstrap.cs
Comment thread GFramework.Game/Config/GameConfigBootstrap.cs
Comment thread GFramework.Game/Config/YamlConfigSchemaValidator.cs Outdated
Comment thread tools/gframework-config-tool/src/configValidation.js Outdated
Comment thread tools/gframework-config-tool/test/configValidation.test.js
- 新增 CI/CD 工作流配置文件,集成代码质量检查、安全扫描和构建测试
- 详细介绍配置系统架构,包括 YAML 源文件、JSON Schema 结构描述和运行时只读查询
- 提供完整的目录结构推荐和 Schema/JSON 示例配置
- 包含项目接入模板,涵盖 csproj 配置、启动帮助器和运行时读取模板
- 说明运行时校验行为,支持必填字段、类型匹配、数值范围等校验规则
- 介绍开发期热重载功能,支持配置文件变更自动刷新
- 详述生成器接入约定,包括配置类型、表包装和注册辅助生成
- 提供 VS Code 工具使用指南,支持配置浏览、表单编辑和批量操作
- 说明当前系统限制和未来发展规划,明确适用场景
Comment thread GFramework.Game/Config/GameConfigBootstrap.cs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (1)
GFramework.Game/Config/GameConfigBootstrap.cs (1)

153-177: ⚠️ Potential issue | 🟠 Major

InitializeAsync() still leaks loaded tables through Registry on hot-reload startup failure.

LoadAsync(Registry, ...) commits into the shared registry before EnableHotReload(...) runs. If watcher creation then throws, _loader and _hotReload stay unpublished, but callers can already observe the new tables through bootstrap.Registry; the Architecture sample in this PR even registers that registry before InitializeAsync(). That means this path does not actually preserve the previous stable state, and a retry may now start from an already-mutated registry.

Based on learnings "Preserve deterministic behavior in registries, lifecycle orchestration, and generated outputs in C# code" and "When adding caching, pooling, or shared mutable state in C#, document thread-safety assumptions and failure modes".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@GFramework.Game/Config/GameConfigBootstrap.cs` around lines 153 - 177,
InitializeAsync currently calls YamlConfigLoader.LoadAsync(Registry, ...) which
mutates the shared Registry before EnableHotReload(...) completes, allowing a
failing hot-reload startup to leave partially-applied tables visible; fix by
loading into a temporary/staging Registry instance (or have
YamlConfigLoader.LoadAsync accept a target Registry parameter you supply that is
not the shared Registry), call loader.LoadAsync(stagingRegistry, ...), then call
loader.EnableHotReload(stagingRegistry, _options.HotReloadOptions) and only
inside the lock(_stateGate) swap _loader/_hotReload and replace the real
Registry with the stagingRegistry if both load and watcher creation succeed;
ensure no changes are applied to the shared Registry if EnableHotReload throws
and preserve the previous Registry and _loader/_hotReload state, and keep
setting _isInitializing = false in the finally block as before.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)

137-139: Use frozen lockfile for deterministic CI installs.

Line 139 should use --frozen-lockfile to prevent dependency drift across CI runs. A bun.lock file exists in the project, and Bun's frozen lockfile mode ensures reproducible installs by requiring exact version matches.

Proposed patch
-      - name: Install config tool dependencies
-        working-directory: tools/gframework-config-tool
-        run: bun install
+      - name: Install config tool dependencies
+        working-directory: tools/gframework-config-tool
+        run: bun install --frozen-lockfile
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 137 - 139, Update the CI step named
"Install config tool dependencies" to use Bun's frozen lockfile mode: replace
the current run command that invokes "bun install" (in the step labeled Install
config tool dependencies under working-directory tools/gframework-config-tool)
with "bun install --frozen-lockfile" so the existing bun.lock is enforced and
installs are deterministic across CI runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/zh-CN/game/config-system.md`:
- Around line 200-250: The page defines two conflicting types named
GameProject.Config.GameConfigRuntime (one lifecycle wrapper using
GameConfigBootstrap and IDisposable and another in the “运行时读取模板” section) which
creates duplicate-type ambiguity; resolve this by renaming one of the samples
(e.g., LifecycleGameConfigRuntime or GameConfigReader) or by merging the
read-facade into the lifecycle wrapper so there is a single canonical
GameConfigRuntime API; update all references in the document (examples,
directory template, and any calls to InitializeAsync, Registry, Dispose, or the
read-facade methods) to use the chosen name and ensure the project/file layout
shows one Config/GameConfigRuntime.cs entry.

In `@GFramework.Game.Tests/Config/GameConfigBootstrapTests.cs`:
- Around line 117-128: The test expects the constructor to throw an
ArgumentException with ParamName "ConfigureLoader" but the current
GameConfigBootstrap constructor throws ArgumentException(..., nameof(options)),
so either update the constructor validation in GameConfigBootstrap to throw
property-specific ArgumentException for missing ConfigureLoader and RootPath
(use nameof(GameConfigBootstrapOptions.ConfigureLoader) and
nameof(GameConfigBootstrapOptions.RootPath) in the respective branches) or
relax/adjust the test to assert the ParamName equals "options" and add a new
sibling test that asserts the constructor throws the correct ParamName for a
missing RootPath; target the GameConfigBootstrap constructor and the
GameConfigBootstrapOptions properties ConfigureLoader and RootPath when making
the change.

---

Duplicate comments:
In `@GFramework.Game/Config/GameConfigBootstrap.cs`:
- Around line 153-177: InitializeAsync currently calls
YamlConfigLoader.LoadAsync(Registry, ...) which mutates the shared Registry
before EnableHotReload(...) completes, allowing a failing hot-reload startup to
leave partially-applied tables visible; fix by loading into a temporary/staging
Registry instance (or have YamlConfigLoader.LoadAsync accept a target Registry
parameter you supply that is not the shared Registry), call
loader.LoadAsync(stagingRegistry, ...), then call
loader.EnableHotReload(stagingRegistry, _options.HotReloadOptions) and only
inside the lock(_stateGate) swap _loader/_hotReload and replace the real
Registry with the stagingRegistry if both load and watcher creation succeed;
ensure no changes are applied to the shared Registry if EnableHotReload throws
and preserve the previous Registry and _loader/_hotReload state, and keep
setting _isInitializing = false in the finally block as before.

---

Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 137-139: Update the CI step named "Install config tool
dependencies" to use Bun's frozen lockfile mode: replace the current run command
that invokes "bun install" (in the step labeled Install config tool dependencies
under working-directory tools/gframework-config-tool) with "bun install
--frozen-lockfile" so the existing bun.lock is enforced and installs are
deterministic across CI runs.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7eb84400-50e8-4d78-9735-babaaba317e2

📥 Commits

Reviewing files that changed from the base of the PR and between c732285 and bba589a.

📒 Files selected for processing (8)
  • .github/workflows/ci.yml
  • GFramework.Game.Tests/Config/GameConfigBootstrapTests.cs
  • GFramework.Game.Tests/Config/YamlConfigLoaderTests.cs
  • GFramework.Game/Config/GameConfigBootstrap.cs
  • GFramework.Game/Config/YamlConfigSchemaValidator.cs
  • docs/zh-CN/game/config-system.md
  • tools/gframework-config-tool/src/configValidation.js
  • tools/gframework-config-tool/test/configValidation.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
  • tools/gframework-config-tool/test/configValidation.test.js
  • GFramework.Game/Config/YamlConfigSchemaValidator.cs

Comment thread docs/zh-CN/game/config-system.md Outdated
Comment thread GFramework.Game.Tests/Config/GameConfigBootstrapTests.cs
- 将断言中的参数名从 "ConfigureLoader" 更新为 "options"
- 确保测试用例与实际实现保持一致

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
GFramework.Game.Tests/Config/GameConfigBootstrapTests.cs (1)

114-128: ⚠️ Potential issue | 🟠 Major

Add constructor coverage for invalid RootPath contract branch.

This currently verifies the missing ConfigureLoader branch, but it skips the sibling constructor validation for RootPath (null/empty/whitespace). That leaves one public API guard branch unpinned and easier to regress.

🧪 Suggested test addition
 [Test]
 public void Constructor_Should_Throw_When_ConfigureLoader_Is_Missing()
 {
     var exception = Assert.Throws<ArgumentException>(() =>
         _ = new GameConfigBootstrap(
             new GameConfigBootstrapOptions
             {
                 RootPath = _rootPath
             }));

     Assert.That(exception!.ParamName, Is.EqualTo("options"));
 }
+
+[Test]
+public void Constructor_Should_Throw_When_RootPath_Is_Whitespace()
+{
+    var exception = Assert.Throws<ArgumentException>(() =>
+        _ = new GameConfigBootstrap(
+            new GameConfigBootstrapOptions
+            {
+                RootPath = "   ",
+                ConfigureLoader = static _ => { }
+            }));
+
+    Assert.That(exception!.ParamName, Is.EqualTo("options"));
+}

Based on learnings "When a public API defines multiple contract branches, tests MUST cover the meaningful variants, including null, empty, default, and filtered inputs when those branches change behavior".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@GFramework.Game.Tests/Config/GameConfigBootstrapTests.cs` around lines 114 -
128, Add a new unit test in GameConfigBootstrapTests that asserts the public
constructor GameConfigBootstrap throws (ArgumentException or
ArgumentNullException as the API defines) when
GameConfigBootstrapOptions.RootPath is null, empty, or whitespace; locate the
constructor under test (new GameConfigBootstrap(GameConfigBootstrapOptions)) and
add parameterized cases (null, "", "   ") verifying the thrown exception and
ParamName equals "options" or "RootPath" per the constructor's contract so the
RootPath guard branch is covered alongside the existing ConfigureLoader test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@GFramework.Game.Tests/Config/GameConfigBootstrapTests.cs`:
- Around line 114-128: Add a new unit test in GameConfigBootstrapTests that
asserts the public constructor GameConfigBootstrap throws (ArgumentException or
ArgumentNullException as the API defines) when
GameConfigBootstrapOptions.RootPath is null, empty, or whitespace; locate the
constructor under test (new GameConfigBootstrap(GameConfigBootstrapOptions)) and
add parameterized cases (null, "", "   ") verifying the thrown exception and
ParamName equals "options" or "RootPath" per the constructor's contract so the
RootPath guard branch is covered alongside the existing ConfigureLoader test.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0ebd897d-9812-468a-abcc-b79202f58708

📥 Commits

Reviewing files that changed from the base of the PR and between bba589a and cd32a00.

📒 Files selected for processing (1)
  • GFramework.Game.Tests/Config/GameConfigBootstrapTests.cs

- 介绍面向静态游戏内容的 AI-First 配表方案
- 详细说明 YAML 配置源文件和 JSON Schema 结构描述功能
- 提供推荐的目录结构和完整的 Schema 与 YAML 示例
- 包含配置系统的接入模板和运行时读取方法
- 说明开发期热重载功能和 VS Code 工具集成
- 记录当前限制和独立 Config Studio 评估结论
- 提供 Architecture 推荐接入模板和热重载配置方法

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
docs/zh-CN/game/config-system.md (1)

669-693: 热重载主示例建议与前文统一为 GameConfigBootstrap.StartHotReload(...)

前文(Line 424 起)已把 GameConfigBootstrap.StartHotReload(...) 定义为推荐路径,但这里仍用 loader.EnableHotReload(...) 作为主示例,读者容易把“兼容入口”误解为“首选入口”。建议把此段主示例改为 bootstrap 版本,并把 EnableHotReload 下沉为“底层高级用法”小节。

📌 文档示例调整建议
-## 开发期热重载
-
-如果你希望在开发期修改配置文件后自动刷新运行时表,可以在初次加载完成后启用热重载:
+## 开发期热重载
+
+如果你希望在开发期修改配置文件后自动刷新运行时表,推荐沿用统一入口,在初次加载完成后通过 `GameConfigBootstrap.StartHotReload(...)` 启用:

 ```csharp
-using GFramework.Game.Abstractions.Config;
-using GFramework.Game.Config;
-using GFramework.Game.Config.Generated;
-
-var registry = new ConfigRegistry();
-var loader = new YamlConfigLoader("config-root")
-    .RegisterAllGeneratedConfigTables();
-
-await loader.LoadAsync(registry);
-
-var hotReload = loader.EnableHotReload(
-    registry,
-    onTableReloaded: tableName => Console.WriteLine($"Reloaded: {tableName}"),
-    onTableReloadFailed: (tableName, exception) =>
-    {
-        var diagnostic = (exception as ConfigLoadException)?.Diagnostic;
-        Console.WriteLine($"Reload failed: {tableName}, {exception.Message}");
-        Console.WriteLine($"Failure kind: {diagnostic?.FailureKind}");
-    });
+await bootstrap.InitializeAsync();
+bootstrap.StartHotReload(
+    new YamlConfigHotReloadOptions
+    {
+        OnTableReloaded = tableName => Console.WriteLine($"Reloaded: {tableName}"),
+        OnTableReloadFailed = (tableName, exception) =>
+        {
+            var diagnostic = (exception as ConfigLoadException)?.Diagnostic;
+            Console.WriteLine($"Reload failed: {tableName}");
+            Console.WriteLine($"Failure kind: {diagnostic?.FailureKind}");
+        }
+    });

+如果你需要直接控制底层加载器,再使用 YamlConfigLoader.EnableHotReload(...)

</details>

As per coding guidelines "Keep code samples, package names, and command examples in documentation aligned with the current repository state" and "Update the relevant `README.md` or `docs/` page when behavior, setup steps, architecture guidance, or user-facing examples change".

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @docs/zh-CN/game/config-system.md around lines 669 - 693, The current
hot-reload example uses YamlConfigLoader.EnableHotReload which conflicts with
the earlier recommended GameConfigBootstrap.StartHotReload; update the example
to call bootstrap.InitializeAsync() followed by
GameConfigBootstrap.StartHotReload(...) using a YamlConfigHotReloadOptions
object (set OnTableReloaded and OnTableReloadFailed and map the diagnostic
extraction from ConfigLoadException as shown), and move the
YamlConfigLoader.EnableHotReload snippet into a separate "advanced / low-level
usage" subsection so readers see StartHotReload
(GameConfigBootstrap.StartHotReload) as the primary example while preserving
EnableHotReload (YamlConfigLoader.EnableHotReload) as an alternative.


</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @docs/zh-CN/game/config-system.md:

  • Around line 669-693: The current hot-reload example uses
    YamlConfigLoader.EnableHotReload which conflicts with the earlier recommended
    GameConfigBootstrap.StartHotReload; update the example to call
    bootstrap.InitializeAsync() followed by GameConfigBootstrap.StartHotReload(...)
    using a YamlConfigHotReloadOptions object (set OnTableReloaded and
    OnTableReloadFailed and map the diagnostic extraction from ConfigLoadException
    as shown), and move the YamlConfigLoader.EnableHotReload snippet into a separate
    "advanced / low-level usage" subsection so readers see StartHotReload
    (GameConfigBootstrap.StartHotReload) as the primary example while preserving
    EnableHotReload (YamlConfigLoader.EnableHotReload) as an alternative.

</details>

---

<details>
<summary>ℹ️ Review info</summary>

<details>
<summary>⚙️ Run configuration</summary>

**Configuration used**: Path: .coderabbit.yaml

**Review profile**: CHILL

**Plan**: Pro

**Run ID**: `154ad9ba-7c9b-4478-b8b9-2e06ebf79f7c`

</details>

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between cd32a006c69eab0d38a80b20c419535117ea5d7b and d99af1cfac58e27edb9bc1d3b9fac836e603f582.

</details>

<details>
<summary>📒 Files selected for processing (1)</summary>

* `docs/zh-CN/game/config-system.md`

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

@GeWuYou GeWuYou merged commit d120236 into main Apr 6, 2026
10 checks passed
@GeWuYou GeWuYou deleted the feat/config-system-integration branch April 6, 2026 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant