Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 2377717 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
.changeset/afraid-states-sin.md:5Vague changeset message doesn't follow AGENTS.md guidelines
💭 Consider (1) 💭
💭 1) append-hash-for-server-only-packages.mts:17-34 Add error handling for directory operations
Issue: The script reads directories without error handling. If .next/node_modules doesn't exist (e.g., build failed or script ran from wrong directory), the script will crash with an unhelpful ENOENT error during prepublishOnly.
Why: Build scripts that run during publishing should fail gracefully with actionable error messages to help developers diagnose issues quickly.
Fix: Consider wrapping the fs.readdir() calls in try-catch blocks with descriptive error messages:
let dirs: string[];
try {
dirs = await fs.readdir(NODE_MODULES_PATH);
} catch (error) {
console.error(`Failed to read ${NODE_MODULES_PATH}. Ensure 'pnpm build' was run first.`);
process.exit(1);
}This is optional since the script runs in a controlled build environment where the directory should always exist after a successful build.
Refs:
💡 APPROVE WITH SUGGESTIONS
Summary: This is a solid bug fix that correctly handles scoped npm packages in the Turbopack hash-appending workflow. The code logic is correct and addresses a real issue with @opentelemetry/* packages in serverExternalPackages. The main suggestion is to improve the changeset message to be more descriptive per project guidelines. The optional error handling improvement is a nice-to-have for better developer experience during failed publishes. 🚀
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
append-hash-for-server-only-packages.mts:22-28 |
Empty scoped package directories edge case | Speculative - Next.js controls what goes into .next/node_modules, so this edge case is unlikely to occur in practice. Not actionable. |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-devops |
3 | 0 | 1 | 0 | 0 | 0 | 1 |
| Total | 4 | 0 | 1 | 0 | 1 | 0 | 1 |
Note: One finding (vague changeset message) was raised by both reviewers and merged.
| "@inkeep/agents-manage-ui": patch | ||
| --- | ||
|
|
||
| script fix for manage ui |
There was a problem hiding this comment.
🟡 Minor: Vague changeset message
Issue: The changeset message "script fix for manage ui" doesn't describe what was fixed or the user impact.
Why: Per AGENTS.md, changelog messages should use sentence case, start with an action verb (Fix, Add, Update, etc.), and be specific about what changed and why it matters to consumers. The current message resembles the "fix bug" anti-pattern called out in the guidelines.
Fix: (1-click apply)
| script fix for manage ui | |
| Fix prepublish script to handle scoped packages in standalone build output |
Refs:
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
Delta re-review — This review covers only changes since the last automated review (2 commits:
e5d8ecdc4and237771764).
💭 Consider (1) 💭
💭 1) instrumentation.ts:68-70 Add code comment explaining disabled instrumentations
Issue: The newly disabled instrumentations (fs, dns, net) lack a comment explaining why they're disabled.
Why: Future maintainers may wonder why these are disabled or accidentally re-enable them. A brief comment prevents confusion and documents the intentional trade-off.
Fix: Optional — add a comment above the disabled instrumentations:
// Disable noisy low-level instrumentations that generate high span volumes
// without proportional debugging value. HTTP/undici remain enabled for
// external call visibility. See also: agents-manage-ui/src/otel.ts
'@opentelemetry/instrumentation-fs': { enabled: false },
'@opentelemetry/instrumentation-dns': { enabled: false },
'@opentelemetry/instrumentation-net': { enabled: false },Refs:
- agents-manage-ui/src/otel.ts:68-70 — existing pattern
🕐 Pending Recommendations (1)
- 🟡
.changeset/lazy-ducks-switch.md:6— Changeset message is vague and now incomplete (coversagents-apitoo but message only mentions manage-ui)
💡 APPROVE WITH SUGGESTIONS
Summary: The delta changes are solid! Disabling fs/dns/net OpenTelemetry auto-instrumentations follows the established pattern in agents-manage-ui and is a reasonable trade-off to reduce trace noise and fix tests. HTTP instrumentation remains enabled for meaningful external call visibility. The only outstanding item is the changeset message from the prior review — now that the changeset covers both @inkeep/agents-manage-ui and @inkeep/agents-api, the message should be updated to reflect both changes. 🎯
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
instrumentation.ts:68-70 |
Pattern alignment validation | Positive observation, not an issue — confirms the change follows existing patterns |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
2 | 0 | 0 | 0 | 0 | 1 | 1 |
pr-review-sre |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
| Total | 3 | 0 | 1 | 0 | 0 | 1 | 1 |
1 participant
No description provided.