Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion lua/codereview/api/client.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ local curl = require("plenary.curl")
local async = require("plenary.async")
local async_util = require("plenary.async.util")
local log = require("codereview.log")
local cache = require("codereview.utils.cache")
Comment thread
afewyards marked this conversation as resolved.
local M = {}

local DEFAULT_TIMEOUT = 30000 -- 30 seconds

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit much


local function build_headers(token, token_type)
if token_type == "oauth" then
return {
Expand Down Expand Up @@ -95,8 +98,18 @@ function M.request(method, base_url, path, opts)
end

local params = build_params(method, base_url, path, opts)
params.timeout = opts.timeout or DEFAULT_TIMEOUT
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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lksdjsjf

local cached = cache.get("http:" .. params.url)
if cached then
log.debug(string.format("CACHE HIT %s", params.url))
return cached
Comment on lines +107 to +109

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lsjdflksjdf

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lskjd fsdfj

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sldkjfslkfj

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sldfkjslkdfj

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lalal

end
end

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check this


local response = curl.request(params)
if not response then
log.error(string.format("REQ %s %s — no response", method:upper(), params.url))
Expand All @@ -119,7 +132,18 @@ function M.request(method, base_url, path, opts)
return nil, err
end

return process_response(response)
local result = process_response(response)

-- Cache successful GET responses
if method == "get" and result then
local cfg = require("codereview.config").get()
Comment on lines +137 to +139

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lksjdflksjfl lkj l

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)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sldjflsjfd

end
end

return result
end

function M.async_request(method, base_url, path, opts)
Expand Down
19 changes: 19 additions & 0 deletions lua/codereview/config.lua
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ local defaults = {
diff = { context = 8, scroll_threshold = 50 },
ai = { enabled = true, claude_cmd = "claude", agent = "code-review" },
keymaps = {},
notifications = {
enabled = true,
timeout = 3000, -- ms before notification auto-dismisses
position = "top_right", -- "top_right" | "bottom_right" | "top_left"
},
cache = {
enabled = true,
ttl = 300, -- seconds to cache API responses
},
}

local current = nil
Expand All @@ -29,6 +38,16 @@ end

local function validate(c)
c.diff.context = math.max(0, math.min(20, c.diff.context))
if c.notifications then

@afewyards afewyards Feb 24, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I should do this

c.notifications.timeout = math.max(500, c.notifications.timeout or 3000)
Comment thread
afewyards marked this conversation as resolved.
local valid_positions = { top_right = true, bottom_right = true, top_left = true }
if not valid_positions[c.notifications.position] then
c.notifications.position = "top_right"
end
end
if c.cache then
Comment thread
afewyards marked this conversation as resolved.
Comment on lines +47 to +48

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nono

c.cache.ttl = math.max(0, c.cache.ttl or 300)
end
return c
end

Expand Down
14 changes: 13 additions & 1 deletion lua/codereview/log.lua
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ local config_mod = require("codereview.config")

local LEVELS = { DEBUG = 1, INFO = 2, WARN = 3, ERROR = 4 }
local NAMES = { "DEBUG", "INFO", "WARN", "ERROR" }
local MAX_LOG_SIZE = 1024 * 1024 -- 1 MB

local function enabled()
local c = config_mod.get()
Expand All @@ -17,11 +18,22 @@ local function log_path()
return vim.fn.stdpath("cache") .. "/codereview.log"
end

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"

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

os.remove(rotated)
os.rename(path, rotated)
end

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_statos.removeos.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

end

local function write(level, msg)
if not enabled() then return end
local path = log_path()
rotate_if_needed(path)
local ts = os.date("%Y-%m-%d %H:%M:%S")
local line = string.format("[%s] %s %s\n", ts, NAMES[level] or "?", msg)
local f = io.open(log_path(), "a")
local f = io.open(path, "a")
if f then
f:write(line)
f:close()
Expand Down
79 changes: 79 additions & 0 deletions lua/codereview/utils/cache.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
--- Simple TTL cache for API responses.
local M = {}

---@class CacheEntry
---@field value any
---@field expires_at number

---@type table<string, CacheEntry>
local store = {}

--- Store a value with a time-to-live in seconds.
---@param key string
---@param value any
---@param ttl_seconds number
function M.set(key, value, ttl_seconds)
store[key] = {
value = value,
expires_at = os.time() + ttl_seconds,
}
end

--- Retrieve a cached value. Returns nil if expired or absent.
---@param key string
---@return any|nil
function M.get(key)
local entry = store[key]
if not entry then return nil end
if os.time() > entry.expires_at then
store[key] = nil
return nil
end
return entry.value
end

--- Remove a single key from the cache.
---@param key string
function M.invalidate(key)
store[key] = nil
end

--- Flush all cached entries.
function M.flush()
store = {}
end

--- Return the number of live (non-expired) entries.
---@return number
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
Comment on lines +48 to +59

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not use this

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lsjfsd jlskdjf


--- Wrap an expensive function with caching.
---@param fn fun(...): any
---@param key_fn fun(...): string Function that derives the cache key from args
---@param ttl number TTL in seconds
---@return fun(...): any
function M.memoize(fn, key_fn, ttl)
return function(...)
local key = key_fn(...)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

local cached = M.get(key)
if cached ~= nil then return cached end
local result = fn(...)
if result ~= nil then
M.set(key, result, ttl)
end
return result
end
end

return M
87 changes: 87 additions & 0 deletions lua/codereview/utils/strings.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
--- String utilities for codereview.nvim
local M = {}

--- Truncate a string to max_len, appending an ellipsis if truncated.
---@param s string
---@param max_len number
---@return string
function M.truncate(s, max_len)
if #s <= max_len then return s end
return s:sub(1, max_len - 1) .. "…"
end

--- Strip leading and trailing whitespace.
---@param s string
---@return string
function M.trim(s)
return s:match("^%s*(.-)%s*$")
end

--- Split a string by a delimiter pattern.
---@param s string
---@param sep string Pattern to split on (e.g. "\n")
---@return string[]
function M.split(s, sep)
local parts = {}
for part in s:gmatch("([^" .. sep .. "]+)") do

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: sep is interpolated directly into a Lua character class [^...], not used as a standalone pattern. This causes two problems:

  1. 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.
  2. Multi-char delimiters don't work: Splitting on ", " splits on each of , and individually, not on the two-character sequence.

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

table.insert(parts, part)
end
return parts
end

--- Wrap text to a maximum line width, breaking on word boundaries.
---@param text string
---@param width number
---@return string
function M.wrap(text, width)
local lines = {}
for _, paragraph in ipairs(M.split(text, "\n")) do
local line = ""
for word in paragraph:gmatch("%S+") do
if #line + #word + 1 > width and #line > 0 then
table.insert(lines, line)
line = word
else
line = #line > 0 and (line .. " " .. word) or word
end
end
if #line > 0 then table.insert(lines, line) end
end
return table.concat(lines, "\n")
end

--- Escape special Lua pattern characters in a string.
---@param s string
---@return string
function M.escape_pattern(s)
return s:gsub("([%(%)%.%%%+%-%*%?%[%]%^%$])", "%%%1")
end

--- Check whether a string starts with a given prefix.
---@param s string
---@param prefix string
---@return boolean
function M.starts_with(s, prefix)
return s:sub(1, #prefix) == prefix
end

--- Check whether a string ends with a given suffix.
---@param s string
---@param suffix string
---@return boolean
function M.ends_with(s, suffix)
return suffix == "" or s:sub(-#suffix) == suffix
end

--- Pad a string on the right to reach the desired width.
---@param s string
---@param width number
---@param char? string Padding character (default: space)
---@return string
function M.pad_right(s, width, char)
char = char or " "
if #s >= width then return s end
return s .. string.rep(char, width - #s)
end

return M
Loading