Fixes for hot reload #376
Fixes for hot reload #376
Conversation
WalkthroughThe changes introduce centralized ignore rule handling, refactor the development server restart logic, and enhance file watching with improved ignore filtering and debouncing. TypeScript type checking is now enforced during JavaScript bundling. Default ignore patterns are expanded and made recursive, and corresponding tests are added to verify correct ignore behavior. Unused dependencies are removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DevCmd
participant Watcher
participant Bundler
participant IgnoreRules
User->>DevCmd: Start development server
DevCmd->>Watcher: Initialize with IgnoreRules
Watcher->>IgnoreRules: Filter files/directories
Watcher->>DevCmd: On file change (debounced)
DevCmd->>Bundler: Rebuild project
Bundler->>Bundler: runTypecheck (TypeScript)
Bundler->>Bundler: Bundle JS if typecheck passes
DevCmd->>DevCmd: Restart server if needed
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
internal/ignore/rules.go (1)
131-169:⚠️ Potential issue
Ignorepanics whenfi == nilSeveral call-sites (e.g. new unit-test
rules_test.go) pass a nilos.FileInfo.
Ignoredereferencesfiunconditionally (fi.IsDir()), leading to a runtime panic.func (r *Rules) Ignore(path string, fi os.FileInfo) bool { ... - for _, p := range r.patterns { + for _, p := range r.patterns { ... - if p.negate { - if p.mustDir && !fi.IsDir() { + if p.negate { + if p.mustDir && (fi == nil || !fi.IsDir()) { return true } - if !p.match(path, fi) { + if !p.match(path, fi) { return true } continue } ... - if p.mustDir && !fi.IsDir() { + if p.mustDir && (fi == nil || !fi.IsDir()) { continue }Fail fast on
path == ""already exists, so this patch is safe.
🧹 Nitpick comments (6)
internal/ignore/rules.go (1)
80-86: Duplicate default patterns – minor cleanup
**/.cursor/**and**/.vscode/**are added twice. Removing duplicates avoids unnecessary matcher allocations.internal/bundler/bundler.go (1)
120-139:runTypecheckemits nothing ifWriteris nil
cmd.Stdout/Stderr = ctx.Writerwill silently droptscoutput whenWriterisnil(e.g. non-interactive deploy).
Consider defaulting toos.Stdout/os.Stderror the logger:if ctx.Writer != nil { cmd.Stdout = ctx.Writer cmd.Stderr = ctx.Writer } else { + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr }This preserves useful diagnostics during CI builds.
cmd/dev.go (1)
252-257: Graceful shutdown – wait for watcher goroutineAfter calling
watcher.Close(log)you still have the goroutine spawned inrunServerpotentially blocking onWait().
Join it (e.g. viasync.WaitGroup) or signal it to avoid leaking goroutines on exit.cmd/cloud.go (1)
130-152: Minor shadowing – avoidrshort-var inside theifUsing
r, err := ...shadows the outerrulesvariable and then re-assigns. Not harmful, but slightly confusing. Consider:if util.Exists(gitignore) { parsed, err := ignore.ParseFile(gitignore) ... rules = parsed }internal/dev/watcher.go (2)
42-45: Consider initializing ignore patterns in the Rules constructorThe ignore patterns are being added to the rules object after it's created. This could lead to inconsistency if multiple watchers are created with the same rules object.
Consider moving these default patterns to the
ignore.Rulesinitialization or create a copy of the rules object before modifying it:- for _, pattern := range ignorePatterns { - rules.Add(pattern) - } + // Option 1: Clone the rules to avoid modifying the shared instance + localRules := rules.Clone() + for _, pattern := range ignorePatterns { + localRules.Add(pattern) + } + fw.ignore = localRulesAlternatively, these patterns could be added as defaults in the
ignore.Rules.AddDefaults()method mentioned in the AI summary.
58-60: Consider using a debug logger method instead of conditional loggingThe pattern of checking
debugLoggingbefore logging could be simplified by using a debug logger method that internally handles the conditional check.Instead of:
if debugLogging { logger.Trace("Ignoring path: %s", path) }Consider implementing a
Debugmethod on the logger that checks the flag internally, allowing cleaner code:logger.Debug("Ignoring path: %s", path)Also applies to: 63-63, 87-87, 100-100, 105-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
cmd/cloud.go(2 hunks)cmd/dev.go(3 hunks)go.mod(0 hunks)internal/bundler/bundler.go(2 hunks)internal/dev/watcher.go(2 hunks)internal/ignore/rules.go(1 hunks)internal/ignore/rules_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🧰 Additional context used
🧬 Code Graph Analysis (3)
cmd/cloud.go (5)
internal/project/project.go (2)
Project(145-154)Bundler(481-490)internal/ignore/rules.go (4)
Rules(46-48)Ignore(40-40)Empty(51-53)ParseFile(95-102)internal/util/io.go (1)
Exists(14-19)internal/errsystem/errsystem.go (2)
New(33-63)WithContextMessage(100-104)internal/errsystem/errorcodes.go (1)
ErrInvalidConfiguration(25-28)
internal/ignore/rules_test.go (1)
internal/ignore/rules.go (2)
Empty(51-53)Ignore(40-40)
internal/bundler/bundler.go (1)
internal/util/io.go (1)
Exists(14-19)
🪛 golangci-lint (1.64.8)
internal/ignore/rules.go
57-57: Error return value of r.parseRule is not checked
(errcheck)
58-58: Error return value of r.parseRule is not checked
(errcheck)
59-59: Error return value of r.parseRule is not checked
(errcheck)
🔇 Additional comments (4)
internal/ignore/rules_test.go (1)
12-38: Tests will panic – supply a fakeos.FileInfoBecause
Ignorecurrently dereferencesfi, passingnilwill panic (see previous comment).
Either adjust the production code to toleratenil(recommended) or create a stub for the tests:type fakeInfo struct{ os.FileInfo } func (fakeInfo) IsDir() bool { return false } fi := fakeInfo{} assert.True(t, rules.Ignore("/path/to/.gitignore", fi))After fixing
Ignoreas suggested, the currentnilusage becomes valid.internal/dev/watcher.go (3)
88-96: Good defensive programming with file existence checksThe addition of file existence and stat checks prevents potential panics from race conditions where files are deleted between event notification and processing.
24-34:⚠️ Potential issueRemove duplicate ignore pattern
The pattern
"**/.agentuity"appears twice in theignorePatternsslice (lines 24 and 33).Apply this diff to remove the duplicate:
var ignorePatterns = []string{ "**/.agentuity/**", "**/package-lock.json", "**/package.json", "**/yarn.lock", "**/pnpm-lock.yaml", "**/bun.lock", "**/bun.lockb", "**/tsconfig.json", "**/agentuity.yaml", - "**/.agentuity", }Likely an incorrect or invalid review comment.
73-107:⚠️ Potential issuePotential race condition in debouncing implementation
The
pendingmap is accessed from multiple goroutine contexts without synchronization. The ticker case reads and deletes from the map while the event case writes to it, which could lead to a race condition.Consider using a mutex to protect access to the
pendingmap:+import "sync" func (fw *FileWatcher) watch(logger logger.Logger) { t := time.NewTicker(250 * time.Millisecond) // how long to debounce changes defer t.Stop() pending := make(map[string]bool) + var mu sync.Mutex for { select { case <-t.C: + mu.Lock() for path := range pending { fw.callback(path) delete(pending, path) } + mu.Unlock() case event, ok := <-fw.watcher.Events: // ... existing code ... if (event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Create == fsnotify.Create) && !fw.ignore.Ignore(event.Name, fi) { logger.Trace("Write detected for %s", event.Name) + mu.Lock() pending[event.Name] = true + mu.Unlock() }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cmd/dev.go (2)
170-190:⚠️ Potential issueCritical: Race condition with shared
projectServerCmdvariable.The
runServerfunction assigns to the sharedprojectServerCmdvariable without proper synchronization, while therestartfunction accesses it with a different mutex. This creates a race condition whererestartmight access a nil or staleprojectServerCmdreference.The past review comment correctly identified this issue. Both functions must use the same mutex when accessing
projectServerCmd.runServer := func() { + restartingLock.Lock() projectServerCmd, err = dev.CreateRunProjectCmd(processCtx, log, theproject, server, dir, orgId, port, os.Stdout, os.Stderr) if err != nil { + restartingLock.Unlock() errsystem.New(errsystem.ErrInvalidConfiguration, err, errsystem.WithContextMessage("Failed to run project")).ShowErrorAndExit() } if err := projectServerCmd.Start(); err != nil { + restartingLock.Unlock() errsystem.New(errsystem.ErrInvalidConfiguration, err, errsystem.WithContextMessage(fmt.Sprintf("Failed to start project: %s", err))).ShowErrorAndExit() } atomic.StoreInt32(&pid, int32(projectServerCmd.Process.Pid)) + restartingLock.Unlock() log.Trace("restarted project server (pid: %d)", projectServerCmd.Process.Pid) log.Trace("waiting for project server to exit (pid: %d)", projectServerCmd.Process.Pid) if err := projectServerCmd.Wait(); err != nil { log.Error("project server (pid: %d) exited with error: %s", projectServerCmd.Process.Pid, err) } // ... rest unchanged }
199-208:⚠️ Potential issuePartial protection against race condition.
The mutex protects against concurrent restarts but doesn't synchronize access to
projectServerCmdwith therunServerfunction. Thedev.KillProjectServercall on line 203 could access a nil or staleprojectServerCmdifrunServeris concurrently reassigning it.This is part of the same race condition issue identified in the past review comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/dev.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
cmd/dev.go (8)
10-11: LGTM: Appropriate imports for synchronization primitives.The addition of
syncandsync/atomicpackages aligns with the new concurrent process management logic.
120-120: LGTM: Atomic-compatible type for PID.Using
int32enables atomic operations for thread-safe PID access across goroutines.
197-197: LGTM: Mutex for preventing concurrent restarts.The
restartingLockmutex appropriately prevents multiple restart operations from running simultaneously.
215-215: LGTM: Simplified watcher callback.The direct call to
restart()removes external debouncing logic, which aligns with the summary indicating that debouncing is now handled internally by the watcher.
228-229: LGTM: Atomic PID storage for thread safety.Using
atomic.StoreInt32ensures thread-safe PID updates that can be safely read from other goroutines.
253-253: LGTM: Clean interrupt character handling.The escape sequence
\b\b\033[Kproperly clears the "^C" characters displayed when Ctrl+C is pressed.
257-257: LGTM: Nice farewell message.The friendly goodbye message improves user experience.
213-213:Details
❌ Incorrect review comment
Verify ignore rules function exists.
The code references
createProjectIgnoreRules(dir, theproject.Project)but this function isn't defined in the provided file.
🏁 Script executed:
#!/bin/bash # Description: Verify that createProjectIgnoreRules function exists in the codebase # Expected: Function definition should be found ast-grep --pattern 'func createProjectIgnoreRules($_, $_) $_ { $$$ }'Length of output: 1263
Function
createProjectIgnoreRulesIs Defined
The functioncreateProjectIgnoreRules(dir string, theproject *project.Project) *ignore.Rulesis implemented incmd/cloud.go(lines 130–152), so the reference incmd/dev.gois valid. No action required.Likely an incorrect or invalid review comment.
Summary by CodeRabbit