Skip to content

Replace hardcoded DANDI identifiers with dynamic instance config#2765

Open
yarikoptic wants to merge 5 commits into
masterfrom
bf-2762
Open

Replace hardcoded DANDI identifiers with dynamic instance config#2765
yarikoptic wants to merge 5 commits into
masterfrom
bf-2762

Conversation

@yarikoptic
Copy link
Copy Markdown
Member

@yarikoptic yarikoptic commented Apr 2, 2026

Summary

  • Replace all hardcoded DANDI: identifier prefixes, DANDI Archive publisher names, and SCR_017571 RRID in frontend code with dynamic values from the backend /api/info/ endpoint (DJANGO_DANDI_INSTANCE_NAME, DJANGO_DANDI_INSTANCE_IDENTIFIER)
  • Add a shared Pinia store (stores/instance.ts) that caches instance config across components
  • Add a grep-based lint check (scripts/check-no-hardcoded-dandi.sh) to prevent re-introducing hardcoded identifiers, wired into pre-commit and tox -e lint
  • Update E2E tests to fetch instance config from the API instead of asserting hardcoded values
  • Closes Bug: Identifier under How to Cite does not appear to use "vendorized" value #2762
  • Fixes some AI slop left for listing of identifiers and composition of them for bibtex key and notes
Files changed overview
File Change
web/src/stores/instance.ts New — shared Pinia store for instance config
web/src/utils/cff.ts All citation format functions accept instanceName parameter
web/src/components/DLP/HowToCiteTab.vue Dynamic identifier, archive name, RRID
web/src/views/DandisetLandingView/DownloadDialog.vue Dynamic identifier prefix and instance URL check
web/src/components/DandisetList.vue Use shared instance store
web/src/directives/index.ts Dynamic page title
scripts/check-no-hardcoded-dandi.sh New — lint check for hardcoded identifiers
.pre-commit-config.yaml Add no-hardcoded-dandi-identifiers hook
tox.ini Run the check in lint environment
e2e/utils.ts Add fetchInstanceConfig() helper
e2e/tests/dandisetLandingPage.spec.ts Use API instance config in assertions
e2e/tests/dandisetsPage.spec.ts Use API instance config in assertions

Tests etc plan

  • npm run lint passes (web/)
  • npm run type-check passes (web/)
  • uv run tox -e lint passes (includes hardcoded identifier check)
  • Seek feedback from @dandi/archive-maintainers on the overall approach
  • Pre-commit hooks pass
  • Frontend CI passes
  • E2E tests pass with DJANGO_DANDI_INSTANCE_NAME=DEV-DANDI
  • Backend CI passes (no backend changes)
  • Move from Draft after CI turns green

🤖 Generated with Claude Code

I also fixed/adjusted for some left-over AI slop, as duplicate identifiers and inconsistent presentation

e.g. compare bibtex cite as on https://dandiarchive.org/dandiset/000690?pos=5 having
image

and

image

to https://deploy-preview-2765--sandbox-dandiarchive-org.netlify.app/dandiset/217838?pos=2

image

and two separate copy pasteable sections for identifiers

image

there is also a fix to bibtex record key. Can also look current sandbox at e.g. https://sandbox.dandiarchive.org/dandiset/217838?pos=2

The frontend hardcoded "DANDI:" as the identifier prefix, "DANDI Archive"
as the publisher name, and "SCR_017571" as the RRID in citations, download
commands, and the "How to Cite" tab. On non-primary instances (e.g.,
EMBER-DANDI), these values were incorrect.

All instance-specific values now come from the backend /api/info/ endpoint
(DJANGO_DANDI_INSTANCE_NAME, DJANGO_DANDI_INSTANCE_IDENTIFIER) via a new
shared Pinia store (stores/instance.ts).

