Add feature switches and new survey#265
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a FeatureFlagManager (cache + remote refresh), initializes it at server startup, exposes flags via the config endpoint, updates the Tally survey base URL, and extends usage-tracking with feature-flag gating, per-session/day limits, attempt caps, new tracking fields, and multiple prompt variants. Changes
Sequence Diagram(s)sequenceDiagram
participant Server as Server Startup
participant FM as FeatureFlagManager
participant Cache as feature-flags.json
participant Remote as Remote Flag Service
Server->>FM: initialize()
activate FM
FM->>Cache: loadFromCache()
alt cache present
Cache-->>FM: return flags
FM->>FM: set internal flags
else cache missing/invalid
FM->>FM: continue with empty/defaults
end
Note over FM: start background fetch & schedule periodic refresh (interval)
FM->>Remote: fetchFlags() [async, 5s timeout]
alt success
Remote-->>FM: flags JSON
FM->>Cache: saveToCache()
FM->>FM: update internal flags
else timeout/error
FM-->>FM: log debug/warn, keep cached flags
end
deactivate FM
Server->>Server: continue startup
sequenceDiagram
participant User as User Session
participant Tracker as UsageTracker
participant FM as FeatureFlagManager
participant Feedback as Feedback System
User->>Tracker: newSession()
Tracker->>Tracker: promptedThisSession = false
loop each tool invocation
User->>Tracker: recordToolUse()
Tracker->>Tracker: increment counters
alt commandsInSession == 1
Tracker->>FM: getAll() (check user_surveys)
alt user_surveys enabled
Tracker->>Tracker: evaluate gating (client, attempts≤3, not prompted today, promptedThisSession=false, 3+ days, ≥10 calls)
alt all gates pass
Tracker->>Feedback: getFeedbackPromptMessage()
Feedback-->>Tracker: prompt variant
Tracker->>Tracker: markFeedbackPrompted()
Tracker->>User: show prompt
else any gate fails
Tracker->>User: no prompt
end
else flag disabled
Tracker->>User: no prompt
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
codeant-ai
Bot
added
the
size:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| private flags: Record<string, any> = {}; | ||
| private lastFetch: number = 0; | ||
| private cachePath: string; | ||
| private cacheMaxAge: number = 0.5 * 60 * 1000; // 5 minutes - hardcoded refresh interval |
There was a problem hiding this comment.
Suggestion: Fix the incorrect milliseconds calculation for the cache refresh interval so the comment and value match (use 5 * 60 * 1000 for five minutes). [possible bug]
Severity Level: Critical 🚨
| private cacheMaxAge: number = 0.5 * 60 * 1000; // 5 minutes - hardcoded refresh interval | |
| private cacheMaxAge: number = 5 * 60 * 1000; // 5 minutes - hardcoded refresh interval |
Why it matters? ⭐
The current expression 0.5 * 60 * 1000 evaluates to 30_000ms (30 seconds), which contradicts the inline comment and the later logging that reports the refresh interval. Changing to 5 * 60 * 1000 fixes an obvious arithmetic bug so setInterval runs at the intended 5-minute cadence. This is a real bugfix (timing) and has no syntactic risk.
Prompt for AI Agent 🤖
<code>This is a comment left during a code review.
**Path:** src/utils/feature-flags.ts
**Line:** 16:16
**Comment:**
*Possible Bug: Fix the incorrect milliseconds calculation for the cache refresh interval so the comment and value match (use 5 * 60 * 1000 for five minutes).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/tools/feedback.ts (1)
91-91: Deduplicate the feedback form URL (single source of truth).The Tally form URL appears twice. Define one constant and reuse to prevent drift on future updates.
+const FEEDBACK_FORM_URL = 'https://tally.so/r/mKqoKg'; ... - `You can still access our feedback form directly at: https://tally.so/r/mKqoKg\n\n` + + `You can still access our feedback form directly at: ${FEEDBACK_FORM_URL}\n\n` + ... - const baseUrl = 'https://tally.so/r/mKqoKg'; + const baseUrl = FEEDBACK_FORM_URL;Please confirm mKqoKg is the intended production form ID and whether we should load it from config/env to allow rotation without code changes.
Also applies to: 103-103
src/utils/usageTracker.ts (1)
340-349: Day-based gating uses UTC date; consider local day to match user expectations.Using toISOString().split('T')[0] gates by UTC. If local time crosses midnight earlier/later than UTC, prompts may repeat or be suppressed unexpectedly.
Compute local “YYYY-MM-DD”:
-const today = new Date().toISOString().split('T')[0]; +const d = new Date(); +const today = `${d.getFullYear()}-${String(d.getMonth()+1).padStart(2,'0')}-${String(d.getDate()).padStart(2,'0')}`;src/tools/config.ts (1)
5-5: Expose flags: LGTM, minor hardening optional.featureFlags: featureFlagManager.getAll() is fine. If some flags are internal, consider filtering or namespacing before exposing. Optionally deep-freeze the object in the response to prevent accidental mutation downstream.
Also applies to: 31-31
src/index.ts (1)
47-49: Initialize flags: LGTM. Consider cleanup on shutdown.Call featureFlagManager.destroy() on exit signals to clear the refresh interval so it never keeps the process alive in edge cases.
+process.on('SIGINT', () => { try { featureFlagManager.destroy(); } finally { process.exit(0); } }); +process.on('SIGTERM', () => { try { featureFlagManager.destroy(); } finally { process.exit(0); } }); +process.on('exit', () => { featureFlagManager.destroy(); });src/utils/feature-flags.ts (1)
43-49: Optional: prevent the timer from keeping the process alive.Call unref() on the interval so Node can exit cleanly when idle.
- this.refreshInterval = setInterval(() => { + this.refreshInterval = setInterval(() => { this.fetchFlags().catch(err => { logger.debug('Periodic flag fetch failed:', err.message); }); - }, this.cacheMaxAge); + }, this.cacheMaxAge); + if (this.refreshInterval.unref) this.refreshInterval.unref();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/index.ts(2 hunks)src/tools/config.ts(2 hunks)src/tools/feedback.ts(2 hunks)src/utils/feature-flags.ts(1 hunks)src/utils/usageTracker.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/index.ts (1)
src/utils/feature-flags.ts (1)
featureFlagManager(175-175)
src/utils/feature-flags.ts (1)
src/utils/logger.ts (1)
logger(55-64)
src/tools/config.ts (1)
src/utils/feature-flags.ts (1)
featureFlagManager(175-175)
src/utils/usageTracker.ts (3)
src/utils/feature-flags.ts (1)
featureFlagManager(175-175)src/server.ts (1)
currentClient(148-148)src/config-manager.ts (1)
configManager(219-219)
🔇 Additional comments (3)
src/utils/feature-flags.ts (2)
25-27: Review comment references incorrect lines; CONFIG_FILE is properly exported and available.The CONFIG_FILE import at line 4 and cache path resolution at lines 21-22 are working correctly. CONFIG_FILE is properly exported from
../config.jsand the module path resolves without issues. Lines 25-27 shown in the snippet don't reference CONFIG_FILE, so the review comment points to the wrong location and raises an unfounded concern.Likely an incorrect or invalid review comment.
115-121: No changes needed—Node version already enforces fetch availability.The project specifies
"engines": { "node": ">=18.0.0" }in package.json, which guarantees native fetch support. Node 18+ includes fetch as a global by default, so no polyfill or guard is required.src/utils/usageTracker.ts (1)
219-236: No call order gating issue exists—verification confirms proper sequencing.The code correctly calls
trackSuccess/trackFailureat lines 1250/1253 beforeshouldPromptForFeedbackat line 1295 in src/server.ts. Both track methods immediately callupdateSession()to incrementcommandsInSession, ensuring the gating check at line 251 (commandsInSession !== 1) works as intended. Only one call site exists, and it follows the correct order.
| private flags: Record<string, any> = {}; | ||
| private lastFetch: number = 0; | ||
| private cachePath: string; | ||
| private cacheMaxAge: number = 0.5 * 60 * 1000; // 5 minutes - hardcoded refresh interval |
There was a problem hiding this comment.
Refresh interval mismatch (30s vs. “5 minutes”).
0.5 * 60 * 1000 = 30,000 ms (30s). The comment and behavior suggest 5 minutes.
- private cacheMaxAge: number = 0.5 * 60 * 1000; // 5 minutes - hardcoded refresh interval
+ private cacheMaxAge: number = 5 * 60 * 1000; // 5 minutes📝 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.
| private cacheMaxAge: number = 0.5 * 60 * 1000; // 5 minutes - hardcoded refresh interval | |
| private cacheMaxAge: number = 5 * 60 * 1000; // 5 minutes |
🤖 Prompt for AI Agents
In src/utils/feature-flags.ts around line 16, the numeric expression 0.5 * 60 *
1000 evaluates to 30,000ms (30s) but the comment states "5 minutes"; fix by
making the code and comment consistent: either set cacheMaxAge to 5 * 60 * 1000
(for 5 minutes) and keep the comment, or if the intended refresh is 30s, update
the comment to "30 seconds" (or "30s - hardcoded refresh interval"); pick the
correct intended interval and update the value or the comment accordingly.
| lastFeedbackPrompt: number; // timestamp (deprecated, use lastFeedbackPromptDate) | ||
| lastFeedbackPromptDate?: string; // date string like '2025-10-20' | ||
| feedbackAttempts?: number; // How many times we've shown feedback prompt (max 3) |
There was a problem hiding this comment.
🧩 Analysis chain
Type narrowing bug after adding new fields — getToolCategory return type now too broad.
Adding lastFeedbackPromptDate/feedbackAttempts expands the key union; stats[category]++ may become invalid at compile time or unsafe at runtime.
Use an explicit category key type to ensure only numeric counters are returned.
+type UsageCategoryKey =
+ | 'filesystemOperations'
+ | 'terminalOperations'
+ | 'editOperations'
+ | 'searchOperations'
+ | 'configOperations'
+ | 'processOperations';
...
- private getToolCategory(toolName: string): keyof Omit<ToolUsageStats, 'totalToolCalls' | 'successfulCalls' | 'failedCalls' | 'toolCounts' | 'firstUsed' | 'lastUsed' | 'totalSessions' | 'lastFeedbackPrompt'> | null {
+ private getToolCategory(toolName: string): UsageCategoryKey | null {Run tsc to ensure no remaining arithmetic on non‑numeric fields compiles cleanly.
🏁 Script executed:
#!/bin/bash
# Verify category typing and numeric increments compile
rg -nP 'getToolCategory\s*\(' src/utils/usageTracker.ts -n -C2
# Type-check whole project (tsconfig assumed)
npm run -s build || npx tsc -p .Length of output: 1355
🏁 Script executed:
#!/bin/bash
# Find ToolUsageStats interface definition
rg -nP 'interface\s+ToolUsageStats|type\s+ToolUsageStats' src/utils/usageTracker.ts -A 20
# Find TOOL_CATEGORIES definition
rg -nP 'TOOL_CATEGORIES\s*=' src/utils/usageTracker.ts -A 15
# Check if new fields appear anywhere as category keys
rg -n 'lastFeedbackPromptDate|feedbackAttempts' src/utils/usageTracker.ts | head -20Length of output: 1747
🏁 Script executed:
#!/bin/bash
# Check the exact type error TypeScript would produce
# by looking at the full getToolCategory function and stats increment usage
rg -nP 'private getToolCategory' src/utils/usageTracker.ts -A 10
# Check if there's a fix already in place (looking for explicit category type or updated Omit)
rg -nP 'type.*CategoryKey|interface.*Category' src/utils/usageTracker.tsLength of output: 730
🏁 Script executed:
#!/bin/bash
# Check lines around stats[category]++ to see the full context
rg -nP 'stats\[category\]\+\+' src/utils/usageTracker.ts -B 3 -A 3
# Check if there are any tsc errors or type checks in package.json or tsconfig
cat tsconfig.json 2>/dev/null || cat package.json | grep -A 5 '"scripts"'Length of output: 896
Type narrowing bug confirmed — getToolCategory return type must exclude new feedback fields.
The Omit at line 104 excludes 8 fields but misses lastFeedbackPromptDate and feedbackAttempts. With strict: true in tsconfig, stats[category]++ at lines 171 and 203 will fail TypeScript type-checking if category could be a string field instead of a numeric counter.
Fix: Add the new fields to the Omit exclusion list, or introduce an explicit category key type:
+type UsageCategoryKey =
+ | 'filesystemOperations'
+ | 'terminalOperations'
+ | 'editOperations'
+ | 'searchOperations'
+ | 'configOperations'
+ | 'processOperations';
...
- private getToolCategory(toolName: string): keyof Omit<ToolUsageStats, 'totalToolCalls' | 'successfulCalls' | 'failedCalls' | 'toolCounts' | 'firstUsed' | 'lastUsed' | 'totalSessions' | 'lastFeedbackPrompt'> | null {
+ private getToolCategory(toolName: string): UsageCategoryKey | null {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/utils/usageTracker.ts around lines 27 and 29 and the Omit at line 104,
the type narrowing for getToolCategory is incomplete because the new fields
lastFeedbackPromptDate and feedbackAttempts are not excluded; update the Omit to
also exclude "lastFeedbackPromptDate" and "feedbackAttempts" (or replace the
Omit with an explicit type for the category keys) so the returned category type
only contains numeric counters and TypeScript will allow stats[category]++ at
lines 171 and 203 to type-check.
| commandsInSession: 1, | ||
| promptedThisSession: false // Reset for new session | ||
| }; |
There was a problem hiding this comment.
Session counter likely off-by-one on first run.
Initializing commandsInSession to 1 and incrementing totalSessions when commandsInSession === 1, combined with default totalSessions = 1, yields 2 after the very first command.
Options:
- Start totalSessions at 0 in getDefaultStats; or
- Initialize commandsInSession to 0 on new session and increment immediately after; or
- Increment totalSessions when creating a new session instead of in trackSuccess/Failure.
Minimal change:
- commandsInSession: 1,
+ commandsInSession: 0,
promptedThisSession: falseAnd in trackSuccess/trackFailure, increment before the session check:
- // Update session count if this is a new session
- if (this.currentSession?.commandsInSession === 1) {
+ // Update session count if this is the first command of a new session
+ if (this.currentSession && ++this.currentSession.commandsInSession === 1) {
stats.totalSessions++;
}Or increment totalSessions when creating the session (preferred).
🤖 Prompt for AI Agents
In src/utils/usageTracker.ts around lines 142-144, the session counter is
off-by-one because new sessions start with commandsInSession: 1 while
totalSessions is also incremented when commandsInSession === 1 (and default
totalSessions = 1), producing 2 after the first command; fix by initializing new
sessions with commandsInSession: 0 (or set totalSessions default to 0) and move
the totalSessions increment to the session-creation path so a new session
increments totalSessions immediately when created (preferred), ensuring
trackSuccess/trackFailure only increments commandsInSession and does not adjust
totalSessions.
wonderwhy-er
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/utils/usageTracker.ts (1)
27-29: Type narrowing bug — past review not addressed.The new fields
lastFeedbackPromptDateandfeedbackAttemptsare not excluded in theOmitat line 104, allowinggetToolCategoryto potentially return these string/number field names. This causesstats[category]++at lines 171 and 203 to fail type-checking understrict: true.Apply the fix from the previous review:
+type UsageCategoryKey = + | 'filesystemOperations' + | 'terminalOperations' + | 'editOperations' + | 'searchOperations' + | 'configOperations' + | 'processOperations'; ... - private getToolCategory(toolName: string): keyof Omit<ToolUsageStats, 'totalToolCalls' | 'successfulCalls' | 'failedCalls' | 'toolCounts' | 'firstUsed' | 'lastUsed' | 'totalSessions' | 'lastFeedbackPrompt'> | null { + private getToolCategory(toolName: string): UsageCategoryKey | null {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/usageTracker.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/usageTracker.ts (3)
src/utils/feature-flags.ts (1)
featureFlagManager(175-175)src/server.ts (1)
currentClient(148-148)src/config-manager.ts (1)
configManager(219-219)
🔇 Additional comments (3)
src/utils/usageTracker.ts (3)
80-80: Session counter initialization fix looks correct.Changing
totalSessionsfrom 1 to 0 resolves the off-by-one issue flagged in the previous review. First command now correctly results intotalSessions = 1.
279-306: Variant-based prompt system looks good.The three message variants with clear CTAs and random selection support effective A/B testing of feedback prompts.
338-352: Tracking updates are correct.The function properly updates all feedback tracking fields and prevents duplicate prompts in the same session.
Note: Line 340 has the same timezone issue flagged earlier (uses UTC instead of local date).
| } | ||
|
|
||
| // Check if client is desktop-commander (disable for this client) | ||
| //return true; |
There was a problem hiding this comment.
Remove debug artifact.
The commented //return true; line should be removed before merging.
Apply this diff:
- //return true;📝 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.
| //return true; |
🤖 Prompt for AI Agents
In src/utils/usageTracker.ts around line 227, there is a leftover debug
artifact: a commented line "//return true;". Remove this commented line to clean
up the codebase before merging, ensuring no behavior changes are introduced.
| // Check if we've already prompted today | ||
| const today = new Date().toISOString().split('T')[0]; // '2025-10-20' | ||
| if (stats.lastFeedbackPromptDate === today) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Timezone mismatch in daily prompt check.
Using toISOString().split('T')[0] produces a UTC date. If the user is in a different timezone (e.g., PST), the "once per day" check may not align with their actual day boundary. For instance, 11 PM PST on Oct 22 is Oct 23 UTC.
Consider using local date:
- const today = new Date().toISOString().split('T')[0]; // '2025-10-20'
+ const today = new Date().toLocaleDateString('en-CA'); // 'YYYY-MM-DD' in local timezoneAnd update line 340 similarly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/utils/usageTracker.ts around lines 260 to 264 (and apply same change at
line ~340), the code uses toISOString().split('T')[0] which yields a UTC date
and causes the daily-prompt check to misalign with the user's local day; replace
that logic to compute the current local date string (e.g., build YYYY-MM-DD from
new Date() using local year/month/day or use toLocaleDateString with a
consistent locale/format) and compare stats.lastFeedbackPromptDate against that
local-date string so the "once per day" check respects the user's timezone; make
the same change at the other occurrence on line ~340.
1 participant
CodeAnt-AI Description
Add remote feature flags and tighten feedback survey prompts
What Changed
Impact
✅ Remote feature toggles visible in config✅ Fewer unwanted feedback prompts✅ Reachable feedback form via updated link💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Improvements