refactor: remove the InfiniteAIAssistant command type - #1633
Open
chhoumann wants to merge 3 commits into
Open
Conversation
It shipped in 1.2.0 and was never creatable in any released version. Across all 202 tags the identifier only ever touched CommandType, the interface, the modal (+test), main.ts, packagePreview, MacroDisclosure, modelRefPinning, refreshStaleDefaultModelSeeds and one test - never a builder, an engine or an API file. The only add-AI path hardcodes CommandType.AIAssistant. Chunked prompting shipped instead as quickAddApi.ai.chunkedPrompt(), which is a strict superset: documented, tested, auto-splitting on provider rejection. Wiring it up was the alternative, and it is not a missing `if`: IInfiniteAIAssistantCommand has no promptTemplate and no text source, and ChunkedPrompt throws without a chunk reference. It would mean designing where the text comes from plus a template picker - a new feature with zero recorded demand, competing with a superset. If chunking ever returns to the builder, the right shape is a "chunk long input" toggle on the existing AI Assistant command reusing its promptTemplate, not a second command type. Removed: the enum member, IInfiniteAIAssistantCommand, the 268-line modal and its test, the systemPromptFields entry, the devMode testQuickAdd command (whose entire body constructed that modal) and its label, the packagePreview case, the MacroDisclosure label, the isAICommand clauses in modelRefPinning and refreshStaleDefaultModelSeeds, and the orphaned aiHelpers pair getMaxChunkTokensUpperBound / getLargestModelMaxTokens with their test, FALLBACK_MODEL_MAX_TOKENS and the estimateModelInputBudget import they were the only local users of. No data migration. Converting to a plain AIAssistant is not inert - with promptTemplate.enable false it opens a template picker and fires a billable request, or throws under disableOnlineFeatures (the default). Dropping the command deletes user data to fix a hazard nobody demonstrably has. And chunkSeparator is a string here but a RegExp in chunkedPrompt, so a stored "\n" is a literal backslash-n that no conversion could carry faithfully. The dispatch loop covers the hazard permanently and generically instead. The engine keeps telling the truth about the type after the enum member is gone: RETIRED_COMMAND_TYPES holds the message, so a data.json still carrying one gets "never runnable, now removed, use chunkedPrompt" rather than the generic "can come from a newer version of QuickAdd", which would be the exact opposite of what happened. Docs: the anchor-pinned "Max chunk tokens" section sat under AIAssistant's "Model settings and token budgets" next to real settings, but the only control that ever rendered it was the slider inside the unreachable modal. It moves to the chunkedPrompt API docs, reframed as the API option it actually is. The frozen anchor is preserved on both pages - the alias-anchor pattern this docs tree already uses - so /docs/AIAssistant#max-chunk-tokens still lands on a pointer to the new home. AGENTS.md pointed agents at the deleted quickadd:testQuickAdd command; it now points at quickadd:run and eval.
…r visible
Review follow-ups on the removal:
- The retired-type notice ran 438 characters, ~50% longer than any notice
QuickAdd has ever shipped, in a 15-second toast. The provenance clause ("no
macro-builder entry and no engine branch in any version") was commit-message
material with no user-side referent; the two actions now come first.
- The docs pointer was a bare `<a id>` under the "Max tokens is the context
window" section, so a reader arriving on the frozen anchor landed mid-way
through a section about a different field, with the phrase they clicked
nowhere on the page. It is a real heading again - same anchor, same ToC
entry, one line of body pointing at the API docs.
- AGENTS.md claimed `eval` takes "an expression, not statements". Statements
are fine and the completion value comes back; only top-level `await` fails.
📝 WalkthroughWalkthroughThe PR removes the unreachable ChangesInfinite AI Assistant retirement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Macro
participant MacroChoiceEngine
participant RetiredTypeLookup
participant PackagePreview
Macro->>MacroChoiceEngine: execute legacy InfiniteAIAssistant
MacroChoiceEngine->>RetiredTypeLookup: describeUnknownType
RetiredTypeLookup-->>MacroChoiceEngine: retired command message
PackagePreview->>PackagePreview: classify legacy type as unknown-command
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: |
186f97d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ddc71bb2.quickadd.pages.dev |
| Branch Preview URL: | https://chhoumann-1571-remove-infini.quickadd.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ai/modelRefPinning.ts`:
- Line 13: The model-mutation paths need regression coverage for legacy
"InfiniteAIAssistant" commands. Add tests for pinAiCommandModelRefs in
src/ai/modelRefPinning.ts and refreshStaleDefaultModelSeeds in
src/migrations/refreshStaleDefaultModelSeeds.ts confirming legacy records remain
unchanged, then run pnpm run test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97466738-b081-44eb-a93c-2d67008f381f
📒 Files selected for processing (22)
AGENTS.mddocs/src/content/docs/docs/AIAssistant.mddocs/src/content/docs/docs/QuickAddAPI.mdsrc/ai/AIAssistant.systemPromptLiteral.test.tssrc/ai/aiHelpers.maxChunkTokens.test.tssrc/ai/aiHelpers.tssrc/ai/modelRefPinning.tssrc/commandLabels.tssrc/engine/MacroChoiceEngine.tssrc/engine/MacroChoiceEngine.unknownCommand.test.tssrc/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.test.tssrc/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.tssrc/gui/PackageManager/MacroDisclosure.sveltesrc/gui/ai/systemPromptFields.test.tssrc/gui/ai/systemPromptLiteralNote.tssrc/main.commandLabels.test.tssrc/main.tssrc/migrations/refreshStaleDefaultModelSeeds.tssrc/services/packagePreview.test.tssrc/services/packagePreview.tssrc/types/macros/CommandType.tssrc/types/macros/QuickCommands/IAIAssistantCommand.ts
💤 Files with no reviewable changes (10)
- src/types/macros/CommandType.ts
- src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.test.ts
- src/commandLabels.ts
- src/ai/aiHelpers.maxChunkTokens.test.ts
- src/types/macros/QuickCommands/IAIAssistantCommand.ts
- src/gui/PackageManager/MacroDisclosure.svelte
- src/main.commandLabels.test.ts
- src/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.ts
- src/ai/aiHelpers.ts
- src/main.ts
… alone Removing the type deliberately ships no migration, so `pinAiCommandModelRefs` and `refreshStaleDefaultModelSeeds` have to leave an "InfiniteAIAssistant" step exactly as the user's data.json has it. Both lost their clause for it with no test failing; re-adding either clause now fails these two. It matters beyond the migration run: pinAiCommandModelRefs also runs on package import, so it meets this shape long after the one-time migration is flagged done.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/migrations/refreshStaleDefaultModelSeeds.test.ts (1)
278-301: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that the legacy discriminator is preserved.
This test claims the command is unchanged, but
storedAICommand()exposes onlymodelandmodelParameters. A migration that rewritestypewould still pass. Includetypein the helper result and assert it remains"InfiniteAIAssistant".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/migrations/refreshStaleDefaultModelSeeds.test.ts` around lines 278 - 301, Update storedAICommand() to expose the command’s type discriminator, then extend the “does not touch a legacy InfiniteAIAssistant command” test to assert it remains "InfiniteAIAssistant" after refreshStaleDefaultModelSeeds.migrate(mockPlugin).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/migrations/refreshStaleDefaultModelSeeds.test.ts`:
- Around line 278-301: Update storedAICommand() to expose the command’s type
discriminator, then extend the “does not touch a legacy InfiniteAIAssistant
command” test to assert it remains "InfiniteAIAssistant" after
refreshStaleDefaultModelSeeds.migrate(mockPlugin).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 071ae883-b753-48e4-8cd2-fb4b83875f7f
📒 Files selected for processing (2)
src/migrations/pinAiModelRefs.test.tssrc/migrations/refreshStaleDefaultModelSeeds.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1623, which is now merged. This is the second half of #1571: #1623 stopped the engine dropping steps it cannot run; this removes the type that motivated it.
(Replaces #1624, which GitHub auto-closed when #1623's branch was deleted - same change, rebased onto master.)
What this removes, and why
CommandType.InfiniteAIAssistantshipped inae90edebon 2023-06-20, six minutes before the1.2.0tag. Every release since has carried it. It was never creatable in any released version: across all 202 tags,git log --all -S"InfiniteAIAssistant" --name-onlyshows the identifier only ever touchedCommandType.ts,IAIAssistantCommand.ts, the modal (+test),main.ts,packagePreview.ts,MacroDisclosure.svelte,modelRefPinning.ts,refreshStaleDefaultModelSeeds.tsand one test file. Never a builder, an engine, or an API file. NoInfiniteAIAssistantCommandclass ever existed; the only add-AI path isCommandSequenceEditor.ts -> new AIAssistantCommand(), which hardcodesCommandType.AIAssistant.It was abandoned deliberately. Chunked prompting shipped as a user script (discussion #535: "I'm kind of on the fence about whether it should be a feature") and later as
quickAddApi.ai.chunkedPrompt().Rejecting "wire it up instead." It is not a missing
if.IInfiniteAIAssistantCommandhas nopromptTemplateand no text source, andChunkedPromptthrows without a chunk reference. Wiring it up means designing where the text comes from plus a template picker: a new feature with zero recorded demand, competing againstquickAddApi.ai.chunkedPrompt, which is a strict superset (documented, 588-line test file, auto-split on provider rejection, 500-prompt cap). If chunking ever returns to the builder, the right shape is a "chunk long input" toggle on the existing AI Assistant command reusing itspromptTemplate, not a second command type. Recording that so it does not get re-proposed.Removed
Enum member ·
IInfiniteAIAssistantCommand· the 268-line modal + its 114-line test · thesystemPromptFieldsMODALSrow · the devModetestQuickAddcommand (its entire body constructed that modal - and passedtype: CommandType.AIAssistantfor anIInfiniteAIAssistantCommand, so even the dev harness built a mistyped object) and its label · thepackagePreviewcase · theMacroDisclosurelabel · theisAICommandclauses inmodelRefPinning.tsandrefreshStaleDefaultModelSeeds.ts· the orphanedgetMaxChunkTokensUpperBound/getLargestModelMaxTokenswith their test, plusFALLBACK_MODEL_MAX_TOKENSand theestimateModelInputBudgetimport they were the only local users of.chunkSeparator/resultJoiner/maxChunkTokens/mergeChunkson the script API path (AIAssistant.ts,quickAddApi.ts) are untouched. Same names, different feature - the biggest trap in this removal.No data migration
Deliberate, and the reasoning is worth keeping:
AIAssistantis not inert. WithpromptTemplate.enable: false,executeAIAssistant->runAIAssistantopens a prompt-template picker and fires a billable network request. It turns a no-op into an interactive, billable step the user never configured. UnderdisableOnlineFeatures(the default) it throws instead.chunkSeparatoris astringhere and aRegExpinchunkedPrompt, so a stored"\n"is a literal backslash-n.Verified in the live vault: a
data.jsonholding one survives load, all migrations, a macro-builder open/reorder/close, andsaveSettings()byte-identical (sha22cc0612…before and after). Nothing in this PR writes to it.Behaviour changes worth naming
1.
pinAiCommandModelRefsstops classifying it as an AI command at import time (packageImportService.ts->modelRefPinning.ts). Same forrefreshStaleDefaultModelSeeds. Both migrations now skip such a command instead of pinning amodelRef/ rewritingmodel/ stripping baked params on it. This removes writes rather than adding them, and the command was never runnable, so nothing observable changes - but it is a real behaviour change, not "2 LOC".2. Package import disclosure flips from a false promise to an honest warning.
KNOWN_COMMAND_TYPESisObject.values(CommandType), so removal moves a legacyInfiniteAIAssistantstep from the "AI assistant / Sends note content to your AI provider over the network" row to "Unknown capability. Review it manually." Pinned by a new test. It also meansMacroDisclosurerenders aCapabilityTagfor the row, never the now-label-less raw type string.3. The engine keeps telling the truth about the type after the enum member is gone. A naive removal would drop it into the generic branch and tell the user it "can come from a newer version of QuickAdd" - the exact opposite of what happened, and no help.
RETIRED_COMMAND_TYPEScarries the fact forward:The
outprompt behind it is the unchanged downstream symptom: the step still does not setout, so the capture still asks. What changed in #1623 is that the user is now told why. This PR adds what to do instead.(Screenshot taken on this branch rebased onto merged master, in this worktree's isolated vault.)
4.
quickadd:runstill answersok: truefor a macro with a skipped step (the envelope already carriesverified: false). Enumerating skipped steps in the run envelope belongs with the #1606 result contract, not here.Docs
AIAssistant.mdhad an anchor-pinned### Max chunk tokens {#max-chunk-tokens}under Model settings and token budgets, sitting next to real settings - but the only "Max chunk tokens" control the plugin ever had was the slider inside the unreachable modal. A reader hunts for a setting that does not exist.The substance moves to the
chunkedPromptAPI docs, reframed as the API option it actually is, and the redundant duplicate bullet is folded into a pointer. Anchors are frozen, so{#max-chunk-tokens}is preserved as a real heading on both pages: the old page keeps its ToC entry and the term the reader clicked, with one line pointing at the new home.docs/scripts/check-links.py: 0 problems across 50 pages; bothid="max-chunk-tokens"anchors present in the built site.Also:
AGENTS.mdpointed agents at the deletedquickadd:testQuickAddcommand.Flagged, not folded in (sibling workers are active on #1607-#1615 in this area)
CommandList.svelte's{:else}fallback renders the retired command as an ordinary row with Delete + Reorder and no marker. Since there is deliberately no migration, the builder is where these live forever. Worth a follow-up - but the population is "users with a hand-editeddata.json", and the acquisition survey below found none.getModelMaxTokens(aiHelpers.ts) has no production caller either - dead onmaster, not introduced or adjacent here.Acquisition surface, for the record
Hand-edited
data.json, a script mutatingsettings.choices, or a hand-authored package. Zero hits in issues, zero in discussions, zero public JSON on GitHub, zero of the 6 public packages. That is "no evidence anyone has one", not "impossible" - which is exactly why the fix is #1623's generic guard rather than a migration.Verification
Re-run after rebasing onto merged master (
1a864faa, i.e. after #1623, #1629 and #1632 landed):tsc -noEmitclean ·eslintclean ·svelte-check1171 files / 0 errors · 4799 tests pass · docs build +check-links.py0 problems across 50 pages, both#max-chunk-tokensanchors present.Live in this worktree's isolated vault, on the rebased build:
data.jsoncommand survives load + all migrations, andsaveSettings()preserves every field (model,modelParameters,outputVariableName,systemPrompt,resultJoiner,chunkSeparator,maxChunkTokens,mergeChunks) - nothing here rewrites user data;quickadd:testQuickAddis gone from the command registry, withrunQuickAdd,runTemplateFromFolder,applyTemplateToActiveFile,reloadQuickAddandopenQuickAddSettingsintact.Closes #1571
Summary by CodeRabbit
Changes
Documentation
pnpm run obsidian:e2e, including choice execution/result interpretation and safe JS evaluation guidance.chunkedPrompt()’smaxChunkTokens, with details on token budgeting and prompt overhead.