Skip to content

refactor(proxy): share the logger formatting core instead of an inlined copy - #3050

Merged
kwakayama merged 1 commit into
mainfrom
refactor/proxy-logger-shared-core
Jul 24, 2026
Merged

refactor(proxy): share the logger formatting core instead of an inlined copy#3050
kwakayama merged 1 commit into
mainfrom
refactor/proxy-logger-shared-core

Conversation

@kojiwakayama

Copy link
Copy Markdown
Contributor

Summary

Candidate #7 from the architecture review. proxy/logger.ts re-inlined ~130 lines of formatting primitives from utils/logger/core.ts behind a manual "keep in sync" comment. Net: −104 lines (+34/−138).

The gating question, answered

The inline comment claimed the proxy Docker build only copies src/proxy/ and cannot reach src/utils/. Verified stale:

  • No proxy Dockerfile exists in this repo or in veryfront-infrastructure/
  • The proxy ships inside the single deno compile binary (deno task start-split:binary)
  • proxy/*.ts already imports many #veryfront/* modules today (errors, observability, platform/compat, utils via earlier consolidations) — a proxy-only-copy build would already be broken

The drift the comment failed to prevent

The copies had already diverged: core's formatContextText filters undefined context values; the proxy copy printed key=undefined. With the shared core, proxy text logs now filter undefined entries too (JSON logs unaffected). This is the one intentional behavior delta.

utils/logger/core.ts is a dependency-free leaf (imports nothing), so no cycle risk; module-boundary lint passes.

Verification

  • Proxy suite: 35 test files / 284 steps, 0 failed
  • deno task verify:quick exit 0 · deno task test:unit 2634 passed / 0 failed · pre-push (fmt + full suite) passed

…ed copy

proxy/logger.ts re-inlined ~130 lines of formatting primitives from
utils/logger/core.ts behind a "keep in sync" comment whose stated reason
(a Docker build that only copies src/proxy/) no longer exists: there is no
proxy Dockerfile in this repo or veryfront-infrastructure, the proxy ships
in the single deno compile binary, and proxy/*.ts already imports many
#veryfront/* modules. The copies had also already drifted: the proxy
version printed key=undefined context entries that core.ts filters out.

Import the shared core (a dependency-free leaf) and delete the copy. The
undefined-filtering behavior of core.ts now applies to proxy text logs.
Copilot AI review requested due to automatic review settings July 23, 2026 19:38
@kojiwakayama
kojiwakayama requested a review from kwakayama as a code owner July 23, 2026 19:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Refactors the proxy logger to reuse the shared logger formatting utilities from #veryfront/utils/logger/core.ts, removing the previously inlined (and drifting) copy of formatting code inside the proxy module. This aligns proxy text-log formatting behavior with the shared core, including filtering undefined context values.

Changes:

  • Replace ~130 lines of inlined formatting primitives in src/proxy/logger.ts with imports from #veryfront/utils/logger/core.ts.
  • Reuse the shared LogLevelName type for the proxy logger’s LogLevel.
  • Proxy text logs now filter out undefined context entries (JSON logs already avoided emitting empty context).

Verification (as reported in PR description):

  • deno task verify:quick (exit 0)
  • deno task test:unit (2634 passed / 0 failed)
  • Proxy test suite (35 test files / 284 steps, 0 failed)
  • Pre-push (fmt + full suite) passed

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@kwakayama
kwakayama enabled auto-merge (squash) July 24, 2026 05:08
@kwakayama
kwakayama merged commit a90979b into main Jul 24, 2026
29 checks passed
@kwakayama
kwakayama deleted the refactor/proxy-logger-shared-core branch July 24, 2026 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants