Skip to content

fix: apply contributed token colours via configurationDefaults#16

Closed
paxcalpt wants to merge 1 commit into
feat/colour-crossref-labelsfrom
fix/apply-token-colours
Closed

fix: apply contributed token colours via configurationDefaults#16
paxcalpt wants to merge 1 commit into
feat/colour-crossref-labelsfrom
fix/apply-token-colours

Conversation

@paxcalpt
Copy link
Copy Markdown
Contributor

What

Relocates all 23 contributes.tokenColors rules into editor.tokenColorCustomizations under contributes.configurationDefaults, and removes the now-empty tokenColors block.

Why

contributes.tokenColors is not a supported VS Code contribution point — VS Code never reads it and the compiled extension does not apply it, so the intended colours for <newpage>, blindtext placeholders, and embedded Python/TeX blocks have never actually rendered. This moves them to the supported mechanism (the same one PR #15 uses for cross-reference labels), so they apply as authored.

Notes

🤖 Generated with Claude Code

contributes.tokenColors is not a supported VS Code contribution point and had
no effect, so the extension's intended colours for <newpage>, blindtext
placeholders, and embedded Python/TeX blocks were never applied.

Move all 23 rules into editor.tokenColorCustomizations under
configurationDefaults (the working mechanism, same one used for the cross-ref
label colour) and remove the inert tokenColors block.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 29, 2026 17:53
@claude
Copy link
Copy Markdown

claude Bot commented May 29, 2026

Code Review: fix/apply-token-colours

Overview: Relocates 23 token colour rules from contributes.tokenColors (unsupported VS Code contribution point — never applied) into editor.tokenColorCustomizations under contributes.configurationDefaults (the same mechanism introduced in PR #15). Removes the now-empty tokenColors block. Scopes and settings are unchanged — this is a pure relocation.


What's Good


Issues to Consider

  1. Trailing whitespace. In the diff, there is a line containing only spaces immediately before the closing ] of textMateRules:

    +          }
    +    
              ]
    

    This stray whitespace should be removed. Most editors and CI linters flag it, and it may cause JSON formatting inconsistencies.

  2. Light theme contrast (pre-existing). Several relocated colours (e.g. #4EC9B0, #FF7F50, #C586C0) are from VS Code's Dark+ palette and may be low-contrast on light themes. This is a pre-existing issue, not introduced here, but activating these rules for the first time makes it newly visible. A visual check on a light theme is recommended.

  3. Behaviour change for existing users. The PR description notes this correctly: colours that were silently inactive will now render. Users who customised their theme assuming these colours were absent may see unexpected highlighting. Worth mentioning in the release notes or version bump so users know what changed.

  4. Merge order dependency. This PR targets feat/colour-crossref-labels (PR feat: cross-reference colour scheme + apply contributed token colours (v0.3.16) #15) as its base. If PR feat: cross-reference colour scheme + apply contributed token colours (v0.3.16) #15 merges before this one is retargeted to main, the target must be updated. Consider retargeting to main proactively once feat: cross-reference colour scheme + apply contributed token colours (v0.3.16) #15 is approved to avoid a broken merge.


Minor


Verdict: Approve pending the trailing whitespace fix and a light-theme visual check. The logic is sound and the migration strategy is correct.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR moves inactive token color contributions into VS Code’s supported configurationDefaults.editor.tokenColorCustomizations path so rxiv-markdown syntax colours can be applied through the extension manifest.

Changes:

  • Relocates token color rules from unsupported contributes.tokenColors into configurationDefaults.
  • Removes the obsolete tokenColors contribution block.
  • Adds an Unreleased changelog entry describing the token colour fix.

Reviewed changes

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

File Description
package.json Moves token color rules into editor.tokenColorCustomizations.textMateRules and removes tokenColors.
CHANGELOG.md Documents the token colour relocation under Unreleased.

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

Comment thread package.json
}
},
{
"scope": "keyword.control.placeholder.blindtext.rxiv",
Comment thread package.json
}
},
{
"scope": "keyword.control.import.python.rxiv",
@claude
Copy link
Copy Markdown

claude Bot commented May 29, 2026

Code Review: fix/apply-token-colours

Overview

This PR relocates 23 contributes.tokenColors rules (which VS Code silently ignores) into editor.tokenColorCustomizations under configurationDefaults, activating colours for <newpage>, blindtext, and embedded Python/TeX blocks that were previously never applied.

The fix is correct and necessary. However, there is one real bug that should be addressed before merging.


Critical Issue

[Bug] markup.subscript and markup.superscript are generic scopes — they will colour those elements in ALL file types globally

{
  "scope": ["markup.subscript", "markup.superscript"],
  "settings": { "foreground": "#A0A0A0" }
}

Unlike every other rule in this block (all of which use .rxiv-suffixed scopes), these two scopes are standard TextMate scopes shared by Markdown, AsciiDoc, reStructuredText, and any other grammar that assigns markup.subscript/markup.superscript. Because editor.tokenColorCustomizations in configurationDefaults is a global override (see PR #15 review), installing this extension will turn subscript/superscript text gray in every file the user opens — including plain Markdown .md files unrelated to rxiv-maker.

Recommended fix: rename these scopes in the grammar to markup.subscript.rxiv and markup.superscript.rxiv (or markup.subscript.rxiv-markdown), then match the renamed scopes in the colour rule. The grammar file already uses the .rxiv naming convention for all other custom scopes. Alternatively, if renaming the scopes is undesirable, these two rules could be excluded from the colour customizations and left to the theme.


Other Issues

[Minor] Trailing whitespace in package.json (line after last added rule)

The diff shows a line + (four spaces, no content) immediately before the closing ]. This is likely an editing artefact. Worth cleaning up to keep the JSON tidy, even though it doesn't affect parsing.

[Minor] Hardcoded Dark+ palette — light theme users will see reduced contrast

All colours (#569CD6, #C586C0, #4EC9B0, #FF7F50, etc.) are from VS Code's Dark+ theme. On a light theme (e.g., Light+, GitHub Light), several of these produce very low contrast against a white background. The PR description acknowledges "worth a visual check against the active theme" — consider either (a) testing with a light theme before merging, or (b) adding a note to the release entry that colours are optimised for dark themes.

[Minor] Version not bumped

package.json still reads 0.3.15. If PR #15 and this PR are merged together as a batch, both sets of changes should be represented in a single version bump. No action needed now, but should happen before publishing.

[Informational] Stacking dependency

PR explicitly states it is stacked on #15 (base = feat/colour-crossref-labels). After #15 merges to main, this PR must be retargeted to main and the diff re-verified to ensure no conflicts.


What Works Well

  • The core diagnosis is correct: contributes.tokenColors is not a VS Code contribution point and has never had any effect. Moving rules to configurationDefaults is the right fix.
  • Pure relocation — scope/settings values are unchanged, easy to verify.
  • CHANGELOG entry is clear and accurate.

Security / Performance

No concerns — static JSON configuration.


Verdict

Request changes. The markup.subscript/markup.superscript global scope bleed is a real user-visible regression (graying out sub/superscript in unrelated files). Fix the scope names in the grammar or exclude those two rules from the global override, then this is good to merge (after #15 lands first).

@paxcalpt
Copy link
Copy Markdown
Contributor Author

Superseded by #15, which now folds in this tokenColors→configurationDefaults migration alongside the full cross-reference colour scheme and the 0.3.16 release.

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