Conversation
Contributor
📝 WalkthroughWalkthroughThe PR refactors the version-link injection in the build script, removes Dev Version navigation links from multiple static pages, adds removed-column state and TSV export flow to the SDRF builder, and updates site SDRF data with reordered entries and a new timestamp. Changes
Sequence Diagram(s)(omitted — changes are multiple but do not introduce a new multi-component sequential feature warranting a sequence diagram) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
site/js/sdrf-builder.js (1)
855-870:⚠️ Potential issue | 🟠 MajorPrune factor selections when the backing characteristic is removed.
This handler removes the column from the picker state, but it never clears
state.factorValues. If the removed column was selected as a factor,resolveColumns()still emitsfactor value[...]without the matchingcharacteristics[...]column, so the generated SDRF becomes inconsistent.Suggested fix
if (!isHardLocked) { var colName = col.name; item.addEventListener('click', function () { + var factorIdx = state.factorValues.indexOf(colName); + if (factorIdx !== -1) { + state.factorValues.splice(factorIdx, 1); + } + if (isExtra) { // Remove from extra var idx = state.extraColumns.indexOf(colName); if (idx !== -1) state.extraColumns.splice(idx, 1); } else if (col.requirement === 'recommended') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/js/sdrf-builder.js` around lines 855 - 870, When removing a column inside the click handler (the block that mutates state.extraColumns, state.removedColumns, or state.addedOptionals), also remove any factor selections that reference that column from state.factorValues so resolveColumns() won't emit factor value[...] without a matching characteristics[...] entry; locate the handler around the item.addEventListener('click', ...) and after splicing the column out, delete or filter out entries in state.factorValues keyed by or containing colName (or remove colName from any arrays stored there), then call updatePreview() as is so the preview/resolution is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/inject-headers.py`:
- Around line 41-46: The Stable Version link is hard-coded to "/" which breaks
docs served from subpaths; replace that literal with a computed stable target
derived from the deployment base or page depth (e.g., compute a relative base
like "../" repeated page_depth or use an existing deployment_base variable if
present) and use that value when building version_link instead of "/". Update
the block that sets version_link (refer to is_dev, VERSION_LINK_PLACEHOLDER,
header_html, resolved_header) to compute stable_target = computed_base + maybe
"index.html" or root of the docs site and then set version_link = f'<a
href="{stable_target}" class="version-link">Stable Version</a>'; apply the same
change to the analogous footer/footer injection code around lines 71-76 so both
injections derive the stable URL the same way.
In `@site/sdrf-data.json`:
- Around line 358-359: The organism labels in the serialized JSON are not
canonicalized, causing duplicates like "mus musculus"/"Mus musculus" to appear;
normalize organism names before writing sdrf-data.json by converting them to a
consistent canonical form (e.g., Title Case "Mus musculus" or a chosen lowercase
canonical form) at the place where the organisms list is assembled (the code
path that populates statistics.organisms / the array that currently contains
"mus musculus" and "homo sapiens"); update the normalization logic to apply to
all occurrences (including the entries around the shown rows and the other
reported ranges 2057-2058 and 3955-3957) so serialization only emits the
canonical label.
- Around line 1871-1875: The disease labels in the generator are not normalized,
causing duplicate keys in statistics.diseases (e.g., "Mlignant Melanoma",
"squamous cell carcinoma", "'Crohn''s Disease'"); update the code that
emits/aggregates the disease field (the part that populates statistics.diseases)
to normalize labels by trimming whitespace, collapsing multiple spaces to one,
un-escaping doubled single quotes, and applying small canonicalization fixes
(e.g., "Mlignant" -> "Malignant") or a short mapping table for common typos;
ensure the normalization runs before using the string as the key so "Breast
Cancer" and "Breast Cancer" or variants map to the same canonical bucket.
---
Outside diff comments:
In `@site/js/sdrf-builder.js`:
- Around line 855-870: When removing a column inside the click handler (the
block that mutates state.extraColumns, state.removedColumns, or
state.addedOptionals), also remove any factor selections that reference that
column from state.factorValues so resolveColumns() won't emit factor value[...]
without a matching characteristics[...] entry; locate the handler around the
item.addEventListener('click', ...) and after splicing the column out, delete or
filter out entries in state.factorValues keyed by or containing colName (or
remove colName from any arrays stored there), then call updatePreview() as is so
the preview/resolution is consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d821bb71-f2d5-49b9-8eae-0118f3a48dbd
⛔ Files ignored due to path filters (1)
psi-document/sdrf-proteomics-specification-v1.1.0-dev.pdfis excluded by!**/*.pdf
📒 Files selected for processing (7)
scripts/inject-headers.pysite/js/sdrf-builder.jssite/quickstart.htmlsite/sdrf-builder.htmlsite/sdrf-data.jsonsite/sdrf-explorer.htmlsite/sdrf-terms.html
💤 Files with no reviewable changes (4)
- site/sdrf-terms.html
- site/quickstart.html
- site/sdrf-explorer.html
- site/sdrf-builder.html
Comment on lines
+41
to
46
| # Resolve version link: only show in dev builds (link back to stable) | ||
| if is_dev: | ||
| version_link = '<a href="/" class="version-link">Stable Version</a>' | ||
| else: | ||
| version_link = '<a href="/dev/" class="version-link">Dev Version</a>' | ||
| version_link = '' | ||
| resolved_header = header_html.replace(VERSION_LINK_PLACEHOLDER, version_link) |
Contributor
There was a problem hiding this comment.
Avoid hard-coding the Stable Version link to "/".
Every other nav URL in this script is relative, but these two injections jump to the domain root. That breaks the version switch when the docs are served from a project subpath or opened from a generated local build. Please derive the stable target from the deployment base/page depth instead of baking in "/".
Also applies to: 71-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/inject-headers.py` around lines 41 - 46, The Stable Version link is
hard-coded to "/" which breaks docs served from subpaths; replace that literal
with a computed stable target derived from the deployment base or page depth
(e.g., compute a relative base like "../" repeated page_depth or use an existing
deployment_base variable if present) and use that value when building
version_link instead of "/". Update the block that sets version_link (refer to
is_dev, VERSION_LINK_PLACEHOLDER, header_html, resolved_header) to compute
stable_target = computed_base + maybe "index.html" or root of the docs site and
then set version_link = f'<a href="{stable_target}" class="version-link">Stable
Version</a>'; apply the same change to the analogous footer/footer injection
code around lines 71-76 so both injections derive the stable URL the same way.
Comment on lines
+358
to
+359
| "mus musculus", | ||
| "homo sapiens" |
Contributor
There was a problem hiding this comment.
Canonicalize organism labels before serializing.
These touched rows still mix mus musculus/homo sapiens with Mus musculus/Homo sapiens. statistics.organisms already shows the fallout in Lines 7-10, where the same species is split across multiple exact-string buckets.
Also applies to: 2057-2058, 3955-3957
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site/sdrf-data.json` around lines 358 - 359, The organism labels in the
serialized JSON are not canonicalized, causing duplicates like "mus
musculus"/"Mus musculus" to appear; normalize organism names before writing
sdrf-data.json by converting them to a consistent canonical form (e.g., Title
Case "Mus musculus" or a chosen lowercase canonical form) at the place where the
organisms list is assembled (the code path that populates statistics.organisms /
the array that currently contains "mus musculus" and "homo sapiens"); update the
normalization logic to apply to all occurrences (including the entries around
the shown rows and the other reported ranges 2057-2058 and 3955-3957) so
serialization only emits the canonical label.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
site/js/sdrf-builder.js (1)
917-926: Replace deprecatedunescape()and address URL length limits.The current implementation has two concerns:
- Deprecated function:
unescape()is deprecated and should be avoided.- URL length limits: Base64 encoding adds ~33% overhead. Templates with 30+ columns will likely exceed browser URL limits (2,000–8,000 characters), potentially breaking the editor link.
Consider using
sessionStoragefor larger payloads:♻️ Alternative using sessionStorage
function openInEditor() { var columns = resolveColumns(); if (!columns.length) return; var headers = []; var values = []; for (var i = 0; i < columns.length; i++) { headers.push(columns[i].name); values.push(columns[i].example_value || ''); } var tsv = headers.join('\t') + '\n' + values.join('\t') + '\n'; - var encoded = btoa(unescape(encodeURIComponent(tsv))); - window.open('sdrf-editor.html?content=' + encodeURIComponent(encoded)); + var storageKey = 'sdrf-builder-content-' + Date.now(); + try { + sessionStorage.setItem(storageKey, tsv); + window.open('sdrf-editor.html?storageKey=' + encodeURIComponent(storageKey)); + } catch (e) { + var encoded = btoa(unescape(encodeURIComponent(tsv))); + window.open('sdrf-editor.html?content=' + encodeURIComponent(encoded)); + } }Note: The editor will need to check for and handle the
storageKeyparameter in addition to the currentcontentparameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/js/sdrf-builder.js` around lines 917 - 926, Replace the deprecated unescape-based base64 encoding and avoid long URLs by encoding TSV safely and falling back to sessionStorage: build headers/values and tsv as before, then create a safe base64 string for tsv using a TextEncoder + a base64 encoder (replace the unescape(encodeURIComponent(...)) pattern used when creating encoded), check the encoded length and if it exceeds a chosen threshold (e.g., ~2000 chars) write the raw TSV or base64 to sessionStorage under a generated key (storageKey) and call window.open('sdrf-editor.html?storageKey=' + storageKey), otherwise call window.open('sdrf-editor.html?content=' + encodeURIComponent(encoded)); update the editor to accept storageKey (it will read sessionStorage[storageKey]) and keep references to the existing variables/functionality: columns, headers, values, tsv, encoded, and the window.open call so the change is localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@site/js/sdrf-builder.js`:
- Around line 917-926: Replace the deprecated unescape-based base64 encoding and
avoid long URLs by encoding TSV safely and falling back to sessionStorage: build
headers/values and tsv as before, then create a safe base64 string for tsv using
a TextEncoder + a base64 encoder (replace the unescape(encodeURIComponent(...))
pattern used when creating encoded), check the encoded length and if it exceeds
a chosen threshold (e.g., ~2000 chars) write the raw TSV or base64 to
sessionStorage under a generated key (storageKey) and call
window.open('sdrf-editor.html?storageKey=' + storageKey), otherwise call
window.open('sdrf-editor.html?content=' + encodeURIComponent(encoded)); update
the editor to accept storageKey (it will read sessionStorage[storageKey]) and
keep references to the existing variables/functionality: columns, headers,
values, tsv, encoded, and the window.open call so the change is localized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07bfb38e-0ff3-466c-9f9e-f9ace268a501
⛔ Files ignored due to path filters (1)
psi-document/sdrf-proteomics-specification-v1.1.0-dev.pdfis excluded by!**/*.pdf
📒 Files selected for processing (1)
site/js/sdrf-builder.js
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.
Summary by CodeRabbit
New Features
Chores