Skip to content

fix(logging): route console output through the shared logger facade - #1235

Merged
Chloe-JY merged 2 commits into
developfrom
worktree-fix-logger-facade-consistency
Sep 3, 2026
Merged

fix(logging): route console output through the shared logger facade#1235
Chloe-JY merged 2 commits into
developfrom
worktree-fix-logger-facade-consistency

Conversation

@Harry19081

@Harry19081 Harry19081 commented Sep 3, 2026

Copy link
Copy Markdown
Member

Problem

An audit of every frontend log call site (prompted by noisy Org2CloudSyncEngine console output in a separate fix) found the shared logging facade at src/hooks/logger/useLogger.ts — the documented "single frontend logging facade" — wasn't consistently reused:

  • 17 call sites across 8 files called raw console.warn/console.error with a hand-rolled `[Module] ` prefix instead of createLogger(...).warn/error. Because useLogger.ts installs a global console.* interceptor, these still got level-gated, but every one of them was persisted to ~/.orgii/logs/frontend.log under the generic "console" namespace instead of the real module name — losing per-module filterability in the one channel meant for post-incident diagnosis.
  • 21 call sites across 13 files did call createLogger correctly, but then retyped that logger's own namespace as a `[Module] ` prefix inside the message text — which the facade already prepends automatically (emitToConsole builds `[${namespace}]` and passes it as a separate arg). Every one of these printed doubled, e.g. [BackgroundImage] [BackgroundImage] Failed to load ..., in both devtools and the persisted log file.

Solution

  • Wired the 5 files whose raw-console call sites aren't pinned by a test that spies on console directly — AnyIcon.tsx, localChannelsAtom.ts, localChannelMessagesAtom.ts, multiRunnerAtom.ts, runGroupsAtom.ts — to createLogger, preserving behavior exactly (in particular, AnyIcon's three warnings keep their existing NODE_ENV !== "production" guard, so they stay silent in production exactly as before).
  • Stripped the 21 redundant duplicate-prefix strings across the 13 files that already used createLogger correctly, now that the logger's own namespace supplies the prefix.
  • Left 3 files' 5 call sites untouched: BoundedMap.ts, launchPayload.ts (×2), useCallbackRefEffect.ts each carry a // Raw console.X kept intentionally: asserted by <file>.test.ts comment — these are deliberate, pre-existing exceptions (low-level/dependency-free utilities, or code whose own test spies on console directly) and changing them would either break that spy-based coverage or introduce a needless dependency into a primitive that's meant to have none.
  • zodStorage.ts's two call sites are the same category (its own zodStorage.test.ts spies on console.warn directly) but weren't documented with that comment — added the matching "kept intentionally" comment for consistency with the other three, without touching behavior.
  • Updated the Team Inbox test harness to preserve the real Git-remotes module exports while overriding only getGitRemotes; this prevents the logging diff’s import graph from exposing a stale full-module mock during full-suite collection.
  • Left one unrelated finding out of scope for this PR: runGroupsAtom.ts's StoredRunGroupsSchema reimplements the exact record-drop-invalid-entries pattern that zodStorage.ts already exports as tolerantRecordSchema — a schema-level duplication, not a logging one. Flagging it rather than folding it in here to keep this PR single-purpose.

Potential risks

  • Pure logging-output change: no control flow, persisted user data, or returned values are touched in any of the 20 files.
  • AnyIcon's three warnings verifiably keep their production no-op behavior (the NODE_ENV guard was left in place around the new log.warn calls).
  • The 9 newly-wired storage-atom warnings (localChannels, localChannelMessages, multiRunner, runGroups) now persist to frontend.log under their real module name where they previously fell under the generic "console" bucket — a diagnosability improvement, not a behavior change a user would notice; no existing test asserted on the old generic bucketing.
  • The CI repair changes only a Vitest mock boundary: production code and runtime behavior are unaffected, while future additions to the Git-remotes module remain visible to transitive imports.

