You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
We reviewed changes in a22e522...d213707 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
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.
The reason will be displayed to describe this comment to others. Learn more.
Switch expression does not have `default` case
The default case is executed when none of the specified cases in the switch statement match. This is particularly useful when switch fails to cover all the possible values, either because all the cases weren't specified or that the range of values was later expanded.
- yield return new WaitForEndOfFrame();+ yield return new WaitOneFrame();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/zh-CN/godot/coroutine.md` around lines 28 - 45, The example uses
Demo().RunCoroutine() which defaults to Segment.Process but then yields
WaitForEndOfFrame (which only completes on Segment.DeferredProcess), causing the
coroutine to never progress to GD.Print("done"); fix by either changing the
yield in Demo() from WaitForEndOfFrame to WaitOneFrame if you want to keep the
default RunCoroutine() behavior, or explicitly run the coroutine in the deferred
stage (e.g., call RunCoroutine(Segment.DeferredProcess)) to preserve the
WaitForEndOfFrame semantics; update the Demo() method or the RunCoroutine()
invocation accordingly.
🧹 Nitpick comments (5)
docs/zh-CN/core/coroutine.md (1)
142-151: Task 桥接示例可考虑收敛为单一“推荐写法”。
当前同时展示两种入口，读者可能不清楚优先级；可在该段保留一种主路径，另一种放到“补充/兼容写法”。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/zh-CN/core/coroutine.md` around lines 142 - 151, 示例中同时展示
LoadDataAsync().AsCoroutineInstruction() 与
scheduler.StartTaskAsCoroutine(LoadDataAsync())
两种入口会让读者不清晰优先级；将文档收敛为单一“推荐写法”（选择一个作为主路径，例如推荐使用 AsCoroutineInstruction() 或者推荐使用
scheduler.StartTaskAsCoroutine(...)），在该段保留选定的主写法并把另一种以“补充/兼容写法”小节或注释形式移动到段落末尾，明确标注哪种为首选并保留对
LoadDataAsync、AsCoroutineInstruction、StartTaskAsCoroutine 和 scheduler
的说明以便定位代码示例。
Verify each finding against the current code and only fix it if needed.
In `@GFramework.Godot.Tests/GFramework.Godot.Tests.csproj` at line 10, 当前测试项目的
MSBuild 配置包含 <WarningLevel>0</WarningLevel>，这会全局抑制编译警告；请在对应测试项目的 .csproj 中移除该
<WarningLevel>0</WarningLevel> 元素或将其改为
<WarningLevel>4</WarningLevel>，如需仅屏蔽特定警告则改用 <NoWarn> 列表来有选择地禁用特定警告；定位并修改包含
<WarningLevel>0</WarningLevel> 的块以恢复默认警告可见性。
Verify each finding against the current code and only fix it if needed.
In `@GFramework.Core/Coroutine/CoroutineScheduler.cs` around lines 582 - 606, The
Clear() method currently relies on FinalizeCoroutine to remove entries from
_completionSources but should defensively clear the dictionary itself to avoid
edge-case leaks; update Clear() to explicitly call _completionSources.Clear()
(e.g., near the other collection clears) so that _completionSources is always
emptied when Clear() completes while leaving FinalizeCoroutine unchanged.
public IReadOnlyList<CoroutineSnapshot> GetActiveSnapshots()
{
- return _metadata- .Select(pair => pair.Value)- .Select(meta => new- {- Metadata = meta,- Slot = _slots[meta.SlotIndex]- })- .Where(item => item.Slot is not null)- .Select(item => CreateSnapshot(item.Metadata, item.Slot!))- .ToArray();+ var result = new List<CoroutineSnapshot>(_metadata.Count);+ foreach (var meta in _metadata.Values)+ {+ var slot = _slots[meta.SlotIndex];+ if (slot is not null)+ {+ result.Add(CreateSnapshot(meta, slot));+ }+ }+ return result;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@GFramework.Core/Coroutine/CoroutineScheduler.cs` around lines 140 - 152,
GetActiveSnapshots currently builds multiple intermediate allocations via LINQ
(anonymous objects and multiple projections). Replace the LINQ chain in
GetActiveSnapshots with an explicit loop: allocate a List<CoroutineSnapshot>
with an appropriate initial capacity, iterate over _metadata.Values (or
_metadata), look up the slot via _slots[meta.SlotIndex], skip null slots, call
CreateSnapshot(meta, slot) and Add the result to the list, then return
list.ToArray() (or the list if IReadOnlyList is acceptable). This minimizes
allocations while keeping behavior identical; refer to GetActiveSnapshots,
_metadata, _slots, and CreateSnapshot to locate the change.
Verify each finding against the current code and only fix it if needed.
In `@GFramework.Core/Coroutine/CoroutineScheduler.cs` around lines 754 - 762,
CanAdvanceInstruction currently uses concrete-type pattern matching
(WaitForFixedUpdate, WaitForEndOfFrame) to gate stages which makes adding new
stage-bound instructions error-prone; modify the design by adding an optional
execution-stage marker to IYieldInstruction (e.g. a nullable/optional property
like DesiredExecutionStage or ExecutionStage? on IYieldInstruction) and then
update CanAdvanceInstruction to first check instruction.ExecutionStage (if set,
compare to coroutine's CoroutineExecutionStage) and only fall back to the
existing type checks for backward compatibility with WaitForFixedUpdate and
WaitForEndOfFrame; update the interface (IYieldInstruction) and the
CanAdvanceInstruction method accordingly so new stage-bound instructions only
need to set the property.
🤖 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.Core/Coroutine/CoroutineScheduler.cs`:
- Line 40: _completionStatuses is never cleared and grows when FinalizeCoroutine
adds entries, causing a memory leak; update the Clear() method to clear
_completionStatuses (in addition to existing cleared collections) and/or remove
the specific entry from _completionStatuses when a coroutine is finalized inside
FinalizeCoroutine to avoid unbounded growth; alternatively implement a simple
eviction policy (time or capacity-based) that prunes old entries from
_completionStatuses—refer to the fields/methods _completionStatuses,
FinalizeCoroutine, and Clear to locate where to add clearing/removal logic.
- Around line 387-392: HandleYieldInstruction currently calls
Run(waitForCoroutine.Coroutine) without propagating the parent coroutine's
CancellationToken, so nested coroutines continue running when the parent is
cancelled; update CoroutineSlot to store the parent CancellationToken (in
addition to the existing CancellationTokenRegistration) and modify Run(...) (and
the call site in HandleYieldInstruction) to accept and pass that token into the
nested coroutine creation, ensuring the nested coroutine inherits cancellation;
alternatively, if this behavior is intended, add clear documentation on
CoroutineSlot/Run and WaitForCoroutine that nested coroutines are
cancellation-isolated from their parents.
In `@GFramework.Godot/Coroutine/GodotTimeSource.cs`:
- Around line 38-58: The absolute-time branch uses the raw value to set
_lastAbsoluteTime and CurrentTime which allows time to go backwards when the
provider is rewound; instead compute a clampedDelta = Math.Max(0, value -
_lastAbsoluteTime), assign DeltaTime = clampedDelta, then advance both
_lastAbsoluteTime and CurrentTime by clampedDelta (e.g. _lastAbsoluteTime +=
clampedDelta; CurrentTime = _lastAbsoluteTime) so CurrentTime never decreases;
keep the existing initialization path for _initialized that sets
_lastAbsoluteTime and CurrentTime from the first provider value.
In `@GFramework.Godot/Coroutine/Timing.cs`:
- Around line 516-566: KillCoroutines(Node) and GetOwnedCoroutineCount(Node)
only query the singleton Instance and thus miss coroutines owned by other Timing
instances; change them to iterate ActiveInstances and aggregate results. For
KillCoroutines(Node) call KillOwnedCoroutinesOnInstance(owner) on each entry in
ActiveInstances and sum the returned counts; for GetOwnedCoroutineCount(Node)
call GetOwnedCoroutineCountOnInstance(owner) on each ActiveInstances entry and
return the total. Keep references to Instance, ActiveInstances,
KillOwnedCoroutinesOnInstance, and GetOwnedCoroutineCountOnInstance to locate
the methods.
- Around line 450-475: 在 RunOwnedCoroutineOnInstance 中，RunCoroutineOnInstance
可能会同步完成/失败/取消并导致句柄在返回前被清理（见 HandleCoroutineFinished），因此在调用
RegisterOwnedCoroutine(owner, handle) 前再次确认句柄仍然有效以避免把已结束的协程注册为 owned
coroutine；具体修复是在 RunOwnedCoroutineOnInstance（调用 RunCoroutineOnInstance 后、调用
RegisterOwnedCoroutine 前）再次检查 handle.IsValid（或使用现有的句柄存活判断方法），仅当仍然有效时才调用
RegisterOwnedCoroutine(owner, handle)，从而避免污染
_ownedCoroutineRegistrations/_ownedCoroutinesByNode 和不必要的 TreeExiting 订阅。
---
Outside diff comments:
In `@docs/zh-CN/godot/coroutine.md`:
- Around line 28-45: The example uses Demo().RunCoroutine() which defaults to
Segment.Process but then yields WaitForEndOfFrame (which only completes on
Segment.DeferredProcess), causing the coroutine to never progress to
GD.Print("done"); fix by either changing the yield in Demo() from
WaitForEndOfFrame to WaitOneFrame if you want to keep the default RunCoroutine()
behavior, or explicitly run the coroutine in the deferred stage (e.g., call
RunCoroutine(Segment.DeferredProcess)) to preserve the WaitForEndOfFrame
semantics; update the Demo() method or the RunCoroutine() invocation
accordingly.
---
Nitpick comments:
In `@docs/zh-CN/core/coroutine.md`:
- Around line 142-151: 示例中同时展示 LoadDataAsync().AsCoroutineInstruction() 与
scheduler.StartTaskAsCoroutine(LoadDataAsync())
两种入口会让读者不清晰优先级；将文档收敛为单一“推荐写法”（选择一个作为主路径，例如推荐使用 AsCoroutineInstruction() 或者推荐使用
scheduler.StartTaskAsCoroutine(...)），在该段保留选定的主写法并把另一种以“补充/兼容写法”小节或注释形式移动到段落末尾，明确标注哪种为首选并保留对
LoadDataAsync、AsCoroutineInstruction、StartTaskAsCoroutine 和 scheduler
的说明以便定位代码示例。
In `@GFramework.Core/Coroutine/CoroutineScheduler.cs`:
- Around line 582-606: The Clear() method currently relies on FinalizeCoroutine
to remove entries from _completionSources but should defensively clear the
dictionary itself to avoid edge-case leaks; update Clear() to explicitly call
_completionSources.Clear() (e.g., near the other collection clears) so that
_completionSources is always emptied when Clear() completes while leaving
FinalizeCoroutine unchanged.
- Around line 140-152: GetActiveSnapshots currently builds multiple intermediate
allocations via LINQ (anonymous objects and multiple projections). Replace the
LINQ chain in GetActiveSnapshots with an explicit loop: allocate a
List<CoroutineSnapshot> with an appropriate initial capacity, iterate over
_metadata.Values (or _metadata), look up the slot via _slots[meta.SlotIndex],
skip null slots, call CreateSnapshot(meta, slot) and Add the result to the list,
then return list.ToArray() (or the list if IReadOnlyList is acceptable). This
minimizes allocations while keeping behavior identical; refer to
GetActiveSnapshots, _metadata, _slots, and CreateSnapshot to locate the change.
- Around line 754-762: CanAdvanceInstruction currently uses concrete-type
pattern matching (WaitForFixedUpdate, WaitForEndOfFrame) to gate stages which
makes adding new stage-bound instructions error-prone; modify the design by
adding an optional execution-stage marker to IYieldInstruction (e.g. a
nullable/optional property like DesiredExecutionStage or ExecutionStage? on
IYieldInstruction) and then update CanAdvanceInstruction to first check
instruction.ExecutionStage (if set, compare to coroutine's
CoroutineExecutionStage) and only fall back to the existing type checks for
backward compatibility with WaitForFixedUpdate and WaitForEndOfFrame; update the
interface (IYieldInstruction) and the CanAdvanceInstruction method accordingly
so new stage-bound instructions only need to set the property.
In `@GFramework.Godot.Tests/GFramework.Godot.Tests.csproj`:
- Line 10: 当前测试项目的 MSBuild 配置包含
<WarningLevel>0</WarningLevel>，这会全局抑制编译警告；请在对应测试项目的 .csproj 中移除该
<WarningLevel>0</WarningLevel> 元素或将其改为
<WarningLevel>4</WarningLevel>，如需仅屏蔽特定警告则改用 <NoWarn> 列表来有选择地禁用特定警告；定位并修改包含
<WarningLevel>0</WarningLevel> 的块以恢复默认警告可见性。
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ae438e8-841d-4571-adef-0a246fe944ec
📥 Commits
Reviewing files that changed from the base of the PR and between a22e522 and 6cac882.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 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.Godot/Coroutine/Timing.cs`:
- Around line 1-4: The file is missing the required using for types from
GFramework.Core.Coroutine, causing unresolved symbols like CoroutineScheduler,
CoroutineHandle, and CoroutineSnapshot; fix it by adding an explicit using
directive for GFramework.Core.Coroutine at the top of Timing.cs so those types
resolve (do not rely on implicit imports), then rebuild to ensure
CoroutineScheduler, CoroutineHandle and CoroutineSnapshot are recognized.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f99b6cf-8fd7-48fe-a9a1-9fba32839d67
📥 Commits
Reviewing files that changed from the base of the PR and between 6cac882 and 2053451.
public static int KillCoroutines(string tag)
{
- return Instance.KillByTagOnInstance(tag);+ var count = 0;+ foreach (var timing in EnumerateActiveInstances())+ {+ count += timing.KillByTagOnInstance(tag);+ }+ return count;
}
public static int KillAllCoroutines()
{
- return Instance.ClearOnInstance();+ var count = 0;+ foreach (var timing in EnumerateActiveInstances())+ {+ count += timing.ClearOnInstance();+ }+ return count;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@GFramework.Godot/Coroutine/Timing.cs` around lines 536 - 548,
KillCoroutines(string tag) and KillAllCoroutines() currently call
Instance.KillByTagOnInstance(tag) and Instance.ClearOnInstance(), which misses
coroutines on other Timing instances; change them to iterate ActiveInstances
(like KillCoroutines(Node) and GetOwnedCoroutineCount(Node) do), call
KillByTagOnInstance(tag) / ClearOnInstance() on each non-null Timing in
ActiveInstances, sum the returned counts, and return the total so behavior is
consistent across multiple Timing instances.
Verify each finding against the current code and only fix it if needed.
In `@GFramework.Godot/Coroutine/Timing.cs` around lines 783 - 789,
HandleCoroutineFinished currently ignores the exception parameter; add a
debug/error log when exception is non-null so coroutine failures are visible
from the Timing layer. Inside HandleCoroutineFinished (which already calls
CleanupOwnedCoroutineRegistration), check if the exception is not null and log
it (including handle and status) via the project's logging mechanism (e.g.,
GD.PrintErr or the existing logger) before or after
CleanupOwnedCoroutineRegistration to aid Godot-side debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 159-165: The workflow runs multiple background `dotnet test` jobs
(e.g., the `dotnet test GFramework.Godot.Tests ... --results-directory
TestResults` invocation) and then uses a bare `wait`, which only returns the
last job's exit code and can hide earlier failures; change to capture each
background PID (store $! for every launched test), wait on each PID
individually, collect and aggregate their exit codes (e.g., track a non-zero
flag or compute max exit code), and then `exit` non-zero if any test returned
failure so the CI step fails when any background test fails.
---
Nitpick comments:
In `@GFramework.Godot/Coroutine/Timing.cs`:
- Around line 536-548: KillCoroutines(string tag) and KillAllCoroutines()
currently call Instance.KillByTagOnInstance(tag) and Instance.ClearOnInstance(),
which misses coroutines on other Timing instances; change them to iterate
ActiveInstances (like KillCoroutines(Node) and GetOwnedCoroutineCount(Node) do),
call KillByTagOnInstance(tag) / ClearOnInstance() on each non-null Timing in
ActiveInstances, sum the returned counts, and return the total so behavior is
consistent across multiple Timing instances.
- Around line 783-789: HandleCoroutineFinished currently ignores the exception
parameter; add a debug/error log when exception is non-null so coroutine
failures are visible from the Timing layer. Inside HandleCoroutineFinished
(which already calls CleanupOwnedCoroutineRegistration), check if the exception
is not null and log it (including handle and status) via the project's logging
mechanism (e.g., GD.PrintErr or the existing logger) before or after
CleanupOwnedCoroutineRegistration to aid Godot-side debugging.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bc1e405-c8fa-434e-a834-681acf81b974
📥 Commits
Reviewing files that changed from the base of the PR and between 2053451 and d213707.
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 159 - 165, The workflow runs multiple
background `dotnet test` jobs (e.g., the `dotnet test GFramework.Godot.Tests ...
--results-directory TestResults` invocation) and then uses a bare `wait`, which
only returns the last job's exit code and can hide earlier failures; change to
capture each background PID (store $! for every launched test), wait on each PID
individually, collect and aggregate their exit codes (e.g., track a non-zero
flag or compute max exit code), and then `exit` non-zero if any test returned
failure so the CI step fails when any background test fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary by CodeRabbit
新特性
测试
文档
其他