feat(daemon): add coding heartbeat tracking with offline persistence#154
feat(daemon): add coding heartbeat tracking with offline persistence#154
Conversation
Add a new socket handler type for coding activity heartbeats (like wakatime). When heartbeats fail to send to the server, they are saved locally and retried every 30 minutes by a background resync service. Changes: - Add HeartbeatPayload/HeartbeatData types mirroring server API - Add SendHeartbeatsToServer() API function - Add SocketMessageTypeHeartbeat handler in daemon - Add HeartbeatResyncService for periodic retry of failed heartbeats - Add CodeTracking config struct (codeTracking.enabled) - Add HEARTBEAT_LOG_FILE path variable Feature is gated by codeTracking.enabled config option. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @AnnatarHe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature to the daemon: coding activity heartbeat tracking with robust offline persistence. The primary goal is to reliably capture and report user coding activity, even in scenarios where the server is temporarily unreachable. It achieves this by defining a new heartbeat data model, establishing an API for sending these heartbeats, and implementing a local storage and retry mechanism to ensure data integrity. The feature is opt-in via a configuration setting, providing users with control over its activation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable new feature for tracking coding activity via heartbeats, complete with offline persistence and a background resync service. The implementation is well-structured. My review focuses on ensuring data integrity, improving performance, and increasing maintainability. I've identified a critical race condition in the resync logic that could lead to data loss, as well as a bug where server-side failures might not be handled correctly. Additionally, there are several opportunities to improve performance by avoiding redundant file reads and inefficient data handling, and to enhance code clarity by reducing duplication. Please see the detailed comments for specific suggestions.
| } | ||
|
|
||
| // resync reads failed heartbeats from the log file and attempts to send them | ||
| func (s *HeartbeatResyncService) resync(ctx context.Context) { |
There was a problem hiding this comment.
There is a critical race condition in the resync logic that can lead to data loss. The current implementation reads the entire log file, processes it in memory, and then overwrites the original file. If a new failed heartbeat is written to the log file while processing is underway, it will be deleted when the file is overwritten.
To fix this, you should make the file handling atomic. A common pattern is:
- Atomically rename the log file (e.g.,
heartbeat.logtoheartbeat.log.processing). - New failed heartbeats can now be safely written to a new (and empty)
heartbeat.log. - Process the contents of
heartbeat.log.processing. - If any heartbeats from the processing file fail again, append them to the new
heartbeat.logfile. - Delete
heartbeat.log.processingonce you're done with it.
| cfg, err := stConfig.ReadConfigFile(ctx) | ||
| if err != nil { | ||
| slog.Error("Failed to read config file", slog.Any("err", err)) | ||
| return err | ||
| } |
There was a problem hiding this comment.
This function calls stConfig.ReadConfigFile(ctx) on every invocation. Since ReadConfigFile reads and parses the configuration from disk each time, this is inefficient and will degrade performance under load from frequent heartbeats.
The configuration is already loaded in main.go. It should be passed down to the message processor and its handlers instead of being re-read from disk. Consider refactoring SocketTopicProccessor to accept the model.ShellTimeConfig object and pass it to this handler.
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The function assumes a successful API call if the HTTP request itself doesn't error. However, the server can return a 200 OK status but indicate a logical failure in the response body (e.g., success: false). These failures are not being handled, which could lead to data loss as the client would incorrectly assume the heartbeats were processed and not save them for retry.
You should inspect the response struct and return an error if the server-side operation was not successful.
// You will need to add "fmt" and "errors" to your imports.
if err != nil {
return err
}
if !response.Success {
msg := "heartbeat API request failed"
if response.Message != "" {
msg = fmt.Sprintf("%s: %s", msg, response.Message)
}
return errors.New(msg)
}
return nil| } | ||
|
|
||
| // Start heartbeat resync service if codeTracking is enabled | ||
| if cfg.CodeTracking != nil && cfg.CodeTracking.Enabled != nil && *cfg.CodeTracking.Enabled { |
There was a problem hiding this comment.
The condition cfg.CodeTracking != nil && cfg.CodeTracking.Enabled != nil && *cfg.CodeTracking.Enabled is verbose and is also duplicated in daemon/socket.go (line 118). To improve readability and reduce duplication, consider adding a helper method to the CodeTracking struct.
For example, in model/types.go:
func (c *CodeTracking) IsEnabled() bool {
return c != nil && c.Enabled != nil && *c.Enabled
}Then the check in both places becomes a much cleaner if cfg.CodeTracking.IsEnabled().
| pb, err := json.Marshal(socketMsgPayload) | ||
| if err != nil { | ||
| slog.Error("Failed to marshal the heartbeat payload again for unmarshal", slog.Any("payload", socketMsgPayload)) | ||
| return err | ||
| } | ||
|
|
||
| var heartbeatPayload model.HeartbeatPayload | ||
| err = json.Unmarshal(pb, &heartbeatPayload) | ||
| if err != nil { | ||
| slog.Error("Failed to parse heartbeat payload", slog.Any("payload", socketMsgPayload)) | ||
| return err | ||
| } |
There was a problem hiding this comment.
The current implementation marshals socketMsgPayload (which is a map[string]interface{}) to a JSON byte slice, only to immediately unmarshal it back into a model.HeartbeatPayload struct. This marshal/unmarshal cycle is inefficient.
A more performant approach is to use a library like mapstructure to directly decode the map into your struct.
Example:
import "github.com/mitchellh/mapstructure"
// ...
func handlePubSubHeartbeat(ctx context.Context, socketMsgPayload interface{}) error {
var heartbeatPayload model.HeartbeatPayload
err := mapstructure.Decode(socketMsgPayload, &heartbeatPayload)
if err != nil {
slog.Error("Failed to parse heartbeat payload", slog.Any("payload", socketMsgPayload), slog.Any("err", err))
return err
}
// ... rest of the function
}This avoids the overhead of the unnecessary JSON marshaling step.
|
|
||
| // saveHeartbeatToFile appends a heartbeat payload as a single JSON line to the log file | ||
| func saveHeartbeatToFile(payload model.HeartbeatPayload) error { | ||
| logFilePath := os.ExpandEnv(fmt.Sprintf("%s/%s", "$HOME", model.HEARTBEAT_LOG_FILE)) |
There was a problem hiding this comment.
For better platform compatibility (e.g., on Windows), it's recommended to use filepath.Join to construct file paths instead of string formatting with /. Additionally, os.UserHomeDir() is a more robust way to get the user's home directory than relying on the $HOME environment variable.
Suggested change:
homeDir, err := os.UserHomeDir()
if err != nil {
return fmt.Errorf("failed to get user home dir: %w", err)
}
logFilePath := filepath.Join(homeDir, model.HEARTBEAT_LOG_FILE)This change should also be applied in daemon/heartbeat_resync.go.
|
|
||
| // resync reads failed heartbeats from the log file and attempts to send them | ||
| func (s *HeartbeatResyncService) resync(ctx context.Context) { | ||
| logFilePath := os.ExpandEnv(fmt.Sprintf("%s/%s", "$HOME", model.HEARTBEAT_LOG_FILE)) |
There was a problem hiding this comment.
For better platform compatibility, it's recommended to use filepath.Join to construct file paths instead of string formatting with /. Additionally, os.UserHomeDir() is a more robust way to get the user's home directory than relying on the $HOME environment variable.
Suggested change:
homeDir, err := os.UserHomeDir()
if err != nil {
slog.Error("could not get user home directory", slog.Any("err", err))
return
}
logFilePath := filepath.Join(homeDir, model.HEARTBEAT_LOG_FILE)| var lines []string | ||
| scanner := bufio.NewScanner(file) | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if line != "" { | ||
| lines = append(lines, line) | ||
| } | ||
| } |
There was a problem hiding this comment.
The resync function reads the entire contents of the heartbeat log file into memory. If a user is offline for an extended period, this file could become very large, leading to high memory consumption.
A more memory-efficient approach would be to process the file line-by-line. When combined with the fix for the race condition, you could read from the renamed file (.processing) one line at a time, and write any lines that fail to resync to the new log file directly, without buffering all lines in memory.
| COMMAND_PRE_STORAGE_FILE = COMMAND_STORAGE_FOLDER + "/pre.txt" | ||
| COMMAND_POST_STORAGE_FILE = COMMAND_STORAGE_FOLDER + "/post.txt" | ||
| COMMAND_CURSOR_STORAGE_FILE = COMMAND_STORAGE_FOLDER + "/cursor.txt" | ||
| HEARTBEAT_LOG_FILE = COMMAND_BASE_STORAGE_FOLDER + "/coding-heartbeat.data.log" |
There was a problem hiding this comment.
The HEARTBEAT_LOG_FILE variable is initialized here and then redundantly re-initialized with the same value inside the InitFolder function on line 38. To avoid redundancy and keep initialization logic in one place, you can remove this line and rely solely on the initialization within InitFolder.
Add caching mechanism to ReadConfigFile using functional options pattern. Cache is stored in-memory with RWMutex for thread safety. Use WithSkipCache() option to bypass cache and read from disk. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
1 participant
Summary
/api/v1/heartbeatsendpoint; failed ones are saved locallycodeTracking.enabledconfig optionChanges
HeartbeatPayload,HeartbeatData,HeartbeatResponsetypesSendHeartbeatsToServer()API functionHEARTBEAT_LOG_FILEpath variableCodeTrackingconfig structSocketMessageTypeHeartbeathandlercodeTracking.enabledConfig Example
Test plan
go build ./...)go test ./daemon/... ./model/...)🤖 Generated with Claude Code