Verification

  • npx tsgo --noEmit --pretty false → clean
  • npx oxlint -c .oxlintrc.json --max-warnings 0 <20 changed files> → clean
  • npx eslint --max-warnings 0 --report-unused-disable-directives <20 changed files> → clean
  • npx prettier --write <20 changed files> → already formatted, no changes
  • npx vitest run --config config/vitest.config.ts on every test file covering a touched module (zodStorage, zodStorage.roundTrip, localChannelsAtom, localChannelMessagesAtom, BoundedMap, launchPayload, CallbackRefEffectLifecycle, zodRegistry, localStorage, sessionAtom/loaders, sessionAtom/sidebarLoaders, workstation tabs storage, useCodeEditor/helpers) → 197/197 passed, 0 failed
  • pnpm run check:test-placement → consistent across 457 directories
  • pnpm exec vitest run --config config/vitest.config.ts src/modules/MainApp/TeamInbox/__tests__/AssignedWorkItemDetail.test.ts --reporter=verbose9/9 passed, 0 failed
  • node --test scripts/ci/*.test.cjs28/28 passed, 0 failed
  • pnpm run test1388/1388 test files and 10646/10646 tests passed, 0 failed
  • Pre-commit hook ran clean after the CI repair: lint-staged (oxlint + eslint --fix + prettier) and staged-file tsgo typecheck passed.
  • Not run: i18n check (no locale files touched), cargo/clippy (no Rust files touched).

Problem: an audit of every log call site found two ways the logging
facade (src/hooks/logger/useLogger.ts) wasn't being reused as intended.

- 17 call sites across 8 files called raw console.warn/error with a
  hand-rolled "[Module] " prefix instead of createLogger(...).warn/error,
  losing the facade's namespace-scoped persistence to
  ~/.orgii/logs/frontend.log (the interceptor logs these under the
  generic "console" namespace instead of the real module).
- 21 call sites across 13 files DID use createLogger correctly, but then
  retyped that logger's own namespace as a "[Module] " prefix inside the
  message text, which the facade already prepends automatically -
  doubling the prefix in devtools and in the persisted log file (e.g.
  "[BackgroundImage] [BackgroundImage] Failed to load ...").

Solution:
- Wired AnyIcon.tsx, localChannelsAtom.ts, localChannelMessagesAtom.ts,
  multiRunnerAtom.ts, and runGroupsAtom.ts to createLogger, preserving
  existing behavior exactly (AnyIcon's prod-only guard kept as-is).
- Stripped the 21 redundant duplicate-prefix strings now that the
  logger's own namespace already supplies them.
- Left 5 other raw-console call sites untouched: BoundedMap.ts,
  launchPayload.ts (x2), useCallbackRefEffect.ts, invoke.ts, and
  zodStorage.ts (x2) are deliberate, pre-existing exceptions whose own
  tests spy on console directly; added a matching "kept intentionally"
  comment to zodStorage.ts's two call sites for consistency with the
  other four, which already carried one.

Potential risks: pure logging-output changes, no control-flow or
persisted-data changes. AnyIcon's three warnings keep their
NODE_ENV-guarded no-op-in-production behavior verbatim. The 9 newly
wired storage-atom warnings persist to frontend.log under their real
module name where they previously fell under the generic "console"
bucket - a strict improvement in diagnosability, not a behavior change
a user would notice.

Verification:
- npx tsgo --noEmit --pretty false -> clean
- npx oxlint -c .oxlintrc.json --max-warnings 0 <changed files> -> clean
- npx eslint --max-warnings 0 --report-unused-disable-directives <changed files> -> clean
- npx prettier --write <changed files> -> already formatted
- npx vitest run --config config/vitest.config.ts on every test file
  covering a touched module (zodStorage, zodStorage.roundTrip,
  localChannelsAtom, localChannelMessagesAtom, BoundedMap, launchPayload,
  CallbackRefEffectLifecycle, zodRegistry, localStorage, loaders,
  sidebarLoaders, workstation tabs storage, useCodeEditor helpers) ->
  109 + 88 = 197 tests passed, 0 failed
- npm run check:test-placement -> consistent across 457 directories
- Full repo `npm run test` (1329 files / ~10441 tests) not run locally;
  left for CI per this repo's targeted-local-verification convention.
@Chloe-JY
Chloe-JY merged commit bba6ebf into develop Sep 3, 2026
7 checks passed
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.

2 participants