Skip duplicate activity events with same file and cursor

[AnnatarHe]

Add duplicate detection to avoid sending redundant heartbeats when the file path and cursor position haven't changed. This reduces unnecessary socket traffic when the user hasn't made any actual changes.

• Track last activity state (file path, line number, cursor position)
• Skip events where all three values match the previous event
• Write events are never skipped (always sent to socket)
• clear_cache() now resets duplicate detection state

[gemini-code-assist]

Summary of Changes

Hello @AnnatarHe, I'm Gemini Code Assist¹! 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 new feature to optimize network traffic by preventing the sending of redundant heartbeat events. It achieves this by tracking the user's last known activity state (file, line, and cursor position) and skipping subsequent heartbeat events if these parameters have not changed. This significantly reduces unnecessary socket communication, improving efficiency and responsiveness.

Highlights

• Duplicate Event Detection: Introduced a mechanism to detect and skip duplicate activity events, preventing redundant heartbeats from being sent.
• Activity State Tracking: Implemented tracking of the last active file path, line number, and cursor position to identify unchanged user activity.
• Write Event Exemption: Ensured that write events are never considered duplicates and are always sent, preserving critical data updates.
• Cache Reset Enhancement: Updated the clear_cache() function to also reset the duplicate detection state, ensuring a clean slate when needed.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a valuable optimization by adding duplicate detection for activity events to reduce redundant heartbeats. The implementation is well-structured, but I've identified a significant logic issue in the event handling flow that could cause incorrect duplicate detection when interacting with the existing debounce mechanism. Additionally, I've provided suggestions to improve the new test case to ensure it properly verifies the intended behavior and a couple of refactoring opportunities to enhance code maintainability and readability.

[gemini-code-assist]

There's a logical flaw in the order of operations here. update_last_activity is called after the should_send_heartbeat debounce check. If an activity is new (not a duplicate) but gets debounced, last_activity is not updated. Consequently, subsequent activities will be compared against an outdated state, potentially leading to incorrect duplicate detection. The last activity state should be updated for any non-duplicate event, regardless of whether it's ultimately sent or debounced.

To fix this, the last_activity state should be updated immediately after an event is determined not to be a duplicate.

  if is_duplicate_activity(file_path, line_number, cursor_position, is_write) then
    return
  end

  -- Update last activity state since it's not a duplicate
  update_last_activity(file_path, line_number, cursor_position)

  if not should_send_heartbeat(file_path, is_write) then
    return
  end

[gemini-code-assist]

The conditional logic to check for duplicate activity can be simplified. The pattern if condition then return true end return false is more verbose than necessary and can be replaced by directly returning the result of the boolean expression. This makes the code more concise and idiomatic.

  return last_activity.file_path == file_path
    and last_activity.line_number == line_number
    and last_activity.cursor_position == cursor_position

[gemini-code-assist]

The table structure used to reset last_activity is a duplication of its initial definition near the top of the file. This is a maintainability concern, as a change to the structure in one place would require a manual update in the other to avoid bugs. To keep the code DRY (Don't Repeat Yourself), consider defining this structure once in a local variable and reusing it for both initialization and reset.

[gemini-code-assist]

This test case is intended to verify that clear_cache resets the duplicate detection state, but it doesn't actually test this behavior. It only asserts that the pending heartbeat count is zero after calling clear_cache(), which is an expected side effect but doesn't confirm the core logic.

A more effective test would:

1. Trigger an event to populate the last_activity state.
2. Trigger the exact same event and assert that it's skipped (i.e., pending count does not increase).
3. Call heartbeat.clear_cache().
4. Trigger the same event again and assert that it is not skipped this time (i.e., pending count increases by one).

This would properly confirm that the duplicate detection state was cleared and the functionality works as intended.

[claude]

Code Review for PR #10: Skip duplicate activity events with same file and cursor

Summary

This PR adds duplicate detection to reduce redundant heartbeats when file path and cursor position haven't changed. The implementation is generally solid and follows the project's conventions well.

---

✅ Strengths

1. Well-aligned with project goals: Reduces unnecessary socket traffic, which aligns with efficient resource usage
2. Follows conventions: Uses snake_case, local variables, proper annotations, and 2-space indentation per CLAUDE.md
3. Maintains existing guarantees: Write events are correctly never skipped
4. Clear documentation: Good LuaDoc annotations explaining parameters and return values
5. Proper state management: clear_cache() correctly resets both debounce and duplicate detection state

---

🔍 Issues & Suggestions

1. Logic Order Issue (Critical)

Location: lua/shelltime/heartbeat.lua:194-201

The duplicate detection happens before the debounce check, which creates a logical inconsistency:

-- Skip duplicate events (same file and cursor position)
if is_duplicate_activity(file_path, line_number, cursor_position, is_write) then
  return
end

if not should_send_heartbeat(file_path, is_write) then
  return
end

-- Update last activity state
update_last_activity(file_path, line_number, cursor_position)

Problem: If you move your cursor at positions (10,5) → (10,6) → (10,5) rapidly (within debounce window), the second move to (10,5) will be incorrectly skipped as a duplicate even though no heartbeat was actually sent for it.

Recommendation: The duplicate check should only apply after debounce passes OR track both states separately. Consider:

-- Check debounce first
if not should_send_heartbeat(file_path, is_write) then
  return
end

-- Then check for duplicates (only for events that passed debounce)
if is_duplicate_activity(file_path, line_number, cursor_position, is_write) then
  return
end

-- Update last activity state
update_last_activity(file_path, line_number, cursor_position)

---

2. Edge Case: Multiple Buffers

Location: lua/shelltime/heartbeat.lua:119-122

The last_activity state is global, not per-buffer:

local last_activity = {
  file_path = nil,
  line_number = nil,
  cursor_position = nil,
}

Scenario: User switches between fileA.lua and fileB.lua, both at line 10, column 5:

1. Switch to fileA.lua:10:5 → heartbeat sent, state updated
2. Switch to fileB.lua:10:5 → incorrectly skipped as duplicate!

Recommendation: The current implementation is actually correct because file_path is part of the check. However, the variable name last_activity (singular) is misleading. Consider renaming to better reflect it's tracking the last global activity across all buffers, or add a comment explaining this is intentional global state.

---

3. Test Coverage Insufficient

Location: tests/heartbeat_spec.lua:146-151

The new test doesn't actually verify duplicate detection behavior:

it('should reset last activity state for duplicate detection', function()
  -- clear_cache should reset both debounce and duplicate tracking
  heartbeat.clear_cache()
  -- After clearing, the next event should not be considered duplicate
  assert.equals(0, heartbeat.get_pending_count())
end)

Issues:

• Doesn't trigger any events to test duplicate detection
• Just checks pending count is 0 (which it always would be)
• Doesn't verify that duplicates are actually skipped
• Doesn't verify that non-duplicates are sent

Recommendation: Add comprehensive tests:

describe('duplicate detection', function()
  it('should skip duplicate cursor events', function()
    -- Setup: trigger event at same position twice
    -- Assert: only 1 heartbeat queued
  end)
  
  it('should send heartbeat when cursor position changes', function()
    -- Setup: trigger events at different positions
    -- Assert: heartbeats sent for both
  end)
  
  it('should always send write events regardless of position', function()
    -- Setup: trigger write event at same position twice
    -- Assert: both heartbeats sent (is_write bypasses duplicate check)
  end)
  
  it('should send heartbeat when file changes', function()
    -- Setup: trigger events for different files at same cursor position
    -- Assert: heartbeats sent for both files
  end)
end)

---

4. Performance: Redundant Cursor Fetch

Location: lua/shelltime/heartbeat.lua:135,190

The cursor position is fetched twice:

-- In on_event() at line 190
local cursor = vim.api.nvim_win_get_cursor(0)

-- In create_heartbeat() at line 135
local cursor = vim.api.nvim_win_get_cursor(0)

Recommendation: Pass cursor position to create_heartbeat() to avoid the second API call:

local function create_heartbeat(bufnr, is_write, cursor)
  -- Use passed cursor instead of fetching again
end

This is a minor optimization but aligns with the PR's goal of reducing unnecessary operations.

---

5. Documentation Gap

The PR description and code comments don't explain why duplicate detection is needed in addition to debouncing. Consider adding a comment explaining the use case:

-- Duplicate detection: Skip events with identical file/cursor even within debounce window.
-- This handles rapid autocmd triggers (e.g., CursorMoved + TextChanged firing together)
-- that debouncing alone doesn't catch.

---

📊 Security & Architecture

• Security: No concerns. Input validation handled by existing is_valid_buffer()
• Performance: Minor improvement from reducing socket traffic. See note about redundant cursor fetch above
• Architecture: Fits well into existing debounce + queue pattern

---

🎯 Summary Recommendations

Must Fix:

1. Reorder logic: debounce check before duplicate check (prevents incorrect skipping)
2. Add real tests that verify duplicate detection behavior

Should Fix:
3. Add explanatory comment about why duplicate detection is needed
4. Eliminate redundant cursor position fetch

Nice to Have:
5. Clarify that last_activity is intentionally global (or rename for clarity)

---

Overall this is a useful optimization, but the logic ordering issue should be addressed before merging. Great job following the project conventions! 🎉

[claude]

Code Review

Thank you for this PR! The duplicate detection feature is a good optimization to reduce unnecessary socket traffic. Here's my detailed review:

✅ Strengths

1. Clear Intent: The PR addresses a specific performance issue - avoiding redundant heartbeats when nothing has changed
2. Proper Scoping: Write events are correctly excluded from duplicate detection (lines 100-102)
3. Code Style: Follows the project's conventions (snake_case, local variables, LuaDoc annotations)
4. Cache Management: clear_cache() properly resets the new last_activity state (lines 278-282)

🐛 Critical Issue: Logic Bug

Problem: The duplicate detection logic has a race condition with the debounce logic that will cause it to fail to detect duplicates in most cases.

Root Cause (lua/shelltime/heartbeat.lua:194-204):

if not should_send_heartbeat(file_path, is_write) then
  return  -- Line 195: Returns early, never reaches duplicate check
end

-- Skip duplicate events (same file and cursor position)
if is_duplicate_activity(file_path, line_number, cursor_position, is_write) then
  return
end

-- Update last activity state
update_last_activity(file_path, line_number, cursor_position)

Why This Fails: When a duplicate event occurs within the debounce window (30s), should_send_heartbeat() returns false at line 194, causing an early return at line 195. The duplicate detection code at lines 199-204 is never reached.

Example Scenario:

1. User opens file.lua, cursor at line 10, column 5 → heartbeat sent
2. 5 seconds later: User still at same position → should_send_heartbeat() returns false (debounce), early return at line 195
3. 35 seconds later: User still at same position → should_send_heartbeat() returns true (debounce passed), duplicate check runs
4. Bug: last_activity was never updated in step 2, so it still matches → duplicate detected and skipped
5. Result: No heartbeat sent for 35 seconds of activity, even though debounce period passed

🔧 Recommended Fix

The duplicate check should happen before the debounce check, or the two mechanisms should be integrated:

Option 1: Check duplicates first (simpler, recommended)

-- Check duplicate before debounce
if is_duplicate_activity(file_path, line_number, cursor_position, is_write) then
  return
end

if not should_send_heartbeat(file_path, is_write) then
  return
end

update_last_activity(file_path, line_number, cursor_position)

Option 2: Update last_activity on every call (more comprehensive)

-- Check if truly duplicate
local is_duplicate = is_duplicate_activity(file_path, line_number, cursor_position, is_write)

-- Always update last_activity (even if we don't send)
update_last_activity(file_path, line_number, cursor_position)

if is_duplicate then
  return
end

if not should_send_heartbeat(file_path, is_write) then
  return
end

🧪 Test Coverage Issues

Missing Test Cases (tests/heartbeat_spec.lua:146-151):

The new test doesn't actually verify duplicate detection behavior:

it('should reset last activity state for duplicate detection', function()
  heartbeat.clear_cache()
  assert.equals(0, heartbeat.get_pending_count())  -- Only checks count is 0
end)

Recommended Tests:

describe('duplicate detection', function()
  it('should skip duplicate events with same file and cursor', function()
    -- Setup: mock buffer with file and cursor position
    -- Trigger event twice with same position
    -- Assert: only 1 heartbeat created
  end)

  it('should send heartbeat when cursor position changes', function()
    -- Trigger event at position 1
    -- Trigger event at position 2
    -- Assert: 2 heartbeats created
  end)

  it('should send heartbeat when file changes', function()
    -- Trigger event for file1.lua
    -- Trigger event for file2.lua (same cursor)
    -- Assert: 2 heartbeats created
  end)

  it('should never skip write events', function()
    -- Trigger write event twice with same position
    -- Assert: 2 heartbeats created
  end)
end)

⚡ Performance Considerations

1. Cursor API calls: Lines 190-192 call nvim_win_get_cursor(0) on every event, even when events are filtered out early. Consider moving this after buffer validation
2. Duplicate cursor call: create_heartbeat() also calls nvim_win_get_cursor(0) at line 135, resulting in duplicate API calls

💡 Minor Suggestions

1. Line 189-192: Consider extracting cursor earlier to avoid duplicate API calls between on_event and create_heartbeat
2. Documentation: Add a comment explaining the interaction between debounce and duplicate detection
3. Naming: Consider renaming is_duplicate_activity to is_duplicate_cursor_position for clarity

📋 Summary

Action Required:

• ❌ Fix the logic ordering bug (duplicate check after debounce check)
• ❌ Add proper integration tests for duplicate detection
• ⚠️ Consider performance optimization for cursor API calls

The concept is solid, but the current implementation has a critical bug that prevents it from working as intended. Please address the logic issue and add comprehensive tests before merging.

---

Review generated by Claude Code

[claude]

Pull Request Review: Skip duplicate activity events with same file and cursor

Overview

This PR adds duplicate detection to avoid sending redundant heartbeats when the file path and cursor position haven't changed. Overall, the implementation is solid and follows the project's coding conventions well.

Code Quality ✅

Strengths:

• Follows snake_case naming conventions (lua/shelltime/heartbeat.lua:98, 118)
• Proper use of local for all variables and functions
• Single quotes for strings throughout
• Consistent 2-space indentation
• Good LuaDoc annotations with types
• Clear, descriptive function names

Style Adherence:

• Matches existing code patterns in the module
• Follows the architecture documented in CLAUDE.md
• Proper separation of concerns

Potential Issues ⚠️

1. Duplicate Detection Logic Flow (lua/shelltime/heartbeat.lua:199-201)

The current implementation updates last_activity state BEFORE the debounce check. This means:

-- Skip duplicate events (same file and cursor position)
if is_duplicate_activity(file_path, line_number, cursor_position, is_write) then
  return
end

-- Update last activity state immediately after duplicate check
-- This ensures we track the latest position even if debounce blocks sending
update_last_activity(file_path, line_number, cursor_position)

if not should_send_heartbeat(file_path, is_write) then
  return
end

Issue: If a user moves the cursor to a new position but is blocked by debounce, the last_activity is updated, but no heartbeat is sent. If the user then moves back to the original position, it will be considered a duplicate and skipped. This could lead to missing legitimate activity.

Example scenario:

1. User at line 10, col 5 → heartbeat sent
2. 10 seconds later: User moves to line 20, col 10 → blocked by debounce (30s), but last_activity updated
3. User moves back to line 10, col 5 → considered duplicate, skipped
4. Result: The movement to line 20 was never recorded

Recommendation: Consider updating last_activity only when a heartbeat is actually added to the queue:

if is_duplicate_activity(file_path, line_number, cursor_position, is_write) then
  return
end

if not should_send_heartbeat(file_path, is_write) then
  return
end

local heartbeat = create_heartbeat(bufnr, is_write)
if heartbeat then
  add_heartbeat(heartbeat)
  update_last_activity(file_path, line_number, cursor_position)
end

2. Cross-File Duplicate Detection (lua/shelltime/heartbeat.lua:105-108)

The duplicate detection is global across all files. If a user switches between two files and happens to have the cursor at the same position in both, the second file's event will be incorrectly marked as duplicate.

Example scenario:

1. User in fileA.lua at line 10, col 5 → heartbeat sent
2. User switches to fileB.lua at line 10, col 5 → incorrectly marked as duplicate!

Why this happens: The check is:

if last_activity.file_path == file_path
  and last_activity.line_number == line_number
  and last_activity.cursor_position == cursor_position then
  return true
end

But if last_activity.file_path is fileA.lua and the current file_path is fileB.lua, they won't match, so this should work correctly.

Resolution: Actually, this is fine! The file_path check protects against cross-file false positives. No issue here.

3. Test Coverage (tests/heartbeat_spec.lua:146-151)

The new test is very minimal and doesn't actually verify the duplicate detection behavior:

it('should reset last activity state for duplicate detection', function()
  heartbeat.clear_cache()
  assert.equals(0, heartbeat.get_pending_count())
end)

This test only checks that the module doesn't crash when clearing cache. It doesn't verify:

• That duplicates are actually detected and skipped
• That non-duplicates are still processed
• That write events are never considered duplicates
• That the state is properly reset

Recommendation: Add comprehensive tests for the new functionality:

describe('duplicate detection', function()
  it('should skip duplicate events with same file and cursor position', function()
    -- Setup mocks for vim APIs
    -- Trigger event at line 10, col 5
    -- Verify heartbeat is sent
    -- Trigger same event again immediately
    -- Verify duplicate is skipped
  end)

  it('should not skip events when cursor moves', function()
    -- Trigger event at line 10, col 5
    -- Trigger event at line 20, col 10
    -- Verify both are processed (may be debounced, but not duplicate-skipped)
  end)

  it('should never skip write events as duplicates', function()
    -- Trigger write event at same position twice
    -- Verify both are sent (write events bypass duplicate check)
  end)

  it('should reset duplicate state on clear_cache', function()
    -- Trigger event
    -- Clear cache
    -- Trigger same event again
    -- Verify it's not considered duplicate after clear
  end)
end)

Performance Considerations ✅

Good:

• O(1) duplicate detection (simple comparison)
• No memory leaks (fixed-size state)
• Early returns prevent unnecessary processing

Consider:

• The duplicate detection adds minimal overhead (3 comparisons)
• No performance concerns

Security Concerns ✅

No issues identified:

• No external input validation needed
• No SQL/command injection vectors
• No sensitive data exposure
• File path comparisons are safe

Recommendations Summary

High Priority:

1. Fix the state update timing to avoid the debounce interaction issue described above
2. Add comprehensive tests for the duplicate detection logic

Medium Priority:

3. Consider adding a debug log when duplicates are skipped (if config.get('debug'))

Low Priority:

4. Document the interaction between duplicate detection and debouncing in code comments

Testing Checklist

• Tests verify duplicates are actually skipped
• Tests verify non-duplicates are processed
• Tests verify write events bypass duplicate check
• Tests verify state reset works correctly
• Integration tests with actual vim autocmd simulation

Conclusion

The PR is well-implemented and follows project conventions, but has a subtle logic issue with state update timing and needs more comprehensive test coverage. The duplicate detection feature is a good optimization for reducing unnecessary socket traffic. With the recommended fixes, this will be ready to merge.

Verdict: Request changes for the state update timing issue and test coverage.

---

Review generated by Claude Code following CLAUDE.md conventions

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.