fix: address Copilot review on release/3.3.2 by igorls · Pull Request #1045 · MemPalace/mempalace

igorls · 2026-04-19T21:20:22Z

Addresses the Copilot review on #1041. Four of five Copilot concerns fixed; the fifth (the waitlist endpoint pointing at br.staging.mempalaceofficial.com) was confirmed as the intended production target and left as-is.

Fixes

Windows UnicodeEncodeError regressions (same class as #681)

mempalace/cli.py — ✗ → ERROR:, ⚠ → WARNING:, em dash → - in the mempalace sweep CLI output paths.
mempalace/sweeper.py — ⚠ → WARNING: in the per-file failure stderr message.

These paths were both running non-ASCII glyphs that would raise UnicodeEncodeError on Windows terminals with non-UTF-8 encodings — undoing part of the #681 fix that the release notes explicitly call out.

Backend argument validation

mempalace/backends/chroma.py — _normalize_get_collection_args now raises TypeError when unexpected positional args remain after parsing, instead of silently dropping them. Surfaces call-site bugs early.

Docs site — Google Analytics

website/.vitepress/config.mts — GA scripts are now gated behind MEMPALACE_DOCS_GA_ID. When unset, no gtag scripts are injected. Self-hosters / local previews no longer get GA unconditionally. CI can set the env var to the same G-PPQE4Z7P1K ID the hardcoded version used.

Landing page SPA leak

website/.vitepress/theme/landing/useLandingEffects.js — all IntersectionObserver instances and event listeners (waitlist form submit / input, replay button click) now push disconnect / removeEventListener thunks into a shared cleanups registry. onBeforeUnmount drains the registry so observers and listeners don't leak across SPA navigations (or fire against detached nodes).

Not fixed

Waitlist endpoint at br.staging.mempalaceofficial.com — confirmed as the intended production target for the waitlist service (not actually a staging environment despite the hostname). No change needed.

Test plan

uv run python -m pytest tests/ --ignore=tests/benchmarks -q → 1033 passed, 102s
uv run ruff check mempalace/ clean
Pre-commit hooks (ruff + ruff-format) pass on the commit

Non-ASCII glyphs (regression of the #681 class of Windows UnicodeEncodeError): - mempalace/cli.py: "✗" → "ERROR:", "⚠" → "WARNING:", em dash → "-" - mempalace/sweeper.py: "⚠" → "WARNING:" Backend arg validation: - mempalace/backends/chroma.py: `_normalize_get_collection_args` now raises TypeError on unexpected trailing positional args instead of silently dropping them — surfaces call-site bugs early. Docs site: - website/.vitepress/config.mts: gate Google Analytics scripts behind MEMPALACE_DOCS_GA_ID env var (default off). Self-hosters no longer get GA injected unconditionally. Landing page SPA hygiene: - website/.vitepress/theme/landing/useLandingEffects.js: collect all IntersectionObserver disconnects and removeEventListener thunks in a shared `cleanups` registry; drain it in `onBeforeUnmount` so observers and form/replay listeners don't leak across SPA navigations.

Copilot

Pull request overview

This PR follows up on Copilot feedback for the v3.3.2 release branch by hardening CLI output for Windows, tightening backend argument validation, and improving docs/landing-page behavior in the VitePress site.

Changes:

Replace non-ASCII glyphs in mempalace sweep outputs to avoid Windows UnicodeEncodeError.
Make Chroma backend argument normalization reject unexpected extra positional args.
Gate Google Analytics injection behind an environment variable and add SPA cleanup teardown for landing-page observers/listeners.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`website/.vitepress/theme/landing/useLandingEffects.js`	Adds centralized cleanup registry to remove listeners/observers on SPA unmount.
`website/.vitepress/config.mts`	Makes GA injection conditional on `MEMPALACE_DOCS_GA_ID`.
`mempalace/sweeper.py`	Replaces Unicode warning glyph in per-file failure output.
`mempalace/cli.py`	Replaces Unicode warning/error glyphs and em dash with ASCII in sweep output.
`mempalace/backends/chroma.py`	Raises `TypeError` on unexpected leftover positional args during normalization.

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

Copilot · 2026-04-19T21:23:02Z

+// Shared cleanup registry — IIFEs push disconnect/removeEventListener thunks
+// here so onBeforeUnmount can tear everything down on SPA nav.
+const cleanups = []
+
 onMounted(() => {
  if (typeof document === 'undefined') return


The cleanups registry comment and declaration are not indented inside useLandingEffects(), while the rest of the function body uses two-space indentation. This makes the block look like it's outside the function and can trip formatting/lint expectations; indent these lines to match the surrounding style.

Suggested change

// Shared cleanup registry — IIFEs push disconnect/removeEventListener thunks

// here so onBeforeUnmount can tear everything down on SPA nav.

const cleanups = []

onMounted(() => {

if (typeof document === 'undefined') return

// Shared cleanup registry — IIFEs push disconnect/removeEventListener thunks

// here so onBeforeUnmount can tear everything down on SPA nav.

const cleanups = []

onMounted(() => {

if (typeof document === 'undefined') return

Copilot · 2026-04-19T21:23:02Z

+      ...(gaId ? [
+        ['script', { async: '', src: `https://www.googletagmanager.com/gtag/js?id=${gaId}` }],
+        ['script', {}, `window.dataLayer = window.dataLayer || [];\nfunction gtag(){dataLayer.push(arguments);}\ngtag('js', new Date());\ngtag('config', '${gaId}');`],
+      ] as const : []),


gaId is interpolated directly into both the gtag script URL and an inline JS string literal. If the env var contains characters like quotes or whitespace, it can break the generated script (and, in the inline case, allow unintended JS injection in the built HTML). Consider validating the GA ID format and/or using encodeURIComponent for the URL and JSON.stringify(gaId) (or equivalent escaping) when embedding it into the inline script.

Copilot · 2026-04-19T21:23:02Z

+        if rest:
+            raise TypeError(f"unexpected positional args: {rest!r}")


This new branch raises when extra positional args are passed to get_collection(...), but there doesn’t appear to be a test covering this error path. Adding a small unit test (e.g., ChromaBackend().get_collection(path, name, False, 'extra') raising TypeError) would help prevent future regressions back to silently ignoring args.

Copilot AI review requested due to automatic review settings April 19, 2026 21:20

igorls requested review from bensig and milla-jovovich as code owners April 19, 2026 21:20

Copilot started reviewing on behalf of igorls April 19, 2026 21:20 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

igorls merged commit 04e11ae into release/3.3.2 Apr 20, 2026
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: address Copilot review on release/3.3.2#1045

fix: address Copilot review on release/3.3.2#1045
igorls merged 1 commit intorelease/3.3.2from
fix/copilot-review-release-3.3.2

igorls commented Apr 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if rest:
		raise TypeError(f"unexpected positional args: {rest!r}")

Conversation

igorls commented Apr 19, 2026

Fixes

Windows UnicodeEncodeError regressions (same class as #681)

Backend argument validation

Docs site — Google Analytics

Landing page SPA leak

Not fixed

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants