Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 65 additions & 21 deletions .github/workflows/claude-code-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ jobs:
runs-on: ubuntu-latest
permissions:
contents: read
pull-requests: read
issues: read
pull-requests: write
id-token: write

steps:
Expand All @@ -37,22 +36,67 @@ jobs:
with:
claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}
prompt: |
Please review this pull request and provide feedback on:
- Code quality and best practices
- Potential bugs or issues
- Performance considerations
- Security concerns
- Backwards compatibility

For each of the points above, do not point out what works well, only what could be improved (if anything).
Be constructive and helpful in your feedback but do not repeat yourself - only summarise potential issues.
Test coverage is currently not a priority.
Prefix section headers with emojis and use dividers for better readability.

Use the repository's CLAUDE.md for guidance on style and conventions. Be constructive and helpful in your feedback.
Use `gh pr comment` with your Bash tool to leave your review as a comment on the PR.

# See https://github.com/anthropics/claude-code-action/blob/main/docs/usage.md
# or https://docs.anthropic.com/en/docs/claude-code/sdk#command-line for available options
claude_args: '--model sonnet --allowed-tools "Bash(gh issue view:*),Bash(gh search:*),Bash(gh issue list:*),Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr list:*)"'
use_sticky_comment: true
You are reviewing PR #${{ github.event.pull_request.number }} in the repository ${{ github.repository }}.

Review this pull request and provide feedback focused only on improvements needed (not what works well):

**Categories to check:**
1. Code quality and best practices
2. Potential bugs or issues
3. Performance considerations
4. Security concerns
5. Backwards compatibility

**Process:**
- The PR number is: ${{ github.event.pull_request.number }}
- View the PR using: `gh pr view ${{ github.event.pull_request.number }} --repo ${{ github.repository }}`
- Read CLAUDE.md to understand best practices
- View the PR diff using: `gh pr diff ${{ github.event.pull_request.number }} --repo ${{ github.repository }}`
- If an issue spans multiple categories, list it only once in the most relevant section
- Prioritize by severity: 🔴 Critical → 🟡 Major → 🔵 Minor
- Focus only on changes introduced in this PR, not pre-existing code issues
- Test coverage is currently not a priority

**Review Workflow (Follow these steps):**
1. **Analysis Phase**: Review the PR diff and identify potential issues
2. **Validation Phase**: For each issue you find, verify it by:
- Re-reading the relevant code carefully
- Checking if your suggested fix is actually different from the current code
- Confirming the issue violates documented standards (check CLAUDE.md)
- Ensuring your criticism is actionable and specific
3. **Draft Phase**: Write your review only after validating all issues
4. **Quality Check**: Before posting, remove any issues where:
- Your "before" and "after" code snippets are identical
- You're uncertain or use phrases like "appears", "might", "should verify"
- The issue is theoretical without clear impact
5. **Post Phase**: Only post the review if you have concrete, validated feedback

**Edge Case Policy:**
Only flag edge cases that meet ALL of these criteria:
1. Realistic: Could happen in normal usage or common error scenarios
2. Impactful: Would cause bugs, security issues, or data problems (not just "it's not perfect")
3. Actionable: Can be fixed with reasonable effort in this PR's scope

Ignore theoretical issues that require multiple unlikely conditions or malicious input patterns.
Use the "would this bother a pragmatic senior developer?" test.

Maximum chain of assumptions: 2 levels deep. Skip exotic input combinations that violate documented assumptions.

**Feedback style:**
- Provide specific code examples or line references showing the issue
- Suggest concrete fixes with code snippets where helpful
- Keep total feedback under 500 words
- Use section headers with emojis and horizontal dividers (---)
- If no improvements needed in a category, simply state "No issues found"
- Use neutral language; focus on the code, not the author
- If the PR looks good overall, say so clearly rather than forcing criticism

**Comment Management (IMPORTANT):**
Post your review using this command, which will edit your last comment if one exists, or create a new one:
```bash
gh pr comment ${{ github.event.pull_request.number }} --repo ${{ github.repository }} --edit-last --create-if-none --body "<review>"
```

Ensure proper escaping of quotes and special characters in the comment body. Use single quotes around the body and escape any single quotes inside with '\''
claude_args: |
--allowedTools "Read,Bash(gh pr:*),Grep,Glob"
2 changes: 1 addition & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"files.exclude": {
"**/*.meta": true
},
"dotnet.defaultSolution": "unity.sln",
"dotnet.defaultSolution": "unity.slnx",
"editor.tabSize": 4,
"editor.indentSize": "tabSize"
}
2 changes: 1 addition & 1 deletion Assets/Talo Game Services/Talo/Runtime/APIs/BaseAPI.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace TaloGameServices
public class BaseAPI
{
// automatically updated with a pre-commit hook
private const string ClientVersion = "0.49.0";
private const string ClientVersion = "0.50.0";

protected string baseUrl;

Expand Down
52 changes: 52 additions & 0 deletions Assets/Talo Game Services/Talo/Runtime/APIs/DebouncedAPI.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using UnityEngine;

namespace TaloGameServices
{
public abstract class DebouncedAPI<TOperation> : BaseAPI where TOperation : Enum
{
private class DebouncedOperation
{
public float nextUpdateTime;
public bool hasPending;
}

private readonly Dictionary<TOperation, DebouncedOperation> operations = new();

protected DebouncedAPI(string service) : base(service) { }

protected void Debounce(TOperation operation)
{
if (!operations.ContainsKey(operation))
{
operations[operation] = new DebouncedOperation();
}

operations[operation].nextUpdateTime = Time.realtimeSinceStartup + Talo.Settings.debounceTimerSeconds;
operations[operation].hasPending = true;
}

public async Task ProcessPendingUpdates()
{
var keysToProcess = new List<TOperation>();

foreach (var kvp in operations)
{
if (kvp.Value.hasPending && Time.realtimeSinceStartup >= kvp.Value.nextUpdateTime)
{
keysToProcess.Add(kvp.Key);
}
}

foreach (var key in keysToProcess)
{
operations[key].hasPending = false;
await ExecuteDebouncedOperation(key);
}
}

protected abstract Task ExecuteDebouncedOperation(TOperation operation);
}
}
11 changes: 11 additions & 0 deletions Assets/Talo Game Services/Talo/Runtime/APIs/DebouncedAPI.cs.meta

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 9 additions & 7 deletions Assets/Talo Game Services/Talo/Runtime/APIs/EventsAPI.cs
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,15 @@ public async Task Flush()
{
Talo.IdentityCheck();

var eventsToSend = queue.ToArray();
if (eventsToSend.Length == 0)
if (lockFlushes)
{
flushAttemptedDuringLock = true;
return;
}

if (lockFlushes)
var eventsToSend = queue.ToArray();
if (eventsToSend.Length == 0)
{
flushAttemptedDuringLock = true;
return;
}

Expand All @@ -103,14 +103,16 @@ public async Task Flush()

await Call(uri, "POST", content);
OnFlushed.Invoke();

eventsToFlush.Clear();
lockFlushes = false;
}
catch (Exception ex)
{
Debug.LogError(ex.Message);
}
finally
{
eventsToFlush.Clear();
lockFlushes = false;
}

if (flushAttemptedDuringLock)
{
Expand Down
9 changes: 9 additions & 0 deletions Assets/Talo Game Services/Talo/Runtime/APIs/HealthCheckAPI.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ public enum HealthCheckStatus
public class HealthCheckAPI : BaseAPI
{
private HealthCheckStatus lastHealthCheckStatus = HealthCheckStatus.UNKNOWN;
private float nextPingTime;

public HealthCheckAPI() : base("v1/health-check") { }

Expand All @@ -24,6 +25,12 @@ public HealthCheckStatus GetLastStatus()

public async Task<bool> Ping()
{
var bustCache = lastHealthCheckStatus == HealthCheckStatus.UNKNOWN || Time.realtimeSinceStartup >= nextPingTime;
if (!bustCache)
{
return lastHealthCheckStatus == HealthCheckStatus.OK;
}

var uri = new Uri(baseUrl);

bool success;
Expand Down Expand Up @@ -57,6 +64,8 @@ public async Task<bool> Ping()
}
}

nextPingTime = Time.realtimeSinceStartup + Talo.Settings.debounceTimerSeconds;

return success;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public async Task<LeaderboardEntriesResponse> GetEntriesForCurrentPlayer(string
var json = await Call(uri, "POST", Prop.SanitiseJson(content));

var res = JsonUtility.FromJson<LeaderboardEntryResponse>(json);
_entriesManager.UpsertEntry(internalName, res.entry);
_entriesManager.UpsertEntry(internalName, res.entry, true);

return (res.entry, res.updated);
}
Expand Down
22 changes: 21 additions & 1 deletion Assets/Talo Game Services/Talo/Runtime/APIs/PlayersAPI.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,13 @@ public class MergeOptions
public string postMergeIdentityService = "";
}

public class PlayersAPI : BaseAPI
public class PlayersAPI : DebouncedAPI<PlayersAPI.DebouncedOperation>
{
public enum DebouncedOperation
{
Update
}

public event Action<Player> OnIdentified;
public event Action OnIdentificationStarted;
public event Action OnIdentificationFailed;
Expand Down Expand Up @@ -103,6 +108,21 @@ public async Task<Player> IdentifySteam(string ticket, string identity = "")
return Talo.CurrentPlayer;
}

protected override async Task ExecuteDebouncedOperation(DebouncedOperation operation)
{
switch (operation)
{
case DebouncedOperation.Update:
await Update();
break;
}
}

public void DebounceUpdate()
{
Debounce(DebouncedOperation.Update);
}

public async Task<Player> Update()
{
Talo.IdentityCheck();
Expand Down
43 changes: 41 additions & 2 deletions Assets/Talo Game Services/Talo/Runtime/APIs/SavesAPI.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,13 @@

namespace TaloGameServices
{
public class SavesAPI : BaseAPI
public class SavesAPI : DebouncedAPI<SavesAPI.DebouncedOperation>
{
public enum DebouncedOperation
{
Update
}

internal SavesManager savesManager;
internal SavesContentManager contentManager;

Expand Down Expand Up @@ -186,9 +191,43 @@ public async Task<GameSave> CreateSave(string saveName, SaveContent content = nu
return savesManager.CreateSave(save);
}

protected override async Task ExecuteDebouncedOperation(DebouncedOperation operation)
{
switch (operation)
{
case DebouncedOperation.Update:
var currentSave = savesManager.CurrentSave;
if (currentSave != null)
{
await UpdateSave(currentSave.id);
}
break;
}
}

public void DebounceUpdate()
{
Debounce(DebouncedOperation.Update);
}

public async Task<GameSave> UpdateCurrentSave(string newName = "")
{
return await UpdateSave(savesManager.CurrentSave.id, newName);
var currentSave = savesManager.CurrentSave;
if (currentSave == null)
{
throw new Exception("No save is currently loaded");
}

// if the save is being renamed, sync it immediately
if (!string.IsNullOrEmpty(newName))
{
return await UpdateSave(currentSave.id, newName);
}

// else, update the save locally and queue it for syncing
currentSave.content = contentManager.Content;
DebounceUpdate();
return currentSave;
}

public async Task<GameSave> UpdateSave(int saveId, string newName = "")
Expand Down
9 changes: 4 additions & 5 deletions Assets/Talo Game Services/Talo/Runtime/Entities/Player.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
using UnityEngine;
using System.Linq;
using System;
using System.Threading.Tasks;

namespace TaloGameServices
{
Expand All @@ -17,23 +16,23 @@ public override string ToString()
return JsonUtility.ToJson(this);
}

public async Task SetProp(string key, string value, bool update = true)
public void SetProp(string key, string value, bool update = true)
{
base.SetProp(key, value);

if (update)
{
await Talo.Players.Update();
Talo.Players.DebounceUpdate();
}
}

public async Task DeleteProp(string key, bool update = true)
public void DeleteProp(string key, bool update = true)
{
base.DeleteProp(key);

if (update)
{
await Talo.Players.Update();
Talo.Players.DebounceUpdate();
}
}

Expand Down
Loading