Changes:
- Add stores/instance.ts: shared Pinia store caching instance config
- cff.ts: all citation format functions accept instanceName parameter
- HowToCiteTab.vue: dynamic identifier prefix, archive name, and RRID
- DownloadDialog.vue: dynamic identifier prefix and instance URL check
- DandisetList.vue: use shared instance store instead of ad-hoc API call
- directives/index.ts: dynamic page title from instance store
- Add scripts/check-no-hardcoded-dandi.sh: lint check for hardcoded
  DANDI identifiers in .vue/.ts files
- .pre-commit-config.yaml: add no-hardcoded-dandi-identifiers hook
- tox.ini: run the check as part of the lint environment
- E2E tests: fetch instance config from API instead of hardcoding values

Closes #2762

Co-Authored-By: Claude Code 2.1.90 / Claude Opus 4.6 <noreply@anthropic.com>
yarikoptic and others added 2 commits April 2, 2026 14:37
dandiIdentifier already included the instance name prefix, causing
BibTeX note and RIS N1 fields to produce e.g. "DANDI:DANDI:217838/...".

Fix by having dandisetVersionIdentifier (renamed from dandiIdentifier)
return just "identifier/version" without the prefix. The prefix is added
in the template for display and in cff functions for citations.

Also rename local variables for clarity:
- dandiIdentifier -> dandisetVersionIdentifier
- name -> instance_name in formattedCitations
- identifier -> dandisetVersionIdentifier in cff.ts function params

Co-Authored-By: Claude Code 2.1.90 / Claude Opus 4.6 <noreply@anthropic.com>
…tifier"

Rename the "DANDI Identifier" section to "Dandiset Identifier" (static
heading, instance-agnostic). Move the archive RRID line into its own
copy-pasteable "Archive Identifier" section (shown only when
instance_identifier is configured). Both sections have copy buttons.

Co-Authored-By: Claude Code 2.1.90 / Claude Opus 4.6 <noreply@anthropic.com>
@yarikoptic yarikoptic added UX Affects usability of the system patch Increment the patch version when merged vendorization labels Apr 2, 2026
@yarikoptic yarikoptic marked this pull request as ready for review April 2, 2026 19:23
@yarikoptic yarikoptic requested a review from Copilot April 3, 2026 21:15
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 removes hardcoded DANDI-specific identifier/publisher/RRID strings from the frontend by sourcing instance-specific values from /api/info/, and adds guardrails (store + lint + E2E updates) to prevent regressions across deployments with different instance branding/config.

Changes:

  • Added a shared Pinia instance-config store backed by /api/info/ and updated components to use it for identifier/publisher display.
  • Updated citation/CFF/BibTeX/RIS formatting utilities to accept instance-specific naming/URLs.
  • Added a grep-based lint/pre-commit/tox check to block reintroducing hardcoded identifiers and updated Playwright E2E assertions to use API-provided config.

Reviewed changes

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

Show a summary per file
File Description
web/src/stores/instance.ts New Pinia store caching instance config from /api/info/.
web/src/utils/cff.ts Parameterized citation outputs (publisher/note/URLs/keys) with instance values.
web/src/components/DLP/HowToCiteTab.vue Uses instance config for identifier + archive identifier display and citation generation.
web/src/views/DandisetLandingView/DownloadDialog.vue Uses instance config for X:<id> download identifier and instance URL check.
web/src/components/DandisetList.vue Uses instance store instead of per-component /api/info/ fetch.
web/src/directives/index.ts Makes page title instance-aware (with fallback).
scripts/check-no-hardcoded-dandi.sh New lint script to detect hardcoded DANDI identifiers/publisher/RRID in frontend.
.pre-commit-config.yaml Adds local pre-commit hook to run the hardcoded-identifier check.
tox.ini Runs the hardcoded-identifier check in the lint env.
e2e/utils.ts Adds fetchInstanceConfig() helper for instance-aware assertions.
e2e/tests/dandisetLandingPage.spec.ts Updates “How to Cite” assertions to use instance config.
e2e/tests/dandisetsPage.spec.ts Updates dandiset list assertions to use instance config.

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

Comment thread web/src/views/DandisetLandingView/DownloadDialog.vue Outdated
Comment on lines +13 to +18
state: (): InstanceState => ({
instanceName: '',
instanceIdentifier: null,
instanceUrl: null,
loaded: false,
}),
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

The store initializes instanceName to an empty string. Several call sites build identifiers/citation keys using this value, which can temporarily render malformed prefixes like :<id> before the async fetch completes. Consider using a non-empty default/fallback (e.g., the legacy instance name) or exposing a computed that falls back until loaded is true.

Copilot uses AI. Check for mistakes.
Comment on lines +20 to +28
async fetchInstanceInfo() {
if (this.loaded) {
return;
}
const info = await dandiRest.info();
this.instanceName = info.instance_config.instance_name;
this.instanceIdentifier = info.instance_config.instance_identifier;
this.instanceUrl = info.instance_config.instance_url;
this.loaded = true;
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

fetchInstanceInfo() only guards on loaded, so multiple components mounting at once can trigger multiple concurrent /api/info/ requests before loaded flips to true. Consider tracking an in-flight promise (or setting/loading state) so concurrent callers share the same request and you avoid duplicated network traffic.

Copilot uses AI. Check for mistakes.

const store = useDandisetStore();
const instanceStore = useInstanceStore();
onMounted(() => instanceStore.fetchInstanceInfo());
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

onMounted(() => instanceStore.fetchInstanceInfo()) starts an async action without awaiting or handling failures. If /api/info/ errors, this becomes an unhandled rejection and the UI may render with an empty instance name. Consider making the mounted hook async and awaiting the fetch (or handling errors explicitly with .catch(...)).

Suggested change
onMounted(() => instanceStore.fetchInstanceInfo());
onMounted(async () => {
try {
await instanceStore.fetchInstanceInfo();
}
catch (error) {
console.error('Failed to fetch instance info.', error);
}
});

Copilot uses AI. Check for mistakes.
Comment thread web/src/components/DLP/HowToCiteTab.vue Outdated
Comment on lines +349 to +360
const dandiset_version_identifier = dandisetVersionIdentifier.value;
const instance_name = instanceName.value;

return {
apa: cffToAPA(cffObject.value),
mla: cffToMLA(cffObject.value),
chicago: cffToChicago(cffObject.value),
harvard: cffToHarvard(cffObject.value),
vancouver: cffToVancouver(cffObject.value),
ieee: cffToIEEE(cffObject.value),
bibtex: cffToBibTeX(cffObject.value, identifier),
ris: cffToRIS(cffObject.value, identifier),
apa: cffToAPA(cffObject.value, instance_name),
mla: cffToMLA(cffObject.value, instance_name),
chicago: cffToChicago(cffObject.value, instance_name),
harvard: cffToHarvard(cffObject.value, instance_name),
vancouver: cffToVancouver(cffObject.value, instance_name),
ieee: cffToIEEE(cffObject.value, instance_name),
bibtex: cffToBibTeX(cffObject.value, dandiset_version_identifier, instance_name),
ris: cffToRIS(cffObject.value, dandiset_version_identifier, instance_name),
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

New local variables use snake_case (dandiset_version_identifier, instance_name) in TypeScript. This is inconsistent with the surrounding code’s camelCase naming and makes the code harder to scan/grep. Prefer dandisetVersionIdentifier / instanceName (or reuse existing computed names) for consistency.

Copilot uses AI. Check for mistakes.
Comment on lines +5 to +8
function getTitle(): string {
const instanceStore = useInstanceStore();
return instanceStore.instanceName ? `${instanceStore.instanceName} Archive` : 'DANDI Archive';
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

The page title is derived from instanceStore.instanceName, but this module never triggers fetchInstanceInfo(). On routes that don't otherwise load instance config, the title will remain the fallback forever. Consider fetching instance info once at app startup (e.g., in App.vue/router guard) or having the directive initiate a one-time fetch (with caching) so titles reliably reflect the instance.

Copilot uses AI. Check for mistakes.
Comment thread scripts/check-no-hardcoded-dandi.sh Outdated
Comment on lines +33 to +48
if grep -nE '("|'\''|`)\s*DANDI:' "$file" >/dev/null 2>&1; then
echo "$file: found hardcoded 'DANDI:' identifier prefix — use instanceName from the instance store"
ERRORS=$((ERRORS + 1))
fi

# Check for lowercase dandi: used as citation key prefix (followed by digits or ${)
# Excludes schema URIs like dandi:OpenAccess, dandi:EmbargoedAccess
if grep -nE '("|'\''|`)\s*dandi:\$\{' "$file" >/dev/null 2>&1 || \
grep -nE '("|'\''|`)\s*dandi:[0-9]' "$file" >/dev/null 2>&1; then
echo "$file: found hardcoded 'dandi:' identifier prefix — use instanceName.toLowerCase() from the instance store"
ERRORS=$((ERRORS + 1))
fi

# Check for "DANDI Archive" as publisher name in string literals
# Exclude HTML comments (<!-- ... -->)
if grep -nE "(\"|\`|').*DANDI Archive" "$file" | grep -vE '^\s*<!--' >/dev/null 2>&1; then
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

The regexes use \s with grep -E (e.g., \s*DANDI: and ^\s*<!--). \s is not portable in ERE (not supported by BSD grep and some GNU grep configs), which can make this hook silently miss matches on some developer machines/CI images. Prefer POSIX character classes like [[:space:]]* / ^[[:space:]]*<!-- for whitespace matching.

Suggested change
if grep -nE '("|'\''|`)\s*DANDI:' "$file" >/dev/null 2>&1; then
echo "$file: found hardcoded 'DANDI:' identifier prefix — use instanceName from the instance store"
ERRORS=$((ERRORS + 1))
fi
# Check for lowercase dandi: used as citation key prefix (followed by digits or ${)
# Excludes schema URIs like dandi:OpenAccess, dandi:EmbargoedAccess
if grep -nE '("|'\''|`)\s*dandi:\$\{' "$file" >/dev/null 2>&1 || \
grep -nE '("|'\''|`)\s*dandi:[0-9]' "$file" >/dev/null 2>&1; then
echo "$file: found hardcoded 'dandi:' identifier prefix — use instanceName.toLowerCase() from the instance store"
ERRORS=$((ERRORS + 1))
fi
# Check for "DANDI Archive" as publisher name in string literals
# Exclude HTML comments (<!-- ... -->)
if grep -nE "(\"|\`|').*DANDI Archive" "$file" | grep -vE '^\s*<!--' >/dev/null 2>&1; then
if grep -nE '("|'\''|`)[[:space:]]*DANDI:' "$file" >/dev/null 2>&1; then
echo "$file: found hardcoded 'DANDI:' identifier prefix — use instanceName from the instance store"
ERRORS=$((ERRORS + 1))
fi
# Check for lowercase dandi: used as citation key prefix (followed by digits or ${)
# Excludes schema URIs like dandi:OpenAccess, dandi:EmbargoedAccess
if grep -nE '("|'\''|`)[[:space:]]*dandi:\$\{' "$file" >/dev/null 2>&1 || \
grep -nE '("|'\''|`)[[:space:]]*dandi:[0-9]' "$file" >/dev/null 2>&1; then
echo "$file: found hardcoded 'dandi:' identifier prefix — use instanceName.toLowerCase() from the instance store"
ERRORS=$((ERRORS + 1))
fi
# Check for "DANDI Archive" as publisher name in string literals
# Exclude HTML comments (<!-- ... -->)
if grep -nE "(\"|\`|').*DANDI Archive" "$file" | grep -vE '^[[:space:]]*<!--' >/dev/null 2>&1; then

Copilot uses AI. Check for mistakes.
Comment thread e2e/utils.ts Outdated
archiveName.value = info.instance_config.instance_name;
});
const instanceStore = useInstanceStore();
onMounted(() => instanceStore.fetchInstanceInfo());
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

onMounted(() => instanceStore.fetchInstanceInfo()) fires an async action without awaiting/handling failures. If /api/info/ errors, this will surface as an unhandled rejection and archiveName remains empty. Consider using an async mounted hook with await, or explicitly handling errors.

Suggested change
onMounted(() => instanceStore.fetchInstanceInfo());
onMounted(async () => {
try {
await instanceStore.fetchInstanceInfo();
} catch (error) {
console.error('Failed to fetch instance info.', error);
}
});

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@bendichter bendichter left a comment

Choose a reason for hiding this comment

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

Review

Overall approach is sound — centralizing instance config in a Pinia store is the right call, and the lint script + pre-commit hook is a nice guardrail. The BibTeX/RIS double-prefix fix is a real bug fix too. A few issues to address:

Issues

1. Race condition in instance store (web/src/stores/instance.ts)

Multiple components (HowToCiteTab, DandisetList, DownloadDialog) all call fetchInstanceInfo() on mount. Since the guard is just if (this.loaded) return, concurrent callers before the first fetch completes will fire multiple /api/info/ requests. Should track an in-flight promise so concurrent callers share the same request.

2. Remaining hardcoded URL in HowToCiteTab.vue:~369

The dandiUrl computed property still has a hardcoded fallback:

return `https://dandiarchive.org/dandiset/${identifier}`;

This is used in "Materials and Methods" and "Data Availability Statement" sections. Should use instanceStore.instanceUrl instead, otherwise non-primary instances link to the wrong archive.

3. Double API call in DownloadDialog.vue

onMounted calls dandiRest.info() directly (for CLI version info) and then instanceStore.fetchInstanceInfo() which internally also calls dandiRest.info(). Should consolidate to avoid the redundant request.

4. No app-level initialization of instance store

directives/index.ts reads from the store but never triggers a fetch. On routes that don't mount HowToCiteTab, DandisetList, or DownloadDialog, the page title stays at the fallback "DANDI Archive" forever. Consider fetching once at app startup (e.g., in App.vue or a router guard).

5. instanceName defaults to empty string

Before the fetch completes, the identifier briefly renders as :<id>/draft (empty prefix). A default like 'DANDI' would prevent the flash of malformed text.

Minor

  • snake_case local variables (dandiset_version_identifier, instance_name) in HowToCiteTab.vue — should be camelCase per project convention.
  • Lint script uses \s in grep -E which is not POSIX-portable (BSD grep on macOS). Consider [[:space:]].
  • fetchInstanceConfig() in e2e/utils.ts doesn't check resp.ok before parsing JSON.

- Fix race condition in instance store: deduplicate concurrent
  fetchInstanceInfo() calls by tracking an in-flight promise
- Fix remaining hardcoded dandiarchive.org URL in HowToCiteTab dandiUrl
  computed property — now uses instanceStore.instanceUrl
- Initialize instance store at app startup in main.ts so page titles
  work on all routes, not just those mounting specific components
- Reorder DownloadDialog.vue onMounted to use cached instance store
  fetch before calling dandiRest.info() for CLI version info
- Fix snake_case local variables in HowToCiteTab to use camelCase
- Use POSIX-portable [[:space:]] instead of \s in lint script grep
- Add resp.ok check in E2E fetchInstanceConfig for clearer errors

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix instance store race condition, hardcoded URL, and minor issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch Increment the patch version when merged UX Affects usability of the system vendorization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Identifier under How to Cite does not appear to use "vendorized" value

3 participants