[issues/552] Claude Code cold-start re-focus loop#567
Merged
Conversation
## Summary The first paste after binding to Claude Code fires before the chat panel's webview IPC clipboard reader is ready, silently losing the link. This adds a cold-start re-focus loop that re-sends focus commands at a configurable interval throughout a configurable window, giving the panel time to initialize before paste dispatch. Subsequent pastes use the existing 200ms warm-path delay. ## Changes - `ColdRefocusConfig` value object (`totalMs`, `intervalMs`) — shared by all webview-based AI assistants (Gemini #529 reuses it) - `AIAssistantFocusCapability.refocusDuring()` — loops focus-command sends at `intervalMs` ticks for up to `totalMs` on cold start; `panelIsWarm` flag ensures the loop only fires once per binding session - `getColdRefocus` hook on `BuiltinAiAssistantDef` — reads two new VS Code settings, validates `totalMs > intervalMs`, falls back to defaults with a warning when invalid - `ConfigReader` on `DestinationBuilderContext` — destination builders can now resolve settings at bind time without threading `ConfigReader` through layers - Two new settings: `rangelink.destinations.claudeCode.coldStartDelayMs` (default 2500, range 500–15000) and `coldRefocusIntervalMs` (default 300, range 100–5000) - Documentation: CHANGELOG added; README updated with settings table (`<sup>Unreleased</sup>` markers) - QA: `claude-code-006` (default config valid) and `claude-code-007` (invalid config fallback), both `automated: true` ## Test Plan - [x] All 1888 unit tests pass - [x] New unit tests: `AIAssistantFocusCapability.test.ts` (10 tests — cold start, command fallback, all-fail, warm path, refocus timing, no refocus on warm, elapsed logging, invalid config fallback) - [x] Updated tests: `FocusCapabilityFactory.test.ts`, `destinationBuilders.test.ts`, `DestinationRegistry.test.ts`, `packageJsonContracts.test.ts`, `settingDefaults.test.ts`, `ComposablePasteDestination.integration.test.ts` - [x] Integration tests: `claude-code-006` and `claude-code-007` in `builtInAiAssistants.test.ts` - [x] QA coverage validator passes (123 automated + 115 assisted) Closes #552
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a configurable Claude Code cold-start "refocus" loop: new setting keys/defaults and package.json schema, a ColdRefocusConfig type, AIAssistantFocusCapability cold-start loop, factory and DI wiring, destination builder hook, tests, and docs/QA entries. ChangesClaude Code Cold-Start Re-Focus Implementation
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
New configurable settings for Claude Code cold-start behavior were introduced, which require additional test cases for validation. Suggested test cases:
Generated by QA Gap Check (GPT-4o-mini via GitHub Models) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yaml`:
- Around line 3171-3176: Update the claude-code-007 test case so the fallback
default values are consistent: change the step text that currently says "(1500 /
300)" to "(2500 / 300)" and update any descriptive text to reference the correct
coldStartDelayMs default of 2500; ensure the expected_result remains "Defaults
(2500 / 300) are used as fallback" so both the step (the line mentioning
fallback defaults) and expected_result match the intended default values for
coldStartDelayMs and coldRefocusIntervalMs.
- Line 3155: Update the test case claude-code-006 expectation to use the correct
default coldStartDelayMs value of 2500 (not 1500): locate the YAML entry for
claude-code-006 in qa-test-cases-v1.1.0.yaml and change the step text that
currently reads "Confirm coldStartDelayMs (1500) > coldRefocusIntervalMs (300)"
to "Confirm coldStartDelayMs (2500) > coldRefocusIntervalMs (300)" so it matches
the PR objective rangelink.destinations.claudeCode.coldStartDelayMs — default
2500 (range 500–15000).
In `@packages/rangelink-vscode-extension/README.md`:
- Line 498: The README documents the wrong default for
rangelink.destinations.claudeCode.coldStartDelayMs (shows 1500 but should be
2500); update the table entry for
`rangelink.destinations.claudeCode.coldStartDelayMs` to show the correct default
value `2500` and ensure the accompanying range text remains `500–15000` so the
documentation matches the PR objectives.
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts`:
- Around line 318-323: The test hardcodes numeric defaults (1500 and 300) for
coldStartDelayMs and coldRefocusIntervalMs; import the shared default constants
from the project's defaults/constants module and replace the literal assertions
so totalMs and intervalMs are asserted against those constants (e.g., the
exported default for coldStartDelayMs and coldRefocusIntervalMs) instead of
1500/300; update the test variables totalMs and intervalMs assertions to use the
imported constants and remove the magic numbers.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/AIAssistantFocusCapability.test.ts`:
- Around line 86-88: The test currently asserts the enum constant
FocusErrorReason.COMMAND_FOCUS_FAILED in the Jest expectation; change this to
assert the corresponding string literal for the error code instead (e.g.,
replace FocusErrorReason.COMMAND_FOCUS_FAILED with the exact string value the
enum represents) in the assertion inside AIAssistantFocusCapability.test (the
expect(result).toBeErrWith callback that checks error.reason on
FocusResult['error']). Ensure you only change the assertion to the string
literal while leaving the rest of the expect(result).toBeErrWith((error:
FocusResult['error']) => { ... }) structure intact.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts`:
- Around line 741-743: Tests are reading mock invocation arguments directly via
.mock.calls[0]; update assertions to use Jest matchers instead—e.g., call
expect(context.factories.focusCapability.createAIAssistantCapability).toHaveBeenCalledTimes(n)
and
expect(context.factories.focusCapability.createAIAssistantCapability).toHaveBeenCalledWith(expectedArgs)
(or use toHaveBeenCalledWith plus more specific matchers) rather than indexing
into .mock.calls; apply the same refactor for every occurrence that accesses
createAIAssistantCapability.mock.calls (including the places that assign
getColdRefocusArg and the other three occurrences) so tests assert call count
and parameters explicitly via toHaveBeenCalledWith()/toHaveBeenCalledTimes().
In `@packages/rangelink-vscode-extension/src/constants/settingDefaults.ts`:
- Line 39: Update the constant
DEFAULT_DESTINATIONS_CLAUDE_CODE_COLD_START_DELAY_MS from 1500 to 2500 in the
file (adjust the value in the exported constant) and ensure the corresponding
default value in package.json and any contract tests are updated to match 2500
so the PR contract target remains consistent.
In
`@packages/rangelink-vscode-extension/src/destinations/capabilities/AIAssistantFocusCapability.ts`:
- Around line 63-76: The current loop in AIAssistantFocusCapability treats
positive-but-invalid configs (e.g., refocus.totalMs <= refocus.intervalMs) as
valid and ends up skipping sends; update the initial validation in the method
where refocus is checked (the block that currently tests refocus.totalMs <= 0 ||
refocus.intervalMs <= 0) to also treat refocus.totalMs <= refocus.intervalMs as
invalid: log a warning including totalMs and intervalMs (same logging shape used
now), await the fallback delay (FOCUS_TO_PASTE_DELAY_MS) and return early so
capability-level validation mirrors the builder restriction and avoids silent
first-paste loss.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ebdbbf0-c172-44a5-b75d-720e4ede20ac
📒 Files selected for processing (21)
packages/rangelink-vscode-extension/CHANGELOG.mdpackages/rangelink-vscode-extension/README.mdpackages/rangelink-vscode-extension/package.jsonpackages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yamlpackages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.tspackages/rangelink-vscode-extension/src/__tests__/constants/packageJsonContracts.test.tspackages/rangelink-vscode-extension/src/__tests__/constants/settingDefaults.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/ComposablePasteDestination.integration.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/DestinationRegistry.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/AIAssistantFocusCapability.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/FocusCapabilityFactory.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.tspackages/rangelink-vscode-extension/src/constants/settingDefaults.tspackages/rangelink-vscode-extension/src/constants/settingKeys.tspackages/rangelink-vscode-extension/src/createWiringServices.tspackages/rangelink-vscode-extension/src/destinations/DestinationRegistry.tspackages/rangelink-vscode-extension/src/destinations/capabilities/AIAssistantFocusCapability.tspackages/rangelink-vscode-extension/src/destinations/capabilities/ColdRefocusConfig.tspackages/rangelink-vscode-extension/src/destinations/capabilities/FocusCapabilityFactory.tspackages/rangelink-vscode-extension/src/destinations/capabilities/index.tspackages/rangelink-vscode-extension/src/destinations/destinationBuilders.ts
|
|
||
| | Setting | Default | Description | | ||
| | --------------------------------------------------------- | ------- | ------------------------------------------------------------------------------------ | | ||
| | `rangelink.destinations.claudeCode.coldStartDelayMs` | `1500` | Total duration (ms) of the cold-start re-focus window for Claude Code | |
Contributor
There was a problem hiding this comment.
Incorrect default value documented for coldStartDelayMs.
The table shows 1500 as the default for rangelink.destinations.claudeCode.coldStartDelayMs, but the PR objectives specify the default should be 2500. This discrepancy will mislead users about the actual default behavior.
Per PR objectives: "rangelink.destinations.claudeCode.coldStartDelayMs — default 2500 (range 500–15000)"
🔧 Proposed fix
| Setting | Default | Description |
| --------------------------------------------------------- | ------- | ------------------------------------------------------------------------------------ |
-| `rangelink.destinations.claudeCode.coldStartDelayMs` | `1500` | Total duration (ms) of the cold-start re-focus window for Claude Code |
+| `rangelink.destinations.claudeCode.coldStartDelayMs` | `2500` | Total duration (ms) of the cold-start re-focus window for Claude Code |
| `rangelink.destinations.claudeCode.coldRefocusIntervalMs` | `300` | Interval (ms) between successive focus-command re-sends during the cold-start window |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `rangelink.destinations.claudeCode.coldStartDelayMs` | `1500` | Total duration (ms) of the cold-start re-focus window for Claude Code | | |
| | `rangelink.destinations.claudeCode.coldStartDelayMs` | `2500` | Total duration (ms) of the cold-start re-focus window for Claude Code | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rangelink-vscode-extension/README.md` at line 498, The README
documents the wrong default for
rangelink.destinations.claudeCode.coldStartDelayMs (shows 1500 but should be
2500); update the table entry for
`rangelink.destinations.claudeCode.coldStartDelayMs` to show the correct default
value `2500` and ensure the accompanying range text remains `500–15000` so the
documentation matches the PR objectives.
Comment on lines
+318
to
+323
| const totalMs = config.get<number>('coldStartDelayMs', 1500); | ||
| const intervalMs = config.get<number>('coldRefocusIntervalMs', 300); | ||
|
|
||
| assert.strictEqual(totalMs, 1500, 'Expected default coldStartDelayMs to be 1500'); | ||
| assert.strictEqual(intervalMs, 300, 'Expected default coldRefocusIntervalMs to be 300'); | ||
| assert.ok( |
Contributor
There was a problem hiding this comment.
Default-value assertions look stale and should come from shared defaults constants.
This test currently hardcodes coldStartDelayMs as 1500, but this PR’s objective defines 2500. Hardcoded values here can silently drift from the real contract; import and assert against shared defaults instead.
Proposed fix
+import {
+ CLAUDE_CODE_COLD_START_DELAY_MS,
+ CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS,
+} from '../../constants/settingDefaults';
...
- const totalMs = config.get<number>('coldStartDelayMs', 1500);
- const intervalMs = config.get<number>('coldRefocusIntervalMs', 300);
+ const totalMs = config.get<number>('coldStartDelayMs', CLAUDE_CODE_COLD_START_DELAY_MS);
+ const intervalMs = config.get<number>(
+ 'coldRefocusIntervalMs',
+ CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS,
+ );
- assert.strictEqual(totalMs, 1500, 'Expected default coldStartDelayMs to be 1500');
- assert.strictEqual(intervalMs, 300, 'Expected default coldRefocusIntervalMs to be 300');
+ assert.strictEqual(
+ totalMs,
+ CLAUDE_CODE_COLD_START_DELAY_MS,
+ `Expected default coldStartDelayMs to be ${CLAUDE_CODE_COLD_START_DELAY_MS}`,
+ );
+ assert.strictEqual(
+ intervalMs,
+ CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS,
+ `Expected default coldRefocusIntervalMs to be ${CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS}`,
+ );As per coding guidelines, "Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts`
around lines 318 - 323, The test hardcodes numeric defaults (1500 and 300) for
coldStartDelayMs and coldRefocusIntervalMs; import the shared default constants
from the project's defaults/constants module and replace the literal assertions
so totalMs and intervalMs are asserted against those constants (e.g., the
exported default for coldStartDelayMs and coldRefocusIntervalMs) instead of
1500/300; update the test variables totalMs and intervalMs assertions to use the
imported constants and remove the magic numbers.
Comment on lines
+741
to
+743
| const [, getColdRefocusArg] = ( | ||
| context.factories.focusCapability.createAIAssistantCapability as jest.Mock | ||
| ).mock.calls[0]; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify direct mock call-index access in this test file
rg -n "mock\.calls\[[0-9]+\]" packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.tsRepository: couimet/rangeLink
Length of output: 169
Use toHaveBeenCalledWith() or toHaveBeenCalledTimes() instead of accessing .mock.calls[0] directly.
These tests read mock call arguments through .mock.calls[0], violating the test guideline. Refactor to assert mock parameters explicitly:
Pattern
- const [, getColdRefocusArg] = (
- context.factories.focusCapability.createAIAssistantCapability as jest.Mock
- ).mock.calls[0];
- expect(typeof getColdRefocusArg).toBe('function');
+ const createAiCapabilityMock = context.factories.focusCapability
+ .createAIAssistantCapability as jest.Mock;
+ expect(createAiCapabilityMock).toHaveBeenCalledTimes(1);
+ const [capabilities, coldRefocus] = createAiCapabilityMock.mock.calls[0];
+ expect(capabilities).toStrictEqual(['claude-vscode.focusChat']);
+ expect(typeof coldRefocus).toBe('function');Apply to all occurrences: lines 743, 760, 776, 797.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.ts`
around lines 741 - 743, Tests are reading mock invocation arguments directly via
.mock.calls[0]; update assertions to use Jest matchers instead—e.g., call
expect(context.factories.focusCapability.createAIAssistantCapability).toHaveBeenCalledTimes(n)
and
expect(context.factories.focusCapability.createAIAssistantCapability).toHaveBeenCalledWith(expectedArgs)
(or use toHaveBeenCalledWith plus more specific matchers) rather than indexing
into .mock.calls; apply the same refactor for every occurrence that accesses
createAIAssistantCapability.mock.calls (including the places that assign
getColdRefocusArg and the other three occurrences) so tests assert call count
and parameters explicitly via toHaveBeenCalledWith()/toHaveBeenCalledTimes().
| // ============================================================================= | ||
|
|
||
| export const DEFAULT_DESTINATIONS_CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS = 300; | ||
| export const DEFAULT_DESTINATIONS_CLAUDE_CODE_COLD_START_DELAY_MS = 1500; |
Contributor
There was a problem hiding this comment.
Default cold-start window is below the PR contract target.
Line [39] sets DEFAULT_DESTINATIONS_CLAUDE_CODE_COLD_START_DELAY_MS to 1500, but this PR’s stated default is 2500. This reduces the cold-start refocus window and can reintroduce first-paste misses on slower startups. Please update this to 2500 and keep package.json + contract tests in sync.
Suggested fix
-export const DEFAULT_DESTINATIONS_CLAUDE_CODE_COLD_START_DELAY_MS = 1500;
+export const DEFAULT_DESTINATIONS_CLAUDE_CODE_COLD_START_DELAY_MS = 2500;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const DEFAULT_DESTINATIONS_CLAUDE_CODE_COLD_START_DELAY_MS = 1500; | |
| export const DEFAULT_DESTINATIONS_CLAUDE_CODE_COLD_START_DELAY_MS = 2500; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rangelink-vscode-extension/src/constants/settingDefaults.ts` at line
39, Update the constant DEFAULT_DESTINATIONS_CLAUDE_CODE_COLD_START_DELAY_MS
from 1500 to 2500 in the file (adjust the value in the exported constant) and
ensure the corresponding default value in package.json and any contract tests
are updated to match 2500 so the PR contract target remains consistent.
…e hardcoded values from QA YAML Applied the 4 valid CodeRabbit findings from the review: fixed QA YAML inconsistency where expected_result still referenced 2500 while the step had been updated to 1500 (both now use setting names instead of hardcoded values so they age when defaults change), fixed T003 violation using enum constant in assertion instead of string literal, fixed T006 violation accessing .mock.calls[0] directly instead of Jest matchers, and added totalMs <= intervalMs validation at the capability level so callers that bypass builder validation still get the guard. Lowered coldStartDelayMs default from 2500 to 1500 after real-world testing showed Claude Code launches significantly faster than Gemini — validated across multiple cold-start test runs. Fixed cc-005 warm-send editor focus loss after verdict dialog. Benefits: - No hardcoded numbers in QA labels — future default changes won't create stale YAML - Capability-level guard matches builder-level guard — no silent first-paste loss through bypass paths - Tests survive T003/T006 audit Ignored Feedback: - Revert default to 2500 (3238771995, 3238772004, 3238772013): 1500 was validated across 3 cold-start test runs and is the correct default for Claude Code - Use constants instead of literals in integration test assertions (3238772006): T003 requires literals in assertions to freeze contracts; importing the constant would test the constant equals itself Ref: #567 (review)
Owner
Author
|
@coderabbitai I see #567 (comment) reports being rate-limited. Was this for your last review ? Or did you complete it after I pushed commit e0247f7 ? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 44298
So the rate-limit notice was posted as a precautionary/in-progress message, but the review ran to completion and fully covers your latest commit. No need to re-trigger a review for those changes. |
Owner
Author
|
@coderabbitai I noticed the In order to keep good visibility on those 2 actions, how complicated would it be to have each of them post a new PR comment when they're done running ? In an ideal world, they would list key info like how long they ran, how many tests they ran and which Test case ids (TC IDs) from their associated version QA test cases YAML file (ie packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yaml) they ran from the batch. Can you create a new GitHub issue for me with the above content as a description + your findings and all the implementation details in a comment on this new issue you'll create ? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me first gather information about the CI workflows and the QA test cases YAML file before creating the issue. 🏁 Script executed: Length of output: 525 🏁 Script executed: Length of output: 161123 🏁 Script executed: Length of output: 5548 Now I have all the details needed. Let me create the GitHub issue with findings and implementation details. 🏁 Script executed: Length of output: 251 The issue was created successfully. Now let me fix the comment posting (the shell syntax error was from unescaped parentheses in the body): Tests: 1888 passed, 1888 total 42 passing (3s) Bonus: Cache marketplace extensions to speed up
|
| Area | Effort |
|---|---|
| Duration tracking | Low — $SECONDS bash built-in |
| Test count parsing (Jest) | Low — well-known output format |
Test count parsing (@vscode/test-cli / Mocha) |
Low–Medium — Mocha output is standard |
| TC ID extraction from QA YAML | Low — Python one-liner, PyYAML available on ubuntu-latest |
Comment upsert via gh api |
Low — pattern already exists in qa-gap-check.yml |
| Composite action refactor (optional) | Medium |
| Extension caching bonus | Low |
Overall: Medium — no new dependencies, no external APIs, all patterns already exist in the repo.
ENDOFCOMMENT
gh issue comment 568
--repo couimet/rangeLink
--body-file /tmp/impl-comment.md
echo "Exit code: $?"
---
🏁 Script executed:
#!/bin/bash
cat > /tmp/impl-comment.md << 'ENDOFCOMMENT'
Implementation Details
Overview
The pattern already exists in .github/workflows/qa-gap-check.yml for posting/updating PR comments via gh api. The same approach can be applied to both jobs in ci.yml.
Step 1 — Add permissions and on guard to both jobs
Both jobs need pull-requests: write permission and should only post comments on PRs (not on direct pushes to main):
# In ci.yml, add at the job level for both 'test' and 'test-with-extensions':
permissions:
contents: read
pull-requests: writeThe comment-posting step should be gated on github.event_name == 'pull_request'.
Step 2 — Capture duration in each job
Use $SECONDS (bash built-in) to compute elapsed time. Set a start marker before the first meaningful step and read it after the last step:
- name: Record start time
shell: bash
run: echo "JOB_START=$SECONDS" >> $GITHUB_ENV
# ... all other steps ...
- name: Post CI summary comment
if: github.event_name == 'pull_request'
shell: bash
env:
GH_TOKEN: ${{ github.token }}
run: |
DURATION=$((SECONDS - JOB_START))
MINUTES=$((DURATION / 60))
SECS=$((DURATION % 60))
DURATION_STR="${MINUTES}m ${SECS}s"Alternatively, extract this into a new composite action .github/actions/post-ci-summary/action.yml that accepts start-time, test-count, tc-ids, and job-name as inputs — this keeps ci.yml clean.
Step 3 — Capture test counts
For the run-tests step (Jest unit tests):
Jest outputs a summary line like:
Length of output: 102
---
🏁 Script executed:
```
pnpm test 2>&1 | tee /tmp/jest-output.txt
TESTS_PASSED=$(grep -oP '(?<=Tests:)\s+\K[0-9]+(?= passed)' /tmp/jest-output.txt | tail -1)
TESTS_TOTAL=$(grep -oP '[0-9]+(?= total)' /tmp/jest-output.txt | tail -1)
```
Unrecoverable error: Request timed out after 300000ms (requestId=4bb610f8-267c-475d-af9c-4a1b693d628c)
---
🏁 Script executed:
```
xvfb-run ... pnpm test:release:automated 2>&1 | tee /tmp/integration-output.txt
INT_PASSING=$(grep -oP '(?<=\s)\d+(?= passing)' /tmp/integration-output.txt | tail -1)
```
Length of output: 103
---
🏁 Script executed:
```
python3 - <<'EOF'
import yaml
with open('packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yaml') as f:
data = yaml.safe_load(f)
ids = [
tc['id'] for tc in data['test_cases']
if tc.get('automated') is True
and 'requires-extensions' not in (tc.get('labels') or [])
]
print(', '.join(ids))
EOF
```
Length of output: 2899
---
🏁 Script executed:
```
python3 - <<'EOF'
import yaml
with open('packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yaml') as f:
data = yaml.safe_load(f)
ids = [
tc['id'] for tc in data['test_cases']
if tc.get('automated') is True
and 'requires-extensions' in (tc.get('labels') or [])
]
print(', '.join(ids))
EOF
```
Length of output: 58
---
🏁 Script executed:
```
# Sentinel values (unique per job):
# CI / Test & Validate -> <!-- ci-summary-test -->
# Integration Tests -> <!-- ci-summary-with-extensions -->
SENTINEL="<!-- ci-summary-test -->"
COMMENT_BODY="${SENTINEL}
### CI / Test & Validate — Summary
| | |
|---|---|
| ⏱ Duration | ${DURATION_STR} |
| ✅ Tests passed | ${TESTS_PASSED} / ${TESTS_TOTAL} |
| 🧪 QA TC IDs exercised | ${TC_IDS} |
| 🔗 Run | ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} |
"
EXISTING=$(gh api \
repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/comments \
--jq ".[] | select(.body | contains(\"${SENTINEL}\")) | .id" | head -1)
if [ -n "$EXISTING" ]; then
gh api repos/${{ github.repository }}/issues/comments/$EXISTING \
-X PATCH -f body="$COMMENT_BODY"
else
gh api repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/comments \
-f body="$COMMENT_BODY"
fi
```
Length of output: 575
---
🏁 Script executed:
#!/bin/bash
python3 << 'PYEOF'
content = r"""## Implementation Details
Overview
The same gh api upsert pattern already used in qa-gap-check.yml can be applied to both jobs in ci.yml — no new tools or external APIs are needed.
Root cause of the speed difference (findings)
CI / Test & Validate |
CI / Integration Tests (with extensions) |
|
|---|---|---|
| Script run | pnpm test:release:automated |
pnpm test:release:with-extensions --no-assisted |
| VS Code binary | ✅ Cached (~/.vscode-test) |
✅ Cached (~/.vscode-test) |
| Marketplace extensions | ❌ None downloaded | |
| Extension host init | Fast (no 3rd-party extensions) | Slow (Claude Code must fully initialize) |
| Cold-start delays in tests | None | Yes — e.g. coldStartDelayMs default is 2500 ms per bind cycle |
| Extra steps | Formatting, linting, unit tests (Jest), TODO check | None beyond the integration run itself |
The bottleneck in test-with-extensions is: uncached marketplace extension download + extension host initialization + cold-start timing waits in the test suite itself. The test job does more steps overall but all of them are fast pure-Node operations.
Step 1 — Add permissions to both jobs
# ci.yml — add under each job:
permissions:
contents: read
pull-requests: writeGate the comment step with if: github.event_name == 'pull_request' since both jobs also fire on push to main (no PR number available there).
Step 2 — Capture duration
Use $SECONDS (bash built-in — already available in every ubuntu-latest runner):
- name: Record start time
shell: bash
run: echo "JOB_START=$SECONDS" >> "$GITHUB_ENV"
# ... all existing steps unchanged ...
- name: Post CI summary comment
if: github.event_name == 'pull_request'
env:
GH_TOKEN: ${{ github.token }}
shell: bash
run: |
ELAPSED=$(( SECONDS - JOB_START ))
DURATION_STR="$(( ELAPSED / 60 ))m $(( ELAPSED % 60 ))s"Step 3 — Capture test counts
Jest unit tests — tee output and grep the summary line:
pnpm test 2>&1 | tee /tmp/jest-out.txt
UNIT_PASSED=$(grep -oP 'Tests:\s+\K\d+(?= passed)' /tmp/jest-out.txt | tail -1)
UNIT_TOTAL=$(grep -oP '\d+(?= total)' /tmp/jest-out.txt | tail -1)@vscode/test-cli / Mocha integration tests — same tee approach:
xvfb-run --auto-servernum pnpm test:release:automated 2>&1 | tee /tmp/int-out.txt
INT_PASSING=$(grep -oP '\d+(?= passing)' /tmp/int-out.txt | tail -1)Both of these changes live inside the respective composite action run: blocks (or you can add a new capture-output output to each composite action and consume it in ci.yml).
Step 4 — Identify QA TC IDs exercised per job (dynamic, from YAML)
python3 and PyYAML are available on every ubuntu-latest runner with no installation needed.
CI / Test & Validate — automated: true, no requires-extensions label:
TC_IDS=$(python3 - <<'EOF'
import yaml
with open('packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yaml') as f:
data = yaml.safe_load(f)
ids = [
tc['id'] for tc in data['test_cases']
if tc.get('automated') is True
and 'requires-extensions' not in (tc.get('labels') or [])
]
print(', '.join(ids))
EOF
)CI / Integration Tests (with extensions) — automated: true AND requires-extensions label:
TC_IDS=$(python3 - <<'EOF'
import yaml
with open('packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yaml') as f:
data = yaml.safe_load(f)
ids = [
tc['id'] for tc in data['test_cases']
if tc.get('automated') is True
and 'requires-extensions' in (tc.get('labels') or [])
]
print(', '.join(ids))
EOF
)Current TC IDs (as of qa-test-cases-v1.1.0.yaml, parsed live):
testjob (no extensions):status-bar-menu-002,status-bar-menu-003,status-bar-menu-005,status-bar-menu-006,bind-to-destination-010,bind-to-destination-013,terminal-picker-001… (113 total)test-with-extensionsjob:custom-ai-assistant-003,claude-code-006,claude-code-007
Since the Python snippet parses the YAML at CI time, the list updates automatically as new TCs are added — no manual maintenance.
Step 5 — Post/update the PR comment (upsert pattern from qa-gap-check.yml)
Use different HTML comment sentinels per job so the two comments are independent:
# For 'test' job: SENTINEL="<!-- ci-summary-test -->"
# For 'test-with-ext': SENTINEL="<!-- ci-summary-with-extensions -->"
BODY="${SENTINEL}
### ${JOB_LABEL} — run summary
| | |
|---|---|
| Duration | ${DURATION_STR} |
| Tests passed | ${TESTS_PASSED} / ${TESTS_TOTAL} |
| QA TC IDs exercised | ${TC_IDS} |
| Run | ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID} |
"
EXISTING=$(gh api \
"repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" \
--jq ".[] | select(.body | startswith(\"${SENTINEL}\")) | .id" | head -1)
if [ -n "$EXISTING" ]; then
gh api "repos/${GITHUB_REPOSITORY}/issues/comments/${EXISTING}" -X PATCH -f body="$BODY"
else
gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" -f body="$BODY"
fiNote: use the environment variables GITHUB_REPOSITORY, GITHUB_SERVER_URL, GITHUB_RUN_ID (always available) and pass PR_NUMBER=${{ github.event.pull_request.number }} via env:.
Bonus — Cache marketplace extensions to close the speed gap
The single biggest time saving for test-with-extensions would be caching downloaded VS Code extensions. Only the binary is currently cached; extensions are not. Add this step to .github/actions/run-integration-tests-with-extensions/action.yml before the Xvfb run:
- name: Cache VS Code marketplace extensions
uses: actions/cache@v4
with:
path: ~/.vscode-test/extensions
key: vscode-extensions-${{ runner.os }}-claude-code-stable
restore-keys: vscode-extensions-${{ runner.os }}-This is independent of the comment-posting work and can be shipped as a separate small PR.
Complexity assessment
| Area | Effort |
|---|---|
Duration tracking ($SECONDS) |
Low |
| Test count parsing — Jest | Low |
Test count parsing — Mocha / @vscode/test-cli |
Low–Medium |
| TC ID extraction from QA YAML (Python one-liner) | Low |
Comment upsert via gh api |
Low (pattern exists in qa-gap-check.yml) |
Refactor into a post-ci-summary composite action |
Medium |
| Extension caching bonus | Low |
Overall: Medium — no new dependencies, no external APIs, all patterns already present in the repo.
"""
with open('/tmp/impl-comment.md', 'w') as f:
f.write(content)
print("File written successfully")
PYEOF
gh issue comment 568
--repo couimet/rangeLink
--body-file /tmp/impl-comment.md
echo "Exit code: $?"
Length of output: 111
---
</details>
`@couimet`: Done! GitHub issue [`#568`](https://github.com/couimet/rangeLink/issues/568) has been created with your description and my findings, and the implementation details are posted as a comment on it.
Here's a summary of everything:
---
### Why `CI / Test & Validate` is significantly faster
After reading the workflow and all composite action files, the root causes are clear:
| Factor | `Test & Validate` | `Integration Tests (with extensions)` |
|---|---|---|
| VS Code binary | ✅ Cached | ✅ Cached |
| Marketplace extensions | ❌ None downloaded | ⚠️ **Downloaded every run — not cached** |
| Extension host init | Fast (no 3rd-party exts) | Slow (Claude Code must fully initialize) |
| Cold-start timing in tests | None | Yes — e.g. default `coldStartDelayMs` is 2500 ms |
| Additional steps | Formatting, linting, Jest unit tests, TODO check | Only the integration run itself |
The `test-with-extensions` job does *fewer* steps overall, yet it's slower because: **marketplace extensions (e.g. `anthropic.claude-code`) are re-downloaded and re-installed on every single run** since there's no extension cache — only the VS Code binary is cached. On top of that, the tests themselves include deliberate cold-start timing waits.
---
### How complicated is the comment-posting feature?
**Overall: Medium complexity.** All the building blocks already exist in the repo (`qa-gap-check.yml` has the exact `gh api` upsert pattern). The full implementation plan is in the issue comment, but the key pieces are:
1. **Duration** — trivial via `$SECONDS` bash built-in
2. **Test counts** — tee Jest/Mocha output and grep the summary line
3. **QA TC IDs** — a Python one-liner parses the YAML at CI time (dynamic, no manual maintenance):
- `test` job covers **113 TC IDs** (`automated: true`, no `requires-extensions`)
- `test-with-extensions` covers **3 TC IDs**: `custom-ai-assistant-003`, `claude-code-006`, `claude-code-007`
4. **Comment upsert** — reuse `gh api` + sentinel pattern from `qa-gap-check.yml` with different sentinels per job so the two comments are independent
There's also a **bonus quick win** noted in the comment: caching the downloaded marketplace extensions (`~/.vscode-test/extensions`) would likely shave 1–2 minutes off `test-with-extensions` on cache hits and can be shipped as its own small PR independently of the comment-posting work.
The integration test suite had duplicated cleanup logic across 20 files — each teardown called unbind and closeAllEditors individually. This caused drift: some teardowns missed `await settle()` after unbind, leaving pending promise work that bled into the next suite. When a suite left a terminal bound and the next suite's setup tried to bind, the pending unbind resolved after the new bind, making VS Code see an already-bound state and show a stuck confirmation picker that nobody dismissed. Moving unbind (setup + suiteTeardown) and closeAllEditors (suiteTeardown) into standardSuite makes cleanup the framework's responsibility. Tests no longer opt in to correct behavior — they get it by construction. Also converts the last remaining raw `suite()` to `standardSuite()` (builtInAiAssistants) and fixes claude-code-007 by adding CMD_JUMP_TO_DESTINATION to trigger the focus path where cold-refocus validation warnings live. Benefits: - Single source of truth for test lifecycle cleanup — impossible for individual suites to forget or mis-order operations - Eliminates the class of bugs where missing `await settle()` after state mutations causes cross-suite contamination - 91 lines of duplicated boilerplate removed; each test file's teardown now only manages its own fixtures
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts (1)
306-319:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDefault value assertions should use shared constants instead of magic numbers.
The test hardcodes
1500and300in both theconfig.getfallback and the assertion. If defaults change, this requires updates in multiple locations.Import shared default constants (e.g.,
CLAUDE_CODE_COLD_START_DELAY_MS,CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS) and use them throughout. This ensures the test stays in sync with the actual defaults.♻️ Recommended pattern
+import { + CLAUDE_CODE_COLD_START_DELAY_MS, + CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS, +} from '../../constants/settingDefaults'; ... -const totalMs = config.get<number>('coldStartDelayMs', 1500); -const intervalMs = config.get<number>('coldRefocusIntervalMs', 300); +const totalMs = config.get<number>('coldStartDelayMs', CLAUDE_CODE_COLD_START_DELAY_MS); +const intervalMs = config.get<number>('coldRefocusIntervalMs', CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS); -assert.strictEqual(totalMs, 1500, 'Expected default coldStartDelayMs to be 1500'); -assert.strictEqual(intervalMs, 300, 'Expected default coldRefocusIntervalMs to be 300'); +assert.strictEqual(totalMs, CLAUDE_CODE_COLD_START_DELAY_MS, `Expected default coldStartDelayMs to be ${CLAUDE_CODE_COLD_START_DELAY_MS}`); +assert.strictEqual(intervalMs, CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS, `Expected default coldRefocusIntervalMs to be ${CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS}`);As per coding guidelines, "Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts` around lines 306 - 319, Replace the magic numbers in the test "claude-code-006: Cold-start default settings produce correct ColdRefocusConfig" with the shared constants: import CLAUDE_CODE_COLD_START_DELAY_MS and CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS at the top of the test file, use them as the fallback values in config.get (instead of 1500 and 300) and in the assert.strictEqual checks, and update the assertion message and the totalMs > intervalMs comparison to reference those constants so the test remains in sync with the real defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts`:
- Around line 306-319: Replace the magic numbers in the test "claude-code-006:
Cold-start default settings produce correct ColdRefocusConfig" with the shared
constants: import CLAUDE_CODE_COLD_START_DELAY_MS and
CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS at the top of the test file, use them as
the fallback values in config.get (instead of 1500 and 300) and in the
assert.strictEqual checks, and update the assertion message and the totalMs >
intervalMs comparison to reference those constants so the test remains in sync
with the real defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be229fd0-6189-4142-a4b6-782aac9383f0
📒 Files selected for processing (24)
packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yamlpackages/rangelink-vscode-extension/src/__integration-tests__/helpers/standardSuite.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/bindToDestination.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/clipboardPreservation.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuEditorContent.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuEditorTab.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuExplorer.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/contextMenuTerminal.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/coreSendCommands.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/customAiAssistants.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/dirtyBufferWarning.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/editorBindingValidation.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/filePicker.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/goToRangeLink.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/linkGeneration.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/sendFilePath.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/smartPadding.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/statusBarMenu.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/terminalPicker.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/textEditorDestination.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/capabilities/AIAssistantFocusCapability.test.tspackages/rangelink-vscode-extension/src/__tests__/destinations/destinationBuilders.test.tspackages/rangelink-vscode-extension/src/destinations/capabilities/AIAssistantFocusCapability.ts
💤 Files with no reviewable changes (15)
- packages/rangelink-vscode-extension/src/integration-tests/suite/linkGeneration.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/contextMenuEditorContent.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/terminalPicker.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/customAiAssistants.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/textEditorDestination.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/bindToDestination.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/statusBarMenu.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/contextMenuTerminal.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/sendFilePath.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/smartPadding.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/clipboardPreservation.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/goToRangeLink.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/filePicker.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/coreSendCommands.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/dirtyBufferWarning.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rangelink-vscode-extension/qa/qa-test-cases-v1.1.0.yaml
- packages/rangelink-vscode-extension/src/destinations/capabilities/AIAssistantFocusCapability.ts
- packages/rangelink-vscode-extension/src/tests/destinations/capabilities/AIAssistantFocusCapability.test.ts
…amination The centralization commit moved closeAllEditors from individual teardown hooks (afterEach) into suiteTeardown. But Mocha's suiteTeardown runs once after all tests, while teardown ran after each test. Editors from test N survived into test N+1, causing cross-test file pollution in pickers and stale editor timeouts in tab-group tests.
… of it for every test
standardSuite.setup now calls closeAllEditors and CMD_UNBIND_DESTINATION before each test. Two suites create shared state in suiteSetup — an untitled document and a bound capturing terminal — that setup destroys. The tests then fail because the document is closed (no selection change event fires) or no destination is bound (send command blocks on picker). Add per-test setup hooks that restore the shared state after standardSuite.setup clears it: reopen the untitled document, rebind the terminal. Also added a centralization of `getLogCapture().isCapturing()` in `StandardSuite.setup()` for every test to get it
Owner
Author
|
@coderabbitai review - Your status says |
Contributor
|
The Comment image shows the PR's checks panel with all 4 checks passing — CI / Integration Tests (with extensions) (6m), CI / Test & Validate (7m), CodeRabbit — Review completed, and QA Coverage Gap Check (14s) — along with a green "No conflicts with base branch" status and the Squash and merge button.
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts (1)
306-319: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse shared constants instead of hardcoded default values.
The test still hardcodes
1500and300for the default values. As flagged in the previous review, these should be imported from shared constants to prevent drift.As per coding guidelines, "Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE". Import
CLAUDE_CODE_COLD_START_DELAY_MSandCLAUDE_CODE_COLD_REFOCUS_INTERVAL_MSfrom the shared defaults module and use them in both theconfig.getfallback parameters and the assertions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts` around lines 306 - 319, Replace the hardcoded numeric defaults in the test "claude-code-006: Cold-start default settings produce correct ColdRefocusConfig" by importing CLAUDE_CODE_COLD_START_DELAY_MS and CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS from the shared defaults module and using them as the fallback values in the calls to vscode.workspace.getConfiguration('rangelink.destinations.claudeCode').get(...) as well as in the assert.strictEqual checks for totalMs and intervalMs (and in the comparison/log message that references those variables) so the test reads and asserts against the shared constants rather than 1500 and 300.
🧹 Nitpick comments (1)
packages/rangelink-vscode-extension/src/__integration-tests__/suite/untitledNavigation.test.ts (1)
74-77: ⚡ Quick winReplace semantic numeric literal with a named constant.
Please extract the line-count literal into a SCREAMING_SNAKE_CASE constant to keep fixture intent explicit and guideline-compliant.
Proposed change
+const UNTITLED_LINE_COUNT = 15; const UNTITLED_CONTENT = Array.from( - { length: 15 }, + { length: UNTITLED_LINE_COUNT }, (_, i) => `untitled line ${i + 1} content here`, ).join('\n');As per coding guidelines, "Define named constants for all numeric literals with semantic meaning using SCREAMING_SNAKE_CASE".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rangelink-vscode-extension/src/__integration-tests__/suite/untitledNavigation.test.ts` around lines 74 - 77, The numeric literal 15 used to build UNTITLED_CONTENT should be extracted into a named SCREAMING_SNAKE_CASE constant (e.g., UNTITLED_LINE_COUNT) to make the fixture intent explicit; define the constant above where UNTITLED_CONTENT is declared and replace the literal in the Array.from call with this constant so UNTITLED_CONTENT continues to use Array.from({ length: UNTITLED_LINE_COUNT }, (_, i) => `untitled line ${i + 1} content here`).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts`:
- Around line 321-353: The test claude-code-007 currently only checks for a
validation warning but must also verify that fallback defaults are applied:
after executing CMD_BIND_TO_CLAUDE_CODE, read
vscode.workspace.getConfiguration('rangelink.destinations.claudeCode') and
assert the effective coldStartDelayMs and coldRefocusIntervalMs equal the
expected defaults (use the shared/default constants or 1500 and 300 as
suggested); if CMD_JUMP_TO_DESTINATION is unnecessary for validation, remove it,
otherwise add an explicit assertion that the destination action uses the
fallback values; also replace magic numbers 100/400 with descriptive constants
(e.g., INVALID_DELAY_MS, INVALID_INTERVAL_MS) or an inline comment.
---
Duplicate comments:
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts`:
- Around line 306-319: Replace the hardcoded numeric defaults in the test
"claude-code-006: Cold-start default settings produce correct ColdRefocusConfig"
by importing CLAUDE_CODE_COLD_START_DELAY_MS and
CLAUDE_CODE_COLD_REFOCUS_INTERVAL_MS from the shared defaults module and using
them as the fallback values in the calls to
vscode.workspace.getConfiguration('rangelink.destinations.claudeCode').get(...)
as well as in the assert.strictEqual checks for totalMs and intervalMs (and in
the comparison/log message that references those variables) so the test reads
and asserts against the shared constants rather than 1500 and 300.
---
Nitpick comments:
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/untitledNavigation.test.ts`:
- Around line 74-77: The numeric literal 15 used to build UNTITLED_CONTENT
should be extracted into a named SCREAMING_SNAKE_CASE constant (e.g.,
UNTITLED_LINE_COUNT) to make the fixture intent explicit; define the constant
above where UNTITLED_CONTENT is declared and replace the literal in the
Array.from call with this constant so UNTITLED_CONTENT continues to use
Array.from({ length: UNTITLED_LINE_COUNT }, (_, i) => `untitled line ${i + 1}
content here`).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0cae406-e4a3-46fe-b90e-7c72592d56c6
📒 Files selected for processing (10)
packages/rangelink-vscode-extension/src/__integration-tests__/helpers/editorHelpers.tspackages/rangelink-vscode-extension/src/__integration-tests__/helpers/index.tspackages/rangelink-vscode-extension/src/__integration-tests__/helpers/standardSuite.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/clipboardPreservation.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/dirtyBufferWarning.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/navigationToastSettings.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/sendFilePath.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/smartPadding.test.tspackages/rangelink-vscode-extension/src/__integration-tests__/suite/untitledNavigation.test.ts
💤 Files with no reviewable changes (2)
- packages/rangelink-vscode-extension/src/integration-tests/suite/navigationToastSettings.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/suite/sendFilePath.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rangelink-vscode-extension/src/integration-tests/suite/clipboardPreservation.test.ts
- packages/rangelink-vscode-extension/src/integration-tests/helpers/standardSuite.ts
Comment on lines
+321
to
+353
| test('claude-code-007: Cold-start validation rejects invalid config and falls back to defaults', async function (this: MochaContext) { | ||
| if (!vscode.extensions.getExtension(CLAUDE_CODE_EXTENSION_ID)) { | ||
| log('Skipping claude-code-007 — Claude Code extension not installed in this test config'); | ||
| this.skip(); | ||
| } | ||
|
|
||
| const config = vscode.workspace.getConfiguration('rangelink.destinations.claudeCode'); | ||
|
|
||
| // Set invalid config: delay (100) <= interval (400) | ||
| await config.update('coldStartDelayMs', 100, vscode.ConfigurationTarget.Workspace); | ||
| await config.update('coldRefocusIntervalMs', 400, vscode.ConfigurationTarget.Workspace); | ||
| await settle(); | ||
|
|
||
| const logCapture = getLogCapture(); | ||
| logCapture.mark('before-cc-007'); | ||
|
|
||
| await vscode.commands.executeCommand(CMD_BIND_TO_CLAUDE_CODE); | ||
| await settle(); | ||
|
|
||
| await vscode.commands.executeCommand(CMD_JUMP_TO_DESTINATION); | ||
| await settle(); | ||
|
|
||
| const lines = logCapture.getLinesSince('before-cc-007'); | ||
| const warningLog = lines.find((line) => | ||
| line.includes('coldStartDelayMs must be greater than coldRefocusIntervalMs'), | ||
| ); | ||
| assert.ok( | ||
| warningLog, | ||
| 'Expected validation warning log when coldStartDelayMs <= coldRefocusIntervalMs', | ||
| ); | ||
|
|
||
| log('✓ claude-code-007 — invalid config triggers fallback to defaults'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Verify the actual fallback values, not just the warning log.
The test asserts that a validation warning is logged but doesn't verify that the fallback values are actually used. The test name claims the config "falls back to defaults," but the test only checks for the warning.
Additional concerns
-
Unclear CMD_JUMP_TO_DESTINATION purpose: The PR objectives state validation happens at bind time, but the test executes
CMD_JUMP_TO_DESTINATIONafter binding. What is this command verifying? If it's unnecessary, remove it; if it's validating that the destination works with fallback values, add an assertion explaining that. -
Magic numbers: The invalid config values
100and400are hardcoded. Consider defining them as constants with descriptive names (e.g.,INVALID_DELAY_MS,INVALID_INTERVAL_MS) or add an inline comment explaining why these specific values make the config invalid.
💚 Proposed enhancement
After the bind command, retrieve the actual configuration values that were applied and assert they match the expected defaults:
await vscode.commands.executeCommand(CMD_BIND_TO_CLAUDE_CODE);
await settle();
+// Verify fallback values were applied
+const configAfterBind = vscode.workspace.getConfiguration('rangelink.destinations.claudeCode');
+const actualTotalMs = configAfterBind.get<number>('coldStartDelayMs', 1500);
+const actualIntervalMs = configAfterBind.get<number>('coldRefocusIntervalMs', 300);
+assert.strictEqual(actualTotalMs, 1500, 'Expected fallback to default coldStartDelayMs');
+assert.strictEqual(actualIntervalMs, 300, 'Expected fallback to default coldRefocusIntervalMs');
+
-await vscode.commands.executeCommand(CMD_JUMP_TO_DESTINATION);
-await settle();
-
const lines = logCapture.getLinesSince('before-cc-007');
const warningLog = lines.find((line) =>
line.includes('coldStartDelayMs must be greater than coldRefocusIntervalMs'),
);(Note: Replace 1500 and 300 with the shared constants once you address the claude-code-006 comment.)
As per coding guidelines (qa-test-cases-v1.1.0.yaml), claude-code-007 should "bind to Claude Code, assert a warning log, and assert fallback to defaults."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/rangelink-vscode-extension/src/__integration-tests__/suite/builtInAiAssistants.test.ts`
around lines 321 - 353, The test claude-code-007 currently only checks for a
validation warning but must also verify that fallback defaults are applied:
after executing CMD_BIND_TO_CLAUDE_CODE, read
vscode.workspace.getConfiguration('rangelink.destinations.claudeCode') and
assert the effective coldStartDelayMs and coldRefocusIntervalMs equal the
expected defaults (use the shared/default constants or 1500 and 300 as
suggested); if CMD_JUMP_TO_DESTINATION is unnecessary for validation, remove it,
otherwise add an explicit assertion that the destination action uses the
fallback values; also replace magic numbers 100/400 with descriptive constants
(e.g., INVALID_DELAY_MS, INVALID_INTERVAL_MS) or an inline comment.
…odeRabbit findings Applied 3 CodeRabbit findings on the claude-code-007 integration test: strengthened the warning-log assertion to verify "using defaults" appears in the message (proving fallback values were returned, not just that the invalid state was detected), added a comment explaining why CMD_JUMP_TO_DESTINATION is needed — the validation lives inside getColdRefocus, a thunk only invoked during focus(), not bind(), and CMD_JUMP_TO_DESTINATION triggers focusBoundDestination() → focus() → getColdRefocus() → validation warning — and replaced magic numbers 100/400 with named INVALID_DELAY_MS and INVALID_INTERVAL_MS constants with a comment explaining the principle (inverting the valid delay > interval relationship) without hardcoding default values that would age when defaults change. Ref: #567 (review)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
The first paste after binding to Claude Code fires before the chat panel's webview IPC clipboard reader is ready, silently losing the link. This adds a cold-start re-focus loop that re-sends focus commands at a configurable interval throughout a configurable window, giving the panel time to initialize before paste dispatch. Subsequent pastes use the existing 200ms warm-path delay.
Changes
ColdRefocusConfigvalue object (totalMs,intervalMs) — shared by all webview-based AI assistants (Gemini Add support for Gemini Code Assist as built-in AI assistant #529 reuses it)AIAssistantFocusCapability.refocusDuring()— loops focus-command sends atintervalMsticks for up tototalMson cold start;panelIsWarmflag ensures the loop only fires once per binding sessiongetColdRefocushook onBuiltinAiAssistantDef— reads two new VS Code settings, validatestotalMs > intervalMs, falls back to defaults with a warning when invalidConfigReaderonDestinationBuilderContext— destination builders can now resolve settings at bind time without threadingConfigReaderthrough layersrangelink.destinations.claudeCode.coldStartDelayMs(default 1500, range 500–15000) andcoldRefocusIntervalMs(default 300, range 100–5000)<sup>Unreleased</sup>markers)claude-code-006(default config valid) andclaude-code-007(invalid config fallback), bothautomated: trueTest Plan
AIAssistantFocusCapability.test.ts(10 tests — cold start, command fallback, all-fail, warm path, refocus timing, no refocus on warm, elapsed logging, invalid config fallback)FocusCapabilityFactory.test.ts,destinationBuilders.test.ts,DestinationRegistry.test.ts,packageJsonContracts.test.ts,settingDefaults.test.ts,ComposablePasteDestination.integration.test.tsclaude-code-006andclaude-code-007inbuiltInAiAssistants.test.tsCloses #552
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests