fix(macros): resolve member access across all user scripts#1155
fix(macros): resolve member access across all user scripts#1155
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe changes refactor SingleMacroEngine's member-access export resolution from eager caching during candidate selection to lazy loading at execution time, removing the preloaded scripts map and simplifying the candidate data structure accordingly. Test coverage validates the new behavior with assertions and a new selector-targeted script test case. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying quickadd with
|
| Latest commit: |
825890f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2c6e414c.quickadd.pages.dev |
| Branch Preview URL: | https://codex-964-expose-macro-scrip.quickadd.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
member-access-owner-plan.md (1)
1-36: Consider removing this planning document before merging.This file appears to be an internal work-tracking document containing:
- Developer-specific local paths (
/Users/christian/Developer/quickadd/)- Implementation notes and debugging artifacts
- Task checkboxes that won't be meaningful post-merge
Planning documents like this are valuable during development but typically shouldn't be committed to the repository. Consider moving this content to the PR description or deleting the file before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@member-access-owner-plan.md` around lines 1 - 36, The file member-access-owner-plan.md is a local development plan with personal paths and ephemeral task/checklist data; remove it from the commit before merging by either deleting member-access-owner-plan.md or moving its content into the PR description (or a private internal note), and if you must keep a planning file sanitize/remove developer-specific paths like "/Users/christian/Developer/quickadd/" and ephemeral checkboxes so the committed file contains only stable, shareable documentation.tests/e2e/macro-member-access.test.ts (1)
16-19: Consider increasing timeout for CI environments.The
WAIT_OPTStimeout of 10 seconds may be tight for slower CI runners. If flakiness is observed, consider increasing to 15-20 seconds or making it configurable via environment variable.💡 Optional: Make timeout configurable
-const WAIT_OPTS = { timeoutMs: 10_000, intervalMs: 200 }; +const WAIT_OPTS = { + timeoutMs: Number(process.env.E2E_TIMEOUT_MS) || 15_000, + intervalMs: 200, +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/macro-member-access.test.ts` around lines 16 - 19, The WAIT_OPTS constant uses a 10_000ms timeout which is tight for CI; update WAIT_OPTS to use a larger default (e.g., 20_000ms) or make it configurable by reading an environment variable (e.g., process.env.WAIT_TIMEOUT_MS) with a numeric fallback, ensuring intervalMs remains unchanged; adjust any test helpers that import WAIT_OPTS if needed so CI can override the timeout via the env var.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@member-access-owner-plan.md`:
- Around line 1-36: The file member-access-owner-plan.md is a local development
plan with personal paths and ephemeral task/checklist data; remove it from the
commit before merging by either deleting member-access-owner-plan.md or moving
its content into the PR description (or a private internal note), and if you
must keep a planning file sanitize/remove developer-specific paths like
"/Users/christian/Developer/quickadd/" and ephemeral checkboxes so the committed
file contains only stable, shareable documentation.
In `@tests/e2e/macro-member-access.test.ts`:
- Around line 16-19: The WAIT_OPTS constant uses a 10_000ms timeout which is
tight for CI; update WAIT_OPTS to use a larger default (e.g., 20_000ms) or make
it configurable by reading an environment variable (e.g.,
process.env.WAIT_TIMEOUT_MS) with a numeric fallback, ensuring intervalMs
remains unchanged; adjust any test helpers that import WAIT_OPTS if needed so CI
can override the timeout via the env var.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6d065fe-13ae-4ec6-a1cf-01540f3e829f
📒 Files selected for processing (8)
docs/docs/Choices/MacroChoice.mdmember-access-owner-plan.mdsrc/engine/SingleMacroEngine.member-access.test.tssrc/engine/SingleMacroEngine.tssrc/gui/MacroGUIs/CommandList.sveltesrc/gui/MacroGUIs/Components/UserScriptCommand.sveltesrc/gui/MacroGUIs/UserScriptSettingsModal.tstests/e2e/macro-member-access.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05af16e811
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
chhoumann
force-pushed
the
codex/964-expose-macro-script-members
branch
from
9981670 to
825890f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/engine/SingleMacroEngine.ts (1)
195-218: Consider caching selected script to avoid double-loading.In multi-script macros,
selectUserScriptCandidatealready loads scripts viagetUserScript()at line 347 to determine which exports the member. The selected script is then loaded again here. SincegetUserScriptperforms file I/O andwindow.eval()on each call, this results in redundant work.You could cache the exports from selection and pass them via the existing
preloadedUserScriptsmechanism inMacroChoiceEngine, or store them directly on the candidate for reuse.♻️ Sketch: pass selected script's exports to avoid re-load
type MemberAccessSelection = { candidate: UserScriptCandidate; memberAccess: string[]; + cachedExports?: unknown; }; // In selectUserScriptCandidate, when a unique match is found: return { candidate: matchingCandidates[0], memberAccess, + cachedExports: matchingCandidates[0].resolvedMember.value + ? /* store the full exportsRef from selection */ : undefined, }; // In tryExecuteExport: -const exportsRef = await getUserScript(userScriptCommand, this.app); +const exportsRef = selection.cachedExports + ?? await getUserScript(userScriptCommand, this.app);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engine/SingleMacroEngine.ts` around lines 195 - 218, The code double-loads a user script: selectUserScriptCandidate calls getUserScript() to inspect exports, then SingleMacroEngine calls getUserScript() again before resolveMemberAccess, causing redundant I/O and eval; modify the flow to reuse the already-loaded exports by reading from the existing preloadedUserScripts cache (or attach the exports to the selected candidate) instead of calling getUserScript() a second time—i.e., when selectUserScriptCandidate determines the chosen userScriptCommand, store its exports in MacroChoiceEngine.preloadedUserScripts (keyed by the same identifier used elsewhere) or on the candidate object, and update SingleMacroEngine to check that cache/field for exportsRef before calling getUserScript(), falling back to getUserScript() only if no cached exports exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/engine/SingleMacroEngine.ts`:
- Around line 195-218: The code double-loads a user script:
selectUserScriptCandidate calls getUserScript() to inspect exports, then
SingleMacroEngine calls getUserScript() again before resolveMemberAccess,
causing redundant I/O and eval; modify the flow to reuse the already-loaded
exports by reading from the existing preloadedUserScripts cache (or attach the
exports to the selected candidate) instead of calling getUserScript() a second
time—i.e., when selectUserScriptCandidate determines the chosen
userScriptCommand, store its exports in MacroChoiceEngine.preloadedUserScripts
(keyed by the same identifier used elsewhere) or on the candidate object, and
update SingleMacroEngine to check that cache/field for exportsRef before calling
getUserScript(), falling back to getUserScript() only if no cached exports
exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79d9b049-ed66-4814-8ad9-b00d2bb64614
📒 Files selected for processing (3)
src/engine/SingleMacroEngine.member-access.test.tssrc/engine/SingleMacroEngine.tstests/e2e/macro-member-access.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/macro-member-access.test.ts
- src/engine/SingleMacroEngine.member-access.test.ts
|
Closing this PR because the main feature already landed on \ as commit \ while this PR remained open after the earlier merge attempt returned a transient GitHub error. I’m opening a fresh follow-up PR with only the remaining review-fix delta from . |
…#1155) * fix(macros): resolve member access across macro scripts * codex: remove plan file from PR (chhoumann#1155)
1 participant
Macro::membercurrently short-circuits to the first user script in a macro. This changes member lookup to search all user scripts in the macro and execute the unique match instead.When multiple scripts export the same member, QuickAdd now aborts with the conflicting script names and tells the user how to disambiguate with
Macro::Script Name::member. When no script exports the member, it aborts clearly instead of guessing or silently falling through.I considered the explicit owner model from the issue write-up, but it adds configuration to the common case. The merged namespace keeps single- and multi-script macros zero-config unless there is a real naming conflict, while still giving users an explicit escape hatch when they need it.
This also removes the unshipped owner toggle/badge path, updates the unreleased docs, adds focused unit coverage for unique/conflict/selector cases, and adds a vault-backed e2e covering later-script resolution plus qualified disambiguation.
Fixes #964
Validation:
bun run testbun run build-with-lintbun run test:e2e tests/e2e/macro-member-access.test.tsSummary by CodeRabbit
Refactor
Tests