feat: add #4
Conversation
- Add utils/cache.lua: TTL-based cache with memoize helper - Add utils/strings.lua: truncate, trim, split, wrap, pad utilities - config: add notifications and cache configuration sections - log: add log rotation when file exceeds 1MB - client: integrate response caching for GET requests
| local cache = require("codereview.utils.cache") | ||
| local M = {} | ||
|
|
||
| local DEFAULT_TIMEOUT = 30000 -- 30 seconds |
|
|
||
| local function validate(c) | ||
| c.diff.context = math.max(0, math.min(20, c.diff.context)) | ||
| if c.notifications then |
There was a problem hiding this comment.
No type check before math.max. If a user supplies a non-number value (e.g. timeout = "fast"), the or 3000 fallback only triggers for nil/false, so math.max(500, "fast") will throw a runtime error.
Add a tonumber coercion or type guard:
local t = tonumber(c.notifications.timeout) or 3000
c.notifications.timeout = math.max(500, t)There was a problem hiding this comment.
not sure I should do this
| local rotated = path .. ".1" | ||
| os.remove(rotated) | ||
| os.rename(path, rotated) | ||
| end |
There was a problem hiding this comment.
os.rename return value is not checked. If rename fails (e.g., permissions error), rotation silently breaks and every subsequent write() call will re-attempt the failing fs_stat → os.remove → os.rename sequence, adding overhead to every log call with no user-visible indication.
Consider:
local ok, err = os.rename(path, rotated)
if not ok then
-- optionally write a one-time warning to stderr or just bail
return
end
| log.debug(string.format("CACHE HIT %s", params.url)) | ||
| return cached | ||
| end | ||
| end |
| ---@return string[] | ||
| function M.split(s, sep) | ||
| local parts = {} | ||
| for part in s:gmatch("([^" .. sep .. "]+)") do |
There was a problem hiding this comment.
Bug: sep is interpolated directly into a Lua character class [^...], not used as a standalone pattern. This causes two problems:
- Pattern injection: Characters meaningful inside
[]break the pattern. Passing"]"or"^"corrupts the regex; passing"a-z"is interpreted as a character range, silently splitting on all lowercase letters. - Multi-char delimiters don't work: Splitting on
", "splits on each of,and
Consider using string.find in a loop with a plain-string search, or at minimum escape sep and document that it is a character set, not a pattern:
function M.split(s, sep)
local parts = {}
local start = 1
while start <= #s do
local i, j = s:find(sep, start, true) -- plain match
if not i then
table.insert(parts, s:sub(start))
break
end
table.insert(parts, s:sub(start, i - 1))
start = j + 1
end
return parts
end| ---@return fun(...): any | ||
| function M.memoize(fn, key_fn, ttl) | ||
| return function(...) | ||
| local key = key_fn(...) |
There was a problem hiding this comment.
If key_fn returns nil, M.get(nil) on line 69 silently returns nil (Lua allows reading t[nil]), but the subsequent M.set(nil, result, ttl) on line 73 will throw a runtime error: "table index is nil" (Lua forbids t[nil] = v).
This creates a confusing failure path: the cache miss looks normal, the original fn executes, and only then does it crash on the set call.
Guard with:
local key = key_fn(...)
if key == nil then return fn(...) end
| end | ||
| if c.cache then |
|
kjsdh fkjshd fjskd fsjdhf kjsdh fkjshd fjskd fsjdhf kjsdh fkjshd fjskd fsjdhf |
|
kjsdh fkjshd fjskd fsjdhf |
| local function rotate_if_needed(path) | ||
| local stat = vim.loop.fs_stat(path) | ||
| if stat and stat.size > MAX_LOG_SIZE then | ||
| local rotated = path .. ".1" |
There was a problem hiding this comment.
Only a single rotated backup (.1) is kept. If another Neovim instance is concurrently writing logs, there is a small window where instance A calls os.remove on .1, then instance B also enters rotation and calls os.rename, clobbering instance A's just-rotated file. In a single-instance scenario this is fine, but worth noting if multi-instance use is expected.
| log.debug(string.format("REQ %s %s", method:upper(), params.url)) | ||
|
|
||
| -- Check cache for GET requests | ||
| if method == "get" and not opts.no_cache then |
afewyards
left a comment
afewyards
left a comment
There was a problem hiding this comment.
lkdjlfsjdlkjsflkjsdflj
| local cfg = require("codereview.config").get() | ||
| local ttl = (cfg.cache and cfg.cache.enabled) and (cfg.cache.ttl or 300) or 0 | ||
| if ttl > 0 then | ||
| cache.set("http:" .. params.url, result, ttl) |
| if cached then | ||
| log.debug(string.format("CACHE HIT %s", params.url)) | ||
| return cached |
There was a problem hiding this comment.
It is a long established fact that a reader will be distracted by the readable content of a page when looking at its layout. The point of using Lorem Ipsum is that it has a more-or-less normal distribution of letters, as opposed to using 'Content here, content here', making it look like readable English. Many desktop publishing packages and web page editors now use Lorem Ipsum as their default model text, and a search for 'lorem ipsum' will uncover many web sites still in their infancy. Various versions have evolved over the years, sometimes by accident, sometimes on purpose (injected humour and the like).
| -- Cache successful GET responses | ||
| if method == "get" and result then | ||
| local cfg = require("codereview.config").get() |
| function M.size() | ||
| local count = 0 | ||
| local now = os.time() | ||
| for k, entry in pairs(store) do | ||
| if now > entry.expires_at then | ||
| store[k] = nil | ||
| else | ||
| count = count + 1 | ||
| end | ||
| end | ||
| return count | ||
| end |
afewyards
changed the title
1 participant
Summary
utils/cache.luamodule with TTL-based caching and amemoize()wrapper. Integrated intoapi/client.luafor automatic GET request caching.utils/strings.luawith common helpers —truncate,trim,split,wrap,pad_right,starts_with,ends_with,escape_pattern.notificationssection to config withenabled,timeout, andpositionoptions.Motivation
API responses are fetched repeatedly when navigating between files in a PR. Caching reduces redundant network calls and improves perceived performance. String utilities consolidate scattered inline string operations.
Test plan