gorm in dedicated package

[pilotso11]

Cleaner package structure

[gemini-code-assist]

Summary of Changes

Hello @pilotso11, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request primarily focuses on improving the architectural cleanliness and data consistency of the lazywritercache project. I've restructured the repository to separate GORM-specific implementations into their own module using Go Workspaces, and significantly enhanced the cache's write-through mechanism to ensure that cached data accurately reflects the database's state after a write operation. Additionally, I've refined error handling for database interactions and updated test suite conventions.

Highlights

• Project Structure Refactoring: I've refactored the project to introduce a cleaner package structure by moving GORM-related code and dependencies into a new, dedicated Go module named lazygormcache. This change leverages Go Workspaces for better modularity and dependency management across the main project, the new GORM module, and the examples.
• Improved Cache Consistency on Writes: The LazyWriterCacheLF's saveDirtyToDB logic has been enhanced to re-fetch an item from the database immediately after a successful save within the same transaction. This ensures that the cache is updated with the definitive state of the item, including any database-generated values like IDs or timestamps, improving cache-to-DB consistency. Error handling for this re-fetch has also been added.
• Refined Transaction Error Handling: I've refined the error handling within saveDirtyToDB for both individual item saves and transaction commits. Unrecoverable errors during item saving now stop the current batch processing, and commit errors are now more precisely categorized and logged (Info for recoverable, Warn for unrecoverable), affecting how items are re-queued for retry.
• Test and Benchmark Renaming: Several test and benchmark functions in lazywritecache_lockfree_test.go have been renamed to follow a more consistent and descriptive naming convention, improving readability and discoverability of tests.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

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

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

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

[Copilot]

Pull Request Overview

This PR extracts the GORM-based cache into its own lazygormcache module, cleans up dependency versions, and enhances the existing lock-free lazy-write cache with improved error handling and re-fetch logic.

• Refactored GORM integration into a dedicated lazygormcache package with its own go.mod.
• Updated NoOpReaderWriterLF.Find to return no error on misses and corrected error messaging.
• Enhanced saveDirtyToDB to re-fetch saved items within the transaction and cleaned up transaction error handling.
• Renamed tests and benchmarks for clarity and added a new TestFlushWithErrOnLoadLF.
• Upgraded Go toolchain setup in CI and adjusted build steps.

Reviewed Changes

Copilot reviewed 9 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
noop_readerwriter_lockfree.go	Changed Find to return nil on misses and fixed the error prefix.
lazywritecache_lockfree_test.go	Renamed test/benchmark functions, added TestFlushWithErrOnLoadLF.
lazywritecache_lockfree.go	Added post-save DB re-fetch logic, adjusted batch transaction flows.
lazygormcache/go.mod	Introduced new module for GORM integration and dependencies.
go.mod, go.work, examples/go.mod	Updated module versions and workspace to include lazygormcache.
.github/workflows/go.yml	Upgraded to actions/setup-go@v5 and refined build commands.

Comments suppressed due to low confidence (3)

.github/workflows/go.yml:26

• The CI Build step compiles modules but does not run tests for the lazygormcache package or examples. Consider adding go test ./... for full coverage in CI.

      run: |

lazywritecache_lockfree_test.go:152

• [nitpick] Benchmark function names use underscores (e.g., Benchmark_LF_*); Go conventions generally prefer CamelCase (BenchmarkLFCacheWriteMax20k) for consistency.

func Benchmark_LF_CacheWriteMax20k(b *testing.B) {

lazywritecache_lockfree_test.go:653

• The time package is used here but not shown as imported. Make sure to add import "time" at the top of the test file to avoid compilation errors.

	cfg.WriteFreq = 50 * time.Second

[Copilot]

Find now returns no error when an item is missing, which may silently hide lookup failures. Consider returning a specific 'not found' error or updating call sites to handle the no-error case explicitly.

Suggested change

	return g.getTemplateItem(key), nil
	item := g.getTemplateItem(key)
	if item == nil { // Assuming nil indicates a missing item; adjust as needed for actual logic.
	return item, errors.New("item not found")
	}
	return item, nil

[Copilot]

After a failed re-fetch, unCommitted is appended twice for the same item, causing duplicate retry entries. Remove the redundant append in the error branch.

Suggested change

	// Add to unCommitted for transaction management, but it's the 'item' before re-fetch.
	unCommitted = append(unCommitted, item)
	// Item already added to unCommitted earlier; no need to append again.

[Copilot]

[nitpick] There's an unaddressed // todo: rollback here? comment. Consider converting to a // TODO: and implementing the rollback logic or documenting why it's not needed.

Suggested change

	// todo: rollback here?
	// If re-fetch fails, this is problematic. The DB save occurred, but we can't get the latest state.
	// This could lead to cache inconsistency. For now, log a warning and proceed to commit
	// the transaction, but the cache for this item will not be updated with post-save DB state.
	// A more robust strategy might involve rolling back if consistency is paramount.
	c.Handler.Warn(ctx, fmt.Sprintf("Failed to re-fetch item %v after save within transaction, cache may not reflect DB state: %v", item.Key(), findAfterSaveErr), ActionWriteDirty, item)
	// We still count this as a "success" for the DB write part, but cache update is skipped.
	// Add to unCommitted for transaction management, but it's the 'item' before re-fetch.
	unCommitted = append(unCommitted, item)
	return false // Continue with other items
	// TODO: Implement rollback logic or document why rollback is not needed.
	// If re-fetch fails, this is problematic. The DB save occurred, but we can't get the latest state.
	// This could lead to cache inconsistency. For now, log a warning and proceed to commit
	// the transaction, but the cache for this item will not be updated with post-save DB state.
	// A more robust strategy might involve rolling back if consistency is paramount.
	rollbackErr := tx.Rollback()
	if rollbackErr != nil {
	c.Handler.Error(ctx, fmt.Sprintf("Failed to rollback transaction after re-fetch failure for item %v: %v", item.Key(), rollbackErr), ActionWriteDirty, item)
	} else {
	c.Handler.Warn(ctx, fmt.Sprintf("Transaction rolled back due to re-fetch failure for item %v. Cache remains consistent, but DB save is undone: %v", item.Key(), findAfterSaveErr), ActionWriteDirty, item)
	}
	return false // Stop processing further items

[Copilot]

[nitpick] Re-fetching each item from the database inside the same transaction adds an extra round-trip per item. This could degrade batch write performance; consider making this optional or batching updates.

[gemini-code-assist]

Code Review

The pull request introduces a dedicated package for GORM integration, improving the package structure. The changes include updates to go.mod and go.sum files, modifications to the lazywritecache_lockfree.go file to re-fetch items from the DB after saving, and several test updates in lazywritecache_lockfree_test.go. The changes look good overall, but I have a few suggestions for improvements.

[gemini-code-assist]

Returning false here will stop processing the batch, which is good, but it might be better to re-add the item to the dirty list so it's retried in a future batch, even if the transaction is rolled back. This ensures that the cache eventually reflects the desired state, assuming the error is transient. Consider if this is the desired behavior or if it should be re-added to dirty for a future attempt.

Suggested change

	return false // Stop processing this batch, it will be retried
	return false // Stop processing this batch, it will be retried

[gemini-code-assist]

Consider adding a rollback here to ensure data consistency. If the re-fetch fails after a successful save, it indicates a potential issue with the database or data integrity. Rolling back the transaction would revert the save operation, preventing the cache from diverging from the database state.

Suggested change

	// todo: rollback here?
	// todo: rollback here?

[gemini-code-assist]

Consider using a more specific log level (e.g., Error) for this message, as it indicates a failure to re-fetch an item after a successful save, which could lead to cache inconsistency. Also, consider including the error itself in the log message for easier debugging.

Suggested change

	// A more robust strategy might involve rolling back if consistency is paramount.
	c.Handler.Warn(ctx, fmt.Sprintf("Failed to re-fetch item %v after save within transaction, cache may not reflect DB state: %v", item.Key(), findAfterSaveErr), ActionWriteDirty, item)
	c.Handler.Warn(ctx, fmt.Sprintf("Failed to re-fetch item %v after save within transaction, cache may not reflect DB state: %v", item.Key(), findAfterSaveErr), ActionWriteDirty, item)

[gemini-code-assist]

Returning nil here means that LookupOnMiss will always return a zero value, which might not be the desired behavior. Consider returning an error here to indicate that the item was not found.

Suggested change

	return g.getTemplateItem(key), nil
	return g.getTemplateItem(key), nil

[gemini-code-assist]

The assert message is not accurate, as there are no warnings expected in this scenario. The info count is also not accurate.

assert.Equal(t, int64(0), testHandler.warnCount.Load(), "Warnings")
	assert.Equal(t, int64(2), testHandler.infoCount.Load(), "Info for 'Recoverable error from CommitTx' and 'Error committing transaction'"

To fix the issue, the vulnerable version v16 of the tj-actions/verify-changed-files action should be updated to the secure version v17. This change ensures that the workflow uses a version of the action that has addressed the known vulnerabilities. The update involves modifying the uses field in the relevant step of the workflow file to reference the new version.

---

[pilotso11]

🤖 revbot automated review

✅ Review posted — see below.

[pilotso11]

🤖 revbot automated review

Recommendation: Fix ❌

This moves gorm and its drivers into a dedicated lazygormcache module behind a go.work workspace (root module slimmed down) and rewrites the build/test/benchmark CI to iterate every go.mod. The module split itself is a reasonable direction, but two things block this:

1. CI is red. The build job (and CodeQL) fail. The CI build step now runs find … go.mod … go build ./... per module, but it exits non-zero. The run logs have expired so I can't cite the exact compiler error, but the most likely cause is that the root module dropped gorm/gorm.io/driver/postgres/go-sqlmock/dburl while lazywritecache_lockfree.go in the root still drives transactions through Handler.Find(ctx, key, tx) / CommitTx / IsRecoverable — if any of those types still resolve into the root package, go build ./... for the root module breaks. Needs a green build before merge.

2. Correctness bug in the rewritten write path — see inline. On a post-save re-fetch failure, item is appended to unCommitted a second time (it was already appended a few lines above), and the return false there stops the whole batch despite its "// Continue with other items" comment.

The mechanical parts (test/benchmark renames, the example → examples move, the samples package extraction) look fine.

(Recommendation: fix — submitted as a comment because GitHub does not allow self-approval/self-request-changes on your own PR.)

[pilotso11]

Severity: Medium — item is appended to unCommitted twice, and the return contradicts its own comment

The success path a few lines above already did unCommitted = append(unCommitted, item). Here, on re-fetch failure, item is appended again, so unCommitted now holds the same item twice — which double-counts it in the commit, the post-commit dirty re-marking loop (for _, item := range unCommitted), and any Handler.Fail(...) call. Separately, this return false is commented "// Continue with other items", but the other return false sites in this function say "// Stop processing this batch, it will be retried" — so this actually stops the batch, contradicting the comment. If the intent really is to continue, it should be return true; and the duplicate append should be dropped either way.

[pilotso11]

/revbot review

[adv-reviewer]

🔎 zai-reviewer — reviewed f015273, found 5 issues.

[pilotso11]

🤖 revbot automated review

✅ Review posted — see below.

[adv-reviewer]

When the post-save re-fetch fails, the item is appended to unCommitted a second time: it was already appended at line 328 after the successful Save, and line 343 appends it again inside the error branch. len(unCommitted) is then inflated, so the recoverable-commit message at line 401 ("%d items will be retried") and the rollback message at line 391 ("%d items in batch marked for retry") report the wrong count, and Handler.Fail(ctx, err, unCommitted...) at lines 381/413 receives duplicate items — a custom Fail handler (dead-letter, alerting) would process the same item twice.

[adv-reviewer]

The post-save Find error branch has two comment/code mismatches that mislead anyone acting on the diagnostics. Line 341 says "We still count this as a 'success' for the DB write part" but success++ is at line 368, which this branch never reaches, so the success counter stays 0 and the final log at line 418 reports "flushed 0 records" even though the record was saved and committed. Line 344 says return false // Continue with other items, but return false from the Range callback stops iteration, so remaining dirty items are not processed in this cycle. The DirtyWrites stat (line 325) and the log message disagree.

[adv-reviewer]

For the root module, find . -name go.mod returns ./go.mod, so moddir=$(dirname ...) is . and cd "$moddir" is a no-op. The subsequent cat profile.tmp >> ../coverage.tmp.out (line 46) and >> ../output.txt (line 93) then resolve to the parent of the repo root, not to the repo's own coverage.tmp.out/output.txt. The root module's coverage and benchmark output is silently written outside the workspace and never merged, so the coverage badge and benchmark data only reflect the nested modules (lazygormcache). The root package — which contains all of the cache code — is unmeasured.

[adv-reviewer]

go test -bench=./... -benchmem passes ./... as the value of the -bench flag (a regex over benchmark names), with no package list argument. The / in the pattern makes Go treat it as a top-level-name/sub-benchmark filter; because none of the benchmarks in this repo define sub-benchmarks, the regex matches nothing and no benchmarks run. The adjacent comment ("Run benchmarks for all packages in the module") describes the intent, not what the command does. The benchmark action then receives empty data and fail-on-alert/comment-on-alert have nothing to act on.

[adv-reviewer]

The lockfree saveDirtyToDB now issues a second Handler.Find (line 333) inside the transaction for every successfully saved item, on top of the pre-existing pre-save Find at line 301. That takes each dirty item from 2 DB round-trips (Find+Save) to 3 (Find+Save+Find), which is a material write-path regression for a library whose README sells itself on throughput. The change is also asymmetric: the non-lockfree LazyWriterCache.saveDirtyToDB in lazywritecache.go (lines 307-370) was not touched and still updates the cache from the saved item with no post-save re-fetch, so the two implementations now diverge in both performance and post-save consistency behavior. For GORM, Save already populates auto-generated fields on the model, so the extra Find buys little in the primary use case.

[pilotso11]

🤖 revbot automated review

Recommendation: Fix ❌

CI is red — both the build and CodeQL checks fail (the Analyze (go)/Analyze (actions) CodeQL jobs pass, but the Go build does not). The run logs have expired (HTTP 410) so I can't cite the exact compiler line from CI, and per my brief I don't build locally — but a failing go build ./... on a module-split PR is a hard blocker regardless. The most productive next step is to reproduce the build failure locally (find . -name go.mod -not -path "./vendor/*" | while read m; do (cd "$(dirname "$m")" && go build ./...); done, the exact command the workflow runs) and fix the compilation error before this can merge.

CI status: ❌ failing — build: exit code 1 (log expired, exact error unavailable); CodeQL: also failing.

On the substantive code change (mixed into this PR): the title says "gorm in dedicated package", but the diff also rewrites the flush path in lazywritecache_lockfree.go — a new re-fetch-after-save step (Handler.Find after Save to capture DB-generated fields, then cache itemAfterSave) and a rework of the commit/save error handling. The module split itself looks coherent: root drops gorm/postgres/sqlmock/dburl (verified no root .go file still imports them), lazygormcache/ and examples/ become separate modules with a go.work, package declarations all match their import paths, and the example is self-contained. But bundling a refactor with non-trivial flush-logic changes makes the behavior change easy to miss under the "package" framing; I'd suggest splitting them so the flush changes get isolated review.

The re-fetch-after-save idea is reasonable (the cache previously stored the pre-save item, missing DB-generated IDs/timestamps), and moving the recoverable-commit log from Warn to Info + dropping the redundant trailing warn is a sensible cleanup. But the new re-fetch-failure branch has a real correctness bug — see the inline comment: item is appended to unCommitted twice, and return false stops the whole batch while the comment claims it continues. TestFlushWithErrOnLoadLF passes despite this because the NoOp commit is indifferent to duplicates and the test only checks dirty.Size()/warnCount, so the double-append is latent.

The noop_readerwriter_lockfree.go change (Find returning nil instead of "not found") is a deliberate test-handler adjustment to make the new re-fetch flow exercise the merge path, which is fine — but it does change what the NoOp handler semantically reports, so anyone relying on the old "not found" behavior in their own tests should note it.

(Recommendation: fix — submitted as a comment because GitHub does not allow self-approval/self-request-changes on your own PR.)

[pilotso11]

Severity: Medium — item is appended to unCommitted twice on re-fetch failure, and return false contradicts its own "Continue with other items" comment.

item was already appended to unCommitted at line 328 (right after Save succeeded), so this error branch appends it a second time. Since fail is not incremented on this path, execution proceeds to CommitTx carrying the duplicate, which inflates len(unCommitted) in the commit/rollback log lines and double-counts the item in Handler.Fail(...) on the unrecoverable-commit branch. Separately, return false stops the dirty.Range iteration entirely (the callback's bool means continue/stop), so one transient re-fetch failure abandons every remaining item in the batch — the opposite of what the inline comment says. If the intent is to commit the saved item and keep processing the batch, this should be return true with the second append removed; if stopping is intended, both the comment and the duplicate append are wrong.

[oshrevbot]

🤖 revbot automated review

✅ Review posted — see below.

[oshrevbot]

🤖 revbot automated review

Recommendation: Fix ❌

The module split itself is sound — gorm is cleanly extracted into a new lazygormcache module, the root module drops its gorm/dburl/sqlmock deps, and a go.work ties root + lazygormcache + examples together with local replace directives. The multi-module build/test/benchmark loop in the workflow is a reasonable way to exercise all three modules. However, this PR bundles a non-trivial logic rewrite of saveDirtyToDB alongside the refactor, and it ships with a failing build, a new CodeQL high alert, and a concrete double-append bug in the core write path — those need to be resolved before merge.

CI status: ❌ failing —

• build (exit code 1; the run's logs have expired so the exact compiler/setup error isn't retrievable, but see the setup-go note below for the most likely cause).
• CodeQL: 1 new alert, high severity — the workflow uses a known-vulnerable action (tj-actions/verify-changed-files@v16); see inline.

Two correctness issues in saveDirtyToDB and the CI causes are detailed in the inline comments.

[oshrevbot]

Severity: Medium — item is appended to unCommitted twice in this branch.

It was already appended unconditionally a few lines above (line 328). This duplicate append inflates len(unCommitted), so the retry/rollback log messages ("%d items ... marked for retry" / "%d items will be retried") over-count, and on an unrecoverable commit failure c.Handler.Fail(ctx, err, unCommitted...) is called with the item duplicated, double-reporting data loss. The comment here ("Add to unCommitted for transaction management") reads as if the item weren't already present — the author likely didn't realize the earlier append covers it. Drop this second append.

Related: when the re-fetch fails you return false (aborting the rest of the batch) without incrementing success or fail, yet the save is committed — so the final "flushed %d records successfully" log under-reports the count of actually-committed records. Worth deciding deliberately whether a non-fatal re-fetch error should stop the whole batch and skip the success tally.

[oshrevbot]

Severity: High — setup-go@v5 has no version source.

The previous go-version: 1.23 was dropped and no go-version-file was added, so the with: block is now empty. actions/setup-go requires one of go-version / go-version-file; without it the step fails (and even if it fell back to a default, the toolchain would be nondeterministic vs. the go 1.23.10 / toolchain go1.24.3 declared in the module files). This is the most probable cause of the build job's exit-code-1 failure. Add with: go-version-file: go.mod (or an explicit go-version).

[oshrevbot]

Severity: Medium — tj-actions/verify-changed-files@v16 is a known-vulnerable action (this is the CodeQL high alert).

This PR didn't introduce v16 — it was already pinned here on main — but you're reformatting these exact lines in this change, so it's a natural place to bump it. CodeQL recommends v17+. Given the broader tj-actions/* supply-chain compromise history, pinning to the fixed major (@v17) is worth doing here rather than leaving the red alert open.

[oshrevbot]

🤖 prysm automated review

🔧 Reviewed — fix requested: duplicate unCommitted append on re-fetch failure, re-fetch-failure commits but skips cache update (unresolved todo), and CI coverage drops the root module.

[oshrevbot]

Summary

Three things in one PR: (1) splits the repo into a Go workspace — root module, new lazygormcache/ module (gorm deps moved out of the core), and an examples/ module; (2) rewrites .github/workflows/go.yml to build/test/benchmark each module and merge coverage; (3) reworks saveDirtyToDB to re-fetch each item inside the tx after save (to pick up DB-generated fields) and tightens the commit-error path.

The module split and the commit-error refactor are solid. The saveDirtyToDB re-fetch and the CI coverage step have real bugs, though — requesting changes on those.

Issues

1. Duplicate append to unCommitted on re-fetch failure (lazywritecache_lockfree.go). When Save succeeds the item is appended to unCommitted (line 328), then if the post-save Find fails it's appended again (line 343). unCommitted then holds the same item twice. On a later unrecoverable commit or rollback error this flows into c.Handler.Fail(ctx, err, unCommitted...) — the data-loss callback gets the item twice, so a Handler that reports/alerts per-item would double-report the loss. Drop the second append (the first already covers transaction management).

2. Re-fetch failure commits the write but skips the cache update — cache/DB divergence (line 335, the // todo: rollback here?). The re-fetch exists precisely to merge DB-generated fields (auto IDs, versions, timestamps) back into the cache. But on re-fetch failure the code logs a warning and lets fail stay 0, so execution falls through to CommitTx and commits anyway, leaving the cache holding the pre-save item without the merged DB fields. The consistency invariant the re-fetch was added for is silently violated in the one path where it matters. The // todo should be resolved before merge: either roll back (treat as recoverable, retry the batch) or document explicitly that the cache may be stale and CopyKeyDataFrom must tolerate it. Also the comment on line 344 (return false // Continue with other items) is backwards — return false stops the Range, it doesn't continue.

3. CI coverage merge drops the root module's coverage and leaks a file outside the checkout (.github/workflows/go.yml, "Test all modules" step). I reproduced the loop logic: for the root module moddir is ., so cat profile.tmp >> ../coverage.tmp.out writes to the parent of the checkout, not the repo root — only lazygormcache's coverage reaches the awk/go tool cover step. The coverage badge therefore reflects just lazygormcache, not the core library (which has the bulk of the tests). Fix by writing to an absolute/relative repo-root path (e.g. $(git rev-parse --show-toplevel)/coverage.tmp.out) instead of ../.

Assessment

The good: workspace + replace directives are consistent, the commit-error branch dedup is cleaner, noop_readerwriter_lockfree.go Find now returns (template, nil) on the not-found path which matches the new re-fetch assumption, and the new TestFlushWithErrOnLoadLF + TestCacheAction_String cover the new behavior. Note the new test passes despite the duplicate append because its commit succeeds — so it won't catch issue #1 in a failure path.

Recommendation

Fix. Resolve #1 (drop duplicate append), #2 (decide the rollback behavior), and #3 (coverage path) before merge.

[oshrevbot]

Duplicate append: item was already appended at line 328 after Save succeeded. This second append puts the same item in unCommitted twice, which flows into c.Handler.Fail(ctx, err, unCommitted...) on a later unrecoverable commit/rollback — double-reporting the same item as data loss. Drop this append (the first already covers transaction management).

[oshrevbot]

Resolve this todo. fail isn't incremented here, so execution falls through to CommitTx and commits the write — but the cache update is skipped, so the cache holds the pre-save item without DB-generated fields. That's exactly the divergence the re-fetch was added to prevent. Either roll back (re-queue as recoverable and retry) or explicitly document that the cache may be stale. Also the return false // Continue with other items comment on the next line is wrong — return false stops the Range.