Skip to content

refactor(site): centralize language storage key and clipboard helper#2779

Open
yonib05 wants to merge 14 commits into
strands-agents:mainfrom
yonib05:homepage-shared-utils
Open

refactor(site): centralize language storage key and clipboard helper#2779
yonib05 wants to merge 14 commits into
strands-agents:mainfrom
yonib05:homepage-shared-utils

Conversation

@yonib05

@yonib05 yonib05 commented Jun 14, 2026

Copy link
Copy Markdown
Member

Description

Follow-up cleanup requested in review of #2775 and #2778: the starlight-synced-tabs__jarkqt storage key was a magic string duplicated across LanguageToggle.astro, Syntax.astro, and HeroSection.astro, and navigator.clipboard.writeText was awaited without a catch in several copy buttons (including the pre-existing CodeBlock.astro), so the "copied" checkmark could flash even when the copy failed in a non-secure context.

This PR introduces a small shared module src/util/language-preference.ts with:

  • LANGUAGE_STORAGE_KEY — single source of truth for the Starlight synced-tabs key
  • copyToClipboard(text) — wraps the clipboard write and returns whether it succeeded

and wires it in everywhere:

  • Processed scripts (HeroSection, AgentSetupCard, CodeBlock) import the helpers directly.
  • The two inline scripts (LanguageToggle, Syntax) receive the key via define:vars, since is:inline scripts cannot import modules. Their tuned internals (idempotency guards, djb2-derived key contract) are otherwise untouched.
  • All copy buttons now only show the "copied" state on a successful write.

Note: stacked on #2778 (which is stacked on #2775). Until those merge, this diff includes their commits. The net change unique to this PR is the new util module plus the key/clipboard wiring in the six files above.

Related Issues

Follow-up to review feedback on #2775 and #2778.

Documentation PR

No documentation changes needed.

Type of Change

Other (please describe): refactor / maintainability cleanup, no behavior change for end users

Testing

  • npm run typecheck and npm run build (656 pages, no broken links) pass in site/

  • Verified the built inline scripts receive the injected key (define:vars emits const STORAGE_KEY = "starlight-synced-tabs__jarkqt" in both LanguageToggle and Syntax); no hardcoded literal remains outside the shared module

  • Verified language sync end-to-end in a headless browser:

    • Homepage install-copy and header toggle both write the key and pin correctly
    • Docs page: selecting Python via the header toggle syncs the toggle state and the Starlight tabs, and persists across reload (this exercises both inline scripts that now get the key via define:vars)
    • All copy buttons (CodeBlock, hero install, agent prompt) copy correctly
    • Forced a clipboard rejection (simulating a non-secure context): the checkmark no longer flashes on a failed copy
    • No new console errors
  • I ran hatch run prepare

Checklist

  • I have read the CONTRIBUTING document
  • I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
  • My change is focused and reasonably small; I have split unrelated work into separate PRs
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@github-actions github-actions Bot added documentation Documentation changes, improvements, additions, content updates, site improvements, examples, guides chore Maintenance tasks, dependency updates, CI changes, refactoring with no user-facing impact strands-running labels Jun 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Comment (approve once tests are addressed)

Clean, well-scoped refactor. Extracting LANGUAGE_STORAGE_KEY into a single source of truth and routing all three copy buttons through copyToClipboard removes the duplicated magic string and fixes the real "checkmark flashes on a failed copy" bug. I verified no hardcoded key and no raw navigator.clipboard.writeText remain outside the new module, and the define:vars wiring is the correct way to feed the key into is:inline scripts. Module docstrings are excellent and explain the djb2/Starlight key contract well.

Review themes
  • Testing: The new util module has no unit tests despite an established site/test/ convention for utils; the copyToClipboard success/failure branch is the core fix and is trivial to cover. (See inline comment — main item to address.)
  • Behavior: One question on whether language pinning in HeroSection should be gated on a successful copy, since it currently runs even when the clipboard write fails.
  • Robustness: Minor early-return suggestion for the empty data-copy edge case in AgentSetupCard.

Note: the diff surfaces the stacked #2778/#2775 commits, so I focused my review on the changes unique to this PR (the util module + key/clipboard wiring). Nice cleanup overall.

@github-actions

github-actions Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Documentation Preview Ready

Your documentation preview has been successfully deployed!

Preview URL: https://d3ehv1nix5p99z.cloudfront.net/pr-cms-2779/docs/user-guide/quickstart/overview/

Updated at: 2026-06-16T18:15:26.448Z

@github-actions

Copy link
Copy Markdown
Contributor

Re-review of f4295cd (use shared pathWithBase in hero, drop baseUrl prop)

Nice follow-up — this is a clean consistency improvement. Verified:

  • HeroSection drops its local withBase helper plus the Props/baseUrl plumbing and now imports pathWithBase from util/links, matching how AgentSetupCard, LabsEvalsSection, StatsSection, and the other landing sections resolve base paths.
  • index.astro correctly removes the now-unused base const and the prop threading; <HeroSection /> renders with no props.
  • The single remaining caller (pathWithBase('/docs/user-guide/quickstart/overview/')) is correct.

No new issues from this commit. The two items from my earlier review remain open (they're unrelated to this refactor): the new language-preference util still has no unit tests, and the HeroSection language pin still fires even when the clipboard copy fails. Happy to re-approve once the test gap is closed.

@yonib05 yonib05 force-pushed the homepage-shared-utils branch from 06365a3 to e6b93f6 Compare June 16, 2026 14:50
@github-actions

Copy link
Copy Markdown
Contributor

Re-review of e6b93f6 (use shared language key in static-load LangCodeBlock)

Clean follow-up that completes the dedup goal. Verified:

  • LangCodeBlock.astro's <script> is a processed script (not is:inline), so importing LANGUAGE_STORAGE_KEY from util/language-preference is the correct mechanism here (rather than define:vars).
  • grep confirms zero hardcoded starlight-synced-tabs__jarkqt literals remain anywhere outside the shared module — the key is now a true single source of truth across processed scripts (import) and inline scripts (define:vars).

No new issues from this commit. The two items from my earlier reviews remain open and unrelated to this change: the language-preference util still has no unit tests, and the HeroSection language pin still fires even when the clipboard copy fails. Adding a small language-preference.test.ts covering the copyToClipboard success/failure branches is the main thing standing between this and an approval.

@yonib05 yonib05 force-pushed the homepage-shared-utils branch from e6b93f6 to ad957c9 Compare June 16, 2026 17:35
@github-actions

Copy link
Copy Markdown
Contributor

Re-review after rebase (HEAD ad957c9)

The branch has been rebased, so the diff is now nicely isolated to this PR's scope. Re-verified the full change set via the file diffs:

  • All three copy paths (CodeBlock, HeroSection install, AgentSetupCard prompt) gate the "copied" checkmark on a successful copyToClipboard, so the checkmark no longer flashes on a failed copy. ✅
  • Both is:inline scripts (LanguageToggle, Syntax) receive the key via define:vars and the processed scripts (CodeBlock, HeroSection, LangCodeBlock) import it directly — LANGUAGE_STORAGE_KEY is a true single source of truth with no remaining hardcoded literals. ✅
  • aria-hidden on the copy/check SVGs is a nice incidental a11y touch.

Two items I can now close out:

  • HeroSection pin-on-failed-copy — withdrawing this. Clicking the language-specific install command is a reasonable signal of user intent to use that language regardless of whether the clipboard write succeeded; gating only the checkmark on success is the right split. No change needed.
  • AgentSetupCard empty data-copy — the || '' fallback handles it harmlessly; not worth churn.

Remaining (and only) substantive item: the language-preference util still has no unit test. The copyToClipboard success/failure branch is the core behavioral fix in this PR and is trivial to cover (mock navigator.clipboard.writeText to resolve/reject), and site/test/ already has an established convention for util tests. Adding a small language-preference.test.ts is the one thing standing between this and an approval from me.

Otherwise this is a clean, well-documented refactor — nice work.

yonib05 added 4 commits June 16, 2026 14:08
Extract the Starlight synced-tabs storage key and a copyToClipboard helper
into a shared src/util/language-preference module. Processed scripts import
it directly; the inline header and syntax-switch scripts receive the key via
define:vars. Routes all copy buttons through the helper, fixing the
pre-existing gap where a failed clipboard write still showed a copied state.
HeroSection's local withBase helper had a single remaining caller after the
agent card moved to the shared util. Replace it with pathWithBase and remove
the baseUrl prop and its threading through index.astro, matching how sibling
landing sections resolve base paths.
@github-actions

Copy link
Copy Markdown
Contributor

Re-review after rebase (HEAD 4c22c3b)

This is a clean rebase onto updated main — the change set is functionally identical to my previous re-review (only a 1-line context shift in HeroSection from the rebase), so I won't repeat the full analysis. All prior verifications still hold.

My position is unchanged: the only outstanding item is the missing unit test for site/src/util/language-preference.ts. Once a small language-preference.test.ts covers the copyToClipboard success/failure branches (mock navigator.clipboard.writeText to resolve/reject), I'm happy to approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance tasks, dependency updates, CI changes, refactoring with no user-facing impact documentation Documentation changes, improvements, additions, content updates, site improvements, examples, guides size/l

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant