Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new event-based logging package "log2": configurable Logger and Config defaults, context-attached Event objects (Add/Step/Error/Finish), a rule-driven tail sampler, HTTP middleware emitting one wide request event, and comprehensive unit tests and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as HTTP Middleware
participant Logger
participant Event
participant Sampler
participant Handler as slog Handler
Client->>Middleware: HTTP Request
Middleware->>Logger: Start(ctx, "http_request", attrs)
Logger->>Event: initialize (context attrs + supplied attrs)
Middleware->>Event: Add(request/response attrs)
Note over Middleware,Event: Handler runs (deferred Finish)
alt Handler panics
Middleware->>Event: Error(panic)
end
Middleware->>Event: Finish(status, bytes)
Event->>Sampler: ShouldSample(EventView)
Sampler-->>Event: SamplingDecision{Keep,Reason}
alt Keep == true
Event->>Logger: emit(eventPayload)
Logger->>Handler: Handle(slog.Record)
end
Middleware-->>Client: HTTP Response (or re-panic)
sequenceDiagram
participant App
participant Logger
participant Event
participant Sampler as Tail Sampler
participant Handler
App->>Logger: New(Config)
Logger-->>App: *Logger (with sampler, context keys)
App->>Logger: Start(ctx, name, attrs...)
Logger->>Event: create with ctx attrs
App->>Event: Add/Step/Error...
App->>Event: Finish(attrs)
Event->>Sampler: ShouldSample(view)
Sampler->>Sampler: evaluate errors/status/slow/rules/random
Sampler-->>Event: SamplingDecision
alt Keep == true
Event->>Logger: emit(payload)
Logger->>Handler: write formatted record
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Pull Request Test Coverage Report for Build 22051299645Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@log2/context.go`:
- Around line 24-26: WithEvent currently calls context.WithValue on the incoming
ctx which will panic if ctx is nil; mirror EventFromContext's guard by checking
for nil and substituting context.Background() before calling context.WithValue.
Update the WithEvent function to do: if ctx == nil { ctx = context.Background()
} then return context.WithValue(ctx, LogEventContextKey, ev) so it no longer
panics on nil contexts.
In `@log2/http_middleware.go`:
- Around line 50-55: The middleware currently adds the client's raw r.RemoteAddr
to requestAttrs (see requestAttrs map in http_middleware.go), which may expose
PII; update the middleware to make including remoteAddr opt-in via a
configuration flag or context option (e.g., includeRemoteAddr boolean) and when
enabled redact or normalize the value (strip port with net.SplitHostPort or
replace with a hashed/pseudonymized token) before inserting into requestAttrs;
also document the opt-in behavior and default it to disabled so remote IPs are
not logged unless explicitly allowed.
- Around line 83-99: The statusRecorder wrapper currently embeds
http.ResponseWriter but doesn't forward optional interfaces, causing type
assertion failures for handlers expecting Hijacker, Flusher, or Pusher; update
the statusRecorder type to implement and forward these by adding methods
Hijack() (net.Conn, *bufio.ReadWriter, error), Flush(), and Push(target string,
opts *http.PushOptions) error that first check whether r.ResponseWriter
implements the corresponding interface and, if so, delegate the call and return
its results (otherwise return the zero-values/error as appropriate or no-op for
Flush), leaving existing methods WriteHeader and Write unchanged.
In `@log2/logger.go`:
- Around line 53-61: The package-level Default variable is written unsafely by
SetDefault and read by Start/Logger.Start; replace the raw global with an
atomic.Pointer[Logger] (e.g., var Default atomic.Pointer[Logger]) and update
SetDefault to use Default.Store(ptr) (storing a pointer returned from
New(DefaultConfig()) when l==nil or the provided l), and update all reads (Start
and Logger.Start) to use Default.Load() and check for nil before dereferencing;
ensure nil handling mirrors existing behavior so callers still get a valid
logger when none was set.
🧹 Nitpick comments (1)
log2/context.go (1)
7-8: Prefer a typed, immutable context key.
anymakes the exported key reassignable/type-unsafe from other packages, which can silently break lookups. Use the unexportedcontextKeytype directly to prevent accidental mutation.♻️ Suggested change
- var LogEventContextKey any = contextKey("platformaLog2Event") //nolint:gochecknoglobals + var LogEventContextKey = contextKey("platformaLog2Event") //nolint:gochecknoglobals
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Taskfile.dist.yml`:
- Line 27: Update documentation and/or Taskfile to keep docs build working for
contributors without bun: either change docs/README.md to require bun (mention
docs/bun.lock and add install + "bun run dev" instructions) or modify the
Taskfile docs target (replace the single "- bun run dev" invocation with a
fallback that tries bun first then falls back to npm/yarn, e.g. an OR-style or
conditional that runs "bun run dev" and if it fails runs "npm run dev" or "yarn
dev"). Ensure you reference Taskfile's docs target and docs/README.md so both
the task and the README are consistent.
🧹 Nitpick comments (1)
docs/package.json (1)
6-6: Consider avoiding unconditional--hostto reduce unintended exposure.
--hostbinds the dev server to all network interfaces (0.0.0.0), making it reachable from other devices on your LAN. While convenient, this can expose the docs server on shared networks. Consider adding a separate script (e.g.,dev:host) or documenting when to use host binding, so the defaultdevscript keeps the server localhost-only.🔧 Suggested script split
"scripts": { - "dev": "astro dev --host", + "dev": "astro dev", + "dev:host": "astro dev --host", "start": "astro dev", "build": "astro build", "preview": "astro preview", "astro": "astro" },
| dir: docs | ||
| cmds: | ||
| - npm run dev | ||
| - bun run dev |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for bun mentions in key documentation files
echo "=== Checking for bun in documentation ==="
find . -maxdepth 3 \( -name "README.md" -o -name "CONTRIBUTING.md" -o -name "package.json" \) | head -20 | while read f; do
if [ -f "$f" ]; then
echo "--- File: $f ---"
rg -i "bun" "$f" || echo "No mention of bun"
fi
done
echo -e "\n=== Checking Taskfile.dist.yml context ==="
if [ -f "Taskfile.dist.yml" ]; then
head -40 Taskfile.dist.yml
fi
echo -e "\n=== Looking for bun in all YAML files ==="
rg -i "bun" --type yaml || echo "No bun mentions in YAML files"Repository: platforma-dev/platforma
Length of output: 919
🏁 Script executed:
#!/bin/bash
# Check the overall tooling and dependencies
echo "=== Checking for package managers references ==="
rg -i "pnpm|npm|yarn|bun" package.json 2>/dev/null | head -20
echo -e "\n=== Check if there's a .node-version, .tool-versions, or similar ==="
ls -la | grep -E "^\." | head -20
echo -e "\n=== Check git history to see what changed ==="
git log --oneline -20 2>/dev/null || echo "Git history not available"Repository: platforma-dev/platforma
Length of output: 267
🏁 Script executed:
#!/bin/bash
# Check the docs directory structure and dependencies
echo "=== Docs directory structure ==="
ls -la docs/ | head -30
echo -e "\n=== Contents of docs/package.json ==="
if [ -f "docs/package.json" ]; then
cat docs/package.json
fi
echo -e "\n=== Check for lockfiles in docs ==="
ls -la docs/ | grep -E "lock|yarn|pnpm"
echo -e "\n=== Check if root has package.json ==="
if [ -f "package.json" ]; then
cat package.json
fi
echo -e "\n=== Check for any dev setup docs ==="
find . -maxdepth 2 -name "*SETUP*" -o -name "*setup*" -o -name "*DEV*" 2>/dev/null | grep -v node_modules | head -10Repository: platforma-dev/platforma
Length of output: 1371
🏁 Script executed:
#!/bin/bash
# Check the docs README for setup instructions
echo "=== Contents of docs/README.md ==="
cat docs/README.mdRepository: platforma-dev/platforma
Length of output: 1888
Update documentation to reflect bun as the required package manager for docs, or add a fallback.
The docs task now requires bun run dev, but docs/README.md still instructs contributors to use npm install and npm run dev. This mismatch will break the workflow for contributors without bun. Since docs/bun.lock confirms bun is the intended package manager, update docs/README.md to document bun as a requirement, or add a fallback command in the Taskfile to maintain npm compatibility.
🔧 Fallback option for Taskfile
docs:
dir: docs
cmds:
- - bun run dev
+ - |
+ if command -v bun >/dev/null 2>&1; then
+ bun run dev
+ else
+ npm run dev
+ fi📝 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.
Suggested change
| - bun run dev | |
| docs: | |
| dir: docs | |
| cmds: | |
| - | | |
| if command -v bun >/dev/null 2>&1; then | |
| bun run dev | |
| else | |
| npm run dev | |
| fi |
🤖 Prompt for AI Agents
In `@Taskfile.dist.yml` at line 27, Update documentation and/or Taskfile to keep
docs build working for contributors without bun: either change docs/README.md to
require bun (mention docs/bun.lock and add install + "bun run dev" instructions)
or modify the Taskfile docs target (replace the single "- bun run dev"
invocation with a fallback that tries bun first then falls back to npm/yarn,
e.g. an OR-style or conditional that runs "bun run dev" and if it fails runs
"npm run dev" or "yarn dev"). Ensure you reference Taskfile's docs target and
docs/README.md so both the task and the README are consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores