feat(session): add request/response headers logging with Tab UI#420
feat(session): add request/response headers logging with Tab UI#420
Conversation
- Add headersToSanitizedObject and parseHeaderRecord helpers in session-manager.ts - Store sanitized request/response headers to Redis with SESSION_TTL - Extend getSessionDetails API to return requestHeaders/responseHeaders - Refactor session details UI from vertical layout to 4-tab layout - Add i18n keys for 5 locales (en, ja, ru, zh-CN, zh-TW) - Add input validation for seq URL parameter to prevent NaN issues Closes #417 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! 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 significantly enhances the session monitoring capabilities by introducing the storage and display of both request and response headers. The session details page now features a more organized tabbed interface, allowing users to easily inspect header information alongside message and body content. A key security consideration has been addressed by implementing automatic sanitization of sensitive header fields before storage, and all header data is managed with a time-to-live in Redis to ensure efficient resource usage. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
github-actions
bot
added
enhancement
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logging for request and response headers, and refactors the session details UI to a tab-based layout. The changes are well-structured and the new UI looks like a good improvement. I've identified a couple of areas with duplicated code and opportunities for simplification and performance improvement, which are detailed in the comments.
src/actions/active-sessions.ts
Outdated
| function normalizeRequestSequence(requestSequence?: number): number | undefined { | ||
| if (typeof requestSequence !== "number") return undefined; | ||
| if (!Number.isFinite(requestSequence)) return undefined; | ||
| if (!Number.isInteger(requestSequence)) return undefined; | ||
| if (requestSequence <= 0) return undefined; | ||
| return requestSequence; | ||
| } |
There was a problem hiding this comment.
The validation logic in normalizeRequestSequence can be made more concise by combining the conditions into a single if statement. The !Number.isFinite(requestSequence) check is also redundant, as Number.isInteger() already ensures the number is finite.
Additionally, this function is duplicated in src/lib/session-manager.ts. Consider creating a single shared utility function to avoid code duplication and ensure consistency.
function normalizeRequestSequence(requestSequence?: number): number | undefined {
if (typeof requestSequence !== "number" || !Number.isInteger(requestSequence) || requestSequence <= 0) {
return undefined;
}
return requestSequence;
}
src/lib/session-manager.ts
Outdated
| function normalizeRequestSequence(requestSequence?: number): number | null { | ||
| if (typeof requestSequence !== "number") return null; | ||
| if (!Number.isFinite(requestSequence)) return null; | ||
| if (!Number.isInteger(requestSequence)) return null; | ||
| if (requestSequence <= 0) return null; | ||
| return requestSequence; | ||
| } |
There was a problem hiding this comment.
This normalizeRequestSequence function is a near-duplicate of the one in src/actions/active-sessions.ts. To improve maintainability, this logic should be extracted to a shared utility file.
The validation can also be made more concise by combining the conditions. The !Number.isFinite() check is redundant because Number.isInteger() already covers it.
function normalizeRequestSequence(requestSequence?: number): number | null {
if (typeof requestSequence !== "number" || !Number.isInteger(requestSequence) || requestSequence <= 0) {
return null;
}
return requestSequence;
}| function headersToSanitizedObject(headers: Headers): Record<string, string> { | ||
| const sanitizedText = sanitizeHeaders(headers); | ||
| if (!sanitizedText || sanitizedText === "(empty)") { | ||
| return {}; | ||
| } | ||
|
|
||
| const obj: Record<string, string> = {}; | ||
| const lines = sanitizedText.split(/\r?\n/).filter(Boolean); | ||
| for (const line of lines) { | ||
| const colonIndex = line.indexOf(":"); | ||
| if (colonIndex === -1) continue; | ||
| const name = line.slice(0, colonIndex).trim(); | ||
| const value = line.slice(colonIndex + 1).trim(); | ||
| if (!name) continue; | ||
|
|
||
| if (obj[name]) { | ||
| obj[name] = `${obj[name]}\n${value}`; | ||
| } else { | ||
| obj[name] = value; | ||
| } | ||
| } | ||
|
|
||
| return obj; | ||
| } |
There was a problem hiding this comment.
This function's implementation is a bit indirect. It converts a Headers object to a string using sanitizeHeaders, and then parses that string back into a Record<string, string>. This Headers -> string -> Record conversion can be inefficient.
A more direct approach would be to iterate over the Headers object and build the sanitized record directly. This would likely require exporting SENSITIVE_HEADERS, maskSensitiveValue, and maskAuthorizationValue from src/app/v1/_lib/proxy/errors.ts to be used here. This would improve performance and code clarity.
Here's an example of a more direct implementation:
function headersToSanitizedObject(headers: Headers): Record<string, string> {
const obj: Record<string, string> = {};
headers.forEach((value, key) => {
const lowerKey = key.toLowerCase();
// Assuming SENSITIVE_HEADERS and mask functions are available
const sanitizedValue = SENSITIVE_HEADERS.has(lowerKey)
? lowerKey === "authorization"
? maskAuthorizationValue(value)
: maskSensitiveValue(value)
: value;
// The forEach method handles multi-valued headers like Set-Cookie correctly
if (obj[key]) {
obj[key] += `\n${sanitizedValue}`;
} else {
obj[key] = sanitizedValue;
}
});
return obj;
}
github-actions
bot
added
size/M
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR adds request/response headers logging and display functionality with a well-structured Tab UI. The implementation follows existing codebase patterns for Redis storage and correctly reuses the existing sanitizeHeaders function for sensitive data protection.
PR Size: M
- Lines changed: 393 (325 additions + 68 deletions)
- Files changed: 9
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority Issues (Should Consider)
-
[TEST-MISSING-CRITICAL]
src/lib/session-manager.ts- New methods lack unit testsThe PR adds 4 new public methods (
storeSessionRequestHeaders,storeSessionResponseHeaders,getSessionRequestHeaders,getSessionResponseHeaders) and 3 helper functions without corresponding test coverage. While the implementation correctly follows existing patterns in the codebase, unit tests would ensure:normalizeRequestSequencehandles edge cases correctlyparseHeaderRecordproperly handles malformed JSON- Redis operations are mocked and verified
Note: This is not blocking as the codebase's test coverage for session-manager.ts appears limited overall, but adding tests would improve confidence in the new functionality.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean (sensitive headers are properly sanitized via existing
sanitizeHeadersfunction) - Error handling - Clean (errors are logged and fail gracefully)
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Missing (see above)
- Code clarity - Good
Positive Observations (Not counted as review items per instructions)
The implementation correctly:
- Reuses existing
sanitizeHeadersfunction fromproxy/errors.ts - Follows the existing pattern of other Redis storage methods in
SessionManager - Properly handles Redis unavailability with early returns
- Uses fire-and-forget pattern (
.catch()) for header storage to avoid blocking the request
Automated review by Claude AI
There was a problem hiding this comment.
Code Review Summary
This PR adds request/response headers logging and display functionality with a well-structured Tab UI. The implementation follows existing codebase patterns for Redis storage and correctly reuses the existing sanitizeHeaders function for sensitive data protection.
PR Size: M
- Lines changed: 393 (325 additions + 68 deletions)
- Files changed: 9
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority Issues (Should Consider)
-
[TEST-MISSING-CRITICAL]
src/lib/session-manager.ts- New methods lack unit testsThe PR adds 4 new public methods (
storeSessionRequestHeaders,storeSessionResponseHeaders,getSessionRequestHeaders,getSessionResponseHeaders) and 3 helper functions without corresponding test coverage. While the implementation correctly follows existing patterns in the codebase, unit tests would ensure:normalizeRequestSequencehandles edge cases correctlyparseHeaderRecordproperly handles malformed JSON- Redis operations are mocked and verified
Note: This is not blocking as the codebase's test coverage for session-manager.ts appears limited overall, but adding tests would improve confidence in the new functionality.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean (sensitive headers are properly sanitized via existing
sanitizeHeadersfunction) - Error handling - Clean (errors are logged and fail gracefully)
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Missing (see above)
- Code clarity - Good
Positive Observations (Not counted as review items per instructions)
The implementation correctly:
- Reuses existing
sanitizeHeadersfunction fromproxy/errors.ts - Follows the existing pattern of other Redis storage methods in
SessionManager - Properly handles Redis unavailability with early returns
- Uses fire-and-forget pattern (
.catch()) for header storage to avoid blocking the request
Automated review by Claude AI
src/lib/session-manager.ts
Outdated
| } | ||
| } | ||
| return record; | ||
| } catch { |
There was a problem hiding this comment.
[CRITICAL] [ERROR-SILENT] JSON parse errors are swallowed when reading stored headers
Evidence: src/lib/session-manager.ts:60
} catch {
return null;
}Why this is a problem: If Redis contains invalid/legacy header JSON, the UI will silently show "No data" and you lose the signal needed to debug format drift or corruption.
Suggested fix:
function parseHeaderRecord(value: string): Record<string, string> {
const parsed: unknown = JSON.parse(value);
if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) {
throw new Error("Invalid header record JSON");
}
const record: Record<string, string> = {};
for (const [key, raw] of Object.entries(parsed as Record<string, unknown>)) {
if (typeof raw === "string") record[key] = raw;
}
return record;
}
// In getSessionRequestHeaders / getSessionResponseHeaders
try {
// ... const key = `session:${sessionId}:req:${sequence}:reqHeaders`
return parseHeaderRecord(value);
} catch (error) {
logger.warn("SessionManager: Invalid stored header record", { error, sessionId, key });
return null;
}
There was a problem hiding this comment.
Code Review Summary
This PR adds request/response header capture to Session details and refactors the UI to a 4-tab layout. The main blocker is silent JSON parse failure when reading stored headers, which can mask Redis data corruption/format drift and make missing header data hard to diagnose.
PR Size: M
- Lines changed: 393
- Files changed: 9
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 1 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
src/lib/session-manager.ts:60[ERROR-SILENT]parseHeaderRecordswallows JSON parse errors and returnsnullwithout logging (Confidence: 95)
High Priority Issues (Should Fix)
- None
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
- Reviewed PR
#420and applied labelsize/M(393 lines changed, 9 files). - Posted 1 inline blocking issue:
src/lib/session-manager.ts:60—parseHeaderRecordswallows JSON parse errors (catch { return null; }), which can hide Redis data corruption/format drift; suggested fix included in the inline comment. - Submitted the required PR review summary comment via
gh pr review.
… and add JSON parse logging - Extract normalizeRequestSequence to src/lib/utils/request-sequence.ts (DRY) - Use Number.isSafeInteger() for stronger validation (per Codex review) - Add logger.warn in parseHeaderRecord catch block for JSON parse failures - Unify return type to number | null Addresses PR #420 review feedback: - Gemini: duplicate normalizeRequestSequence functions - GitHub Actions: silent JSON parse error in parseHeaderRecord Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
- Add tests for normalizeRequestSequence (edge cases, type errors) - Add tests for parseHeaderRecord (JSON parsing, logger.warn verification) - Add tests for headersToSanitizedObject (header conversion, colon handling) - Export helper functions from session-manager.ts for testing 12 test cases covering all scenarios Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
Changes
src/lib/session-manager.tsheadersToSanitizedObject,parseHeaderRecord, and 4 store/get methodssrc/app/v1/_lib/proxy/forwarder.tssrc/actions/active-sessions.tsgetSessionDetailsto returnrequestHeaders/responseHeaderssession-messages-client.tsxHeadersDisplaycomponentmessages/*/dashboard.jsonTest plan
Closes #417
Generated with Claude Code