Skip to content

Move pause_control and paused_since from GobanBase to GobanEngineConfig#217

Merged
anoek merged 1 commit intoonline-go:mainfrom
lazerwalker:pause_control
Dec 28, 2025
Merged

Move pause_control and paused_since from GobanBase to GobanEngineConfig#217
anoek merged 1 commit intoonline-go:mainfrom
lazerwalker:pause_control

Conversation

@lazerwalker
Copy link
Contributor

See online-go/online-go.com#3010.

These keys are both included in WS game/:id/gamedata messages, which says to me they belong in GobanEngineConfig rather than GobanBase. pause_control is necessary for a functioning implementation of pausing in a third-party client, so it's awkward that the GobanEngineConfig definition doesn't include it. Fully possible I don't understand the convention here, but this seems like common sense to me?

paused_since being unusable is a bit awkward (it currently reflects "when was the last time the game began a pause, regardless of whether it's currently paused"), but I thnk adding a comment about it to ward off folks is probably warranted. Open to tweaking the wording.

Thanks!

@github-actions
Copy link

Code Review

I've analyzed this PR and found one minor style issue:

Style Issue - Missing Semicolon

In src/engine/GobanEngine.ts at line 167 (new code):

pause_control?: AdHocPauseControl

This line is missing a trailing semicolon. All other properties in the GobanEngineConfig interface use semicolons (see lines 97-165 for examples). For consistency with the codebase style, this should be:

pause_control?: AdHocPauseControl;

Overall Assessment

The refactoring logic is sound:

  • ✅ Moving these fields from GobanConfig to GobanEngineConfig is safe since GobanConfig extends GobanEngineConfig
  • ✅ The AdHocPauseControl import is correctly added
  • ✅ All existing code that accesses these fields via config.pause_control and config.paused_since will continue to work through interface inheritance
  • ✅ The comment explaining paused_since behavior is helpful

@github-actions
Copy link

I've reviewed this PR for bugs and performance issues. The code changes look good - no issues found.

This is a clean refactoring that correctly moves pause_control and paused_since from GobanConfig to GobanEngineConfig. Since GobanConfig extends GobanEngineConfig, the change is fully backward compatible. The move makes sense architecturally as these properties represent game state (received via WebSocket messages) rather than UI-specific configuration.

The import of AdHocPauseControl is correctly added, and the helpful comment about paused_since semantics is a nice touch.

@lazerwalker
Copy link
Contributor Author

Rebased and force-pushed the missing semicolon (linter error) and removing the AdHocPauseControl import from GobanBase (test failure).

Sorry to make you work thrice, Claude 😅

@github-actions
Copy link

Code Review

I've analyzed this PR for bugs and performance issues. The changes look good - no issues found.

Summary:

  • The refactoring correctly moves pause_control and paused_since from GobanConfig to GobanEngineConfig
  • This aligns with the server protocol where these properties are sent via game/:id/gamedata messages (typed as GobanEngineConfig)
  • Type safety is maintained through interface inheritance (GobanConfig extends GobanEngineConfig)
  • No breaking changes introduced - all existing code paths remain functional
  • The helpful comment about paused_since behavior is a nice addition

The PR successfully fixes a type system inconsistency and improves third-party client compatibility. ✅

@anoek
Copy link
Member

anoek commented Dec 28, 2025

Yep I think it's better there, thanks!

@anoek anoek merged commit b234987 into online-go:main Dec 28, 2025
2 checks passed
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.

2 participants