Update experiment navigation to include experiment name in URLs#1417
Merged
Update experiment navigation to include experiment name in URLs#1417
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRouting and navigation were switched to use experiment-name-based dynamic paths. A nested Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Menu as SelectExperimentMenu
participant Router as Router (navigate)
participant Layout as ExperimentLayout
participant Context as ExperimentContext
participant Analytics as PageTracker
User->>Menu: click experiment item (name)
Menu->>Router: navigate to /experiment/{encodedName}/{restOfPath}
Router->>Layout: route -> ExperimentLayout with :experimentName
Layout->>Context: setExperimentId(experimentId) // sync context from name
Layout->>Analytics: analytics.page(normalizePath(currentPath))
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
To test this, start the app, select an experiment and navigate around. Copy the URL. Then change experiments. Then try loading the copied URL you had and the experiment should change |
Paragon SummaryThis pull request review identified 4 issues across 2 categories in 7 files. The review analyzed code changes, potential bugs, security vulnerabilities, performance issues, and code quality concerns using automated analysis tools. This PR updates the experiment navigation to include the experiment name in URLs, enabling users to share direct links to specific experiments with others. Key changes:
Confidence score: 3/5
7 files reviewed, 4 comments Severity breakdown: High: 2, Medium: 2 Tip: |
![paragon-review[bot]](https://avatars.githubusercontent.com/in/1544258?s=60&v=4){kind=small}
|
|
||
| const handleEmbeddingModelClick = () => { | ||
| navigate('/experiment/embedding-model', { | ||
| navigate(`/experiment/${experimentInfo?.name}/embedding-model`, { |
There was a problem hiding this comment.
Bug: Navigation uses optional chaining without a fallback
Navigation uses optional chaining without a fallback. Users are redirected to broken literal undefined URLs. Guard the navigation call with an explicit check.
View Details
Location: src/renderer/components/Experiment/Foundation/CurrentFoundationInfo.tsx (lines 376)
Analysis
Navigation uses optional chaining without a fallback
| What fails | Navigation redirects to the literal string 'undefined' in the URL path. |
| Result | The browser navigates to an invalid path like /experiment/undefined/embedding-model. |
| Expected | Navigation should abort or wait until the experiment name is fully loaded. |
| Impact | Users land on broken pages with 404 errors or blank screens if data is loading. |
How to reproduce
Trigger navigation to the embedding model or interactive route while experimentInfo is still loading.AI Fix Prompt
Fix this issue: Navigation uses optional chaining without a fallback. Users are redirected to broken literal undefined URLs. Guard the navigation call with an explicit check.
Location: src/renderer/components/Experiment/Foundation/CurrentFoundationInfo.tsx (lines 376)
Problem: Navigation redirects to the literal string 'undefined' in the URL path.
Current behavior: The browser navigates to an invalid path like /experiment/undefined/embedding-model.
Expected: Navigation should abort or wait until the experiment name is fully loaded.
Steps to reproduce: Trigger navigation to the embedding model or interactive route while experimentInfo is still loading.
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
| // If currently on an experiment page, update the URL to reflect the new experiment | ||
| const match = location.pathname.match(/^\/experiment\/[^/]+\/(.+)$/); | ||
| if (match) { | ||
| navigate(`/experiment/${experimentName}/${match[1]}`); |
There was a problem hiding this comment.
Bug: Experiment names are inserted into URLs without encoding
Experiment names are inserted into URLs without encoding. Names with spaces or special characters create invalid routes. Wrap the parameters with URI encoding.
View Details
Location: src/renderer/components/Experiment/SelectExperimentMenu.tsx (lines 157)
Analysis
Experiment names are inserted into URLs without encoding
| What fails | URLs break when experiment names contain spaces or special characters. |
| Result | The app generates an unencoded URL that fails to match the application's route definitions. |
| Expected | The URL should be properly encoded using encodeURIComponent. |
| Impact | Users cannot navigate to, share, or deep-link experiments with complex names. |
How to reproduce
Create or select an experiment named 'My Beta Test' with a space in it.Patch Details
- navigate(`/experiment/${experimentName}/${match[1]}`);
+ navigate(`/experiment/${encodeURIComponent(experimentName)}/${match[1]}`);AI Fix Prompt
Fix this issue: Experiment names are inserted into URLs without encoding. Names with spaces or special characters create invalid routes. Wrap the parameters with URI encoding.
Location: src/renderer/components/Experiment/SelectExperimentMenu.tsx (lines 157)
Problem: URLs break when experiment names contain spaces or special characters.
Current behavior: The app generates an unencoded URL that fails to match the application's route definitions.
Expected: The URL should be properly encoded using encodeURIComponent.
Steps to reproduce: Create or select an experiment named 'My Beta Test' with a space in it.
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
| // If there's no supports array or it's empty, default to 'chat' | ||
| useEffect(() => { | ||
| if (location.pathname === '/experiment/chat') { | ||
| if (location.pathname.endsWith('/chat')) { |
There was a problem hiding this comment.
Bug: The chat route matching logic is too broad
The chat route matching logic is too broad. Unrelated chat routes will accidentally trigger experiment mode resets. Use a strict regex to match the exact prefix.
View Details
Location: src/renderer/components/MainAppPanel.tsx (lines 151)
Analysis
The chat route matching logic is too broad. Unrelated chat routes will accidentally trigger experime
| What fails | Mode reset logic automatically triggers for any URL ending in /chat. |
| Result | The experiment chat mode is reset unexpectedly. |
| Expected | Mode reset should strictly occur only for valid experiment chat routes. |
| Impact | Causes unintended side effects, UI glitches, and state corruption on unrelated pages. |
How to reproduce
Navigate to a non-experiment route anywhere in the application that ends in /chat.Patch Details
- if (location.pathname.endsWith('/chat')) {
+ if (location.pathname.match(/^\/experiment\/[^/]+\/chat$/)) {AI Fix Prompt
Fix this issue: The chat route matching logic is too broad. Unrelated chat routes will accidentally trigger experiment mode resets. Use a strict regex to match the exact prefix.
Location: src/renderer/components/MainAppPanel.tsx (lines 151)
Problem: Mode reset logic automatically triggers for any URL ending in /chat.
Current behavior: The experiment chat mode is reset unexpectedly.
Expected: Mode reset should strictly occur only for valid experiment chat routes.
Steps to reproduce: Navigate to a non-experiment route anywhere in the application that ends in /chat.
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
|
|
||
| function ExperimentMenuItems({ experimentInfo }: ExperimentMenuItemsProps) { | ||
| const experimentReady = Boolean(experimentInfo?.name); | ||
| const basePath = `/experiment/${experimentInfo?.name || ''}`; |
There was a problem hiding this comment.
Bug: The sidebar path generator uses an empty string fallback
The sidebar path generator uses an empty string fallback. This creates invalid double slash URLs when state is empty. Build paths conditionally to fix this.
View Details
Location: src/renderer/components/Nav/Sidebar.tsx (lines 39)
Analysis
The sidebar path generator uses an empty string fallback
| What fails | The sidebar generates invalid navigation URLs containing double slashes. |
| Result | The generated links point to invalid paths like /experiment//interactive. |
| Expected | Links should be valid or conditionally disabled until the experiment name is ready. |
| Impact | Users clicking links during load states or empty states get routed to broken pages. |
How to reproduce
View the sidebar menu while the experiment state is initializing or missing.Patch Details
- const basePath = `/experiment/${experimentInfo?.name || ''}`;
+ const basePath = experimentInfo?.name ? `/experiment/${encodeURIComponent(experimentInfo.name)}` : '';AI Fix Prompt
Fix this issue: The sidebar path generator uses an empty string fallback. This creates invalid double slash URLs when state is empty. Build paths conditionally to fix this.
Location: src/renderer/components/Nav/Sidebar.tsx (lines 39)
Problem: The sidebar generates invalid navigation URLs containing double slashes.
Current behavior: The generated links point to invalid paths like /experiment//interactive.
Expected: Links should be valid or conditionally disabled until the experiment name is ready.
Steps to reproduce: View the sidebar menu while the experiment state is initializing or missing.
Provide a code fix.
Tip: Reply with @paragon-run to automatically fix this issue
![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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/Experiment/SelectExperimentMenu.tsx`:
- Around line 152-158: The dropdown handler createHandleClose currently passes
an experiment name into setExperimentId (mixing name vs ID) and elsewhere calls
createHandleClose(newId) without invoking the returned function; fix by ensuring
setExperimentId always receives the canonical experiment ID (not name) — convert
the selected name to its ID before calling setExperimentId inside
createHandleClose or change the dropdown to pass the ID — and change callers
that do createHandleClose(newId) to call the returned function (e.g.,
createHandleClose(newId)() or refactor to return void). Update both
createHandleClose and the other occurrence around the later use (near the
reported second location) to consistently use IDs and to invoke the handler so
navigate(`/experiment/${experimentId}/...`) receives the correct identifier.
In `@src/renderer/components/MainAppPanel.tsx`:
- Around line 319-351: Add a child Route for the missing "embedding-model" path
under the same parent route that currently contains ExperimentNotes, Interact,
TrainLoRA, etc. Specifically, in MainAppPanel.tsx add a Route with
path="embedding-model" and element={<EmbeddingModel />} (or the correct
component that renders embedding model details) so that navigation from
CurrentFoundationInfo.tsx (which navigates to
/experiment/${experimentInfo?.name}/embedding-model) resolves; place it
alongside the existing Route entries (e.g., near ExperimentNotes and Interact)
and ensure you import the EmbeddingModel component if not already imported.
- Around line 51-57: The analytics call in MainAppPanel normalizes the path but
still sends a raw URL via window.location.href (leaking experimentName); update
the logic around normalizedPath and the analytics.page call to also produce a
normalizedUrl by replacing the same /^\/experiment\/[^/]+/ segment in
window.location.href (or reconstruct a safe URL using location.origin +
normalizedPath) and pass that normalizedUrl to analytics.page instead of
window.location.href so both path and url are redacted.
In `@src/renderer/components/Nav/Sidebar.tsx`:
- Line 39: The basePath construction in Sidebar.tsx uses raw experimentInfo.name
which can break routing for names with slashes, spaces, or query chars; update
the basePath creation to URL-encode the experiment name (use encodeURIComponent
on experimentInfo.name when present) so the computed basePath (`basePath`) is
safe for use in links and routing (keep the fallback behavior when name is
missing).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/renderer/components/Experiment/Foundation/CurrentFoundationInfo.tsxsrc/renderer/components/Experiment/SelectExperimentMenu.tsxsrc/renderer/components/MainAppPanel.tsxsrc/renderer/components/Nav/Sidebar.tsxsrc/renderer/components/Nav/SubNavItem.tsxsrc/renderer/components/TasksGallery/TasksGallery.tsxsrc/renderer/components/Welcome/Welcome.tsx
Comment on lines
+152
to
+158
| const createHandleClose = (experimentName: string) => () => { | ||
| setExperimentId(experimentName); | ||
| // If currently on an experiment page, update the URL to reflect the new experiment | ||
| const match = location.pathname.match(/^\/experiment\/[^/]+\/(.+)$/); | ||
| if (match) { | ||
| navigate(`/experiment/${experimentName}/${match[1]}`); | ||
| } |
Contributor
There was a problem hiding this comment.
setExperimentId now receives experiment name here, creating inconsistent identifier semantics.
This file still uses IDs in other flows, but dropdown selection now writes name into context. That inconsistency can break experiment switching behavior. Also, Line 212 currently calls createHandleClose(newId) without invoking the returned handler.
Proposed fix
- const createHandleClose = (experimentName: string) => () => {
- setExperimentId(experimentName);
+ const createHandleClose =
+ (experimentId: string | number, experimentName: string) => () => {
+ setExperimentId(String(experimentId));
// If currently on an experiment page, update the URL to reflect the new experiment
const match = location.pathname.match(/^\/experiment\/[^/]+\/(.+)$/);
if (match) {
navigate(`/experiment/${experimentName}/${match[1]}`);
}
- };
+ };
...
- onClick={createHandleClose(experiment.name)}
+ onClick={createHandleClose(experiment.id, experiment.name)}Also applies to: 392-392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/Experiment/SelectExperimentMenu.tsx` around lines 152
- 158, The dropdown handler createHandleClose currently passes an experiment
name into setExperimentId (mixing name vs ID) and elsewhere calls
createHandleClose(newId) without invoking the returned function; fix by ensuring
setExperimentId always receives the canonical experiment ID (not name) — convert
the selected name to its ID before calling setExperimentId inside
createHandleClose or change the dropdown to pass the ID — and change callers
that do createHandleClose(newId) to call the returned function (e.g.,
createHandleClose(newId)() or refactor to return void). Update both
createHandleClose and the other occurrence around the later use (near the
reported second location) to consistently use IDs and to invoke the handler so
navigate(`/experiment/${experimentId}/...`) receives the correct identifier.
Comment on lines
+51
to
57
| const normalizedPath = location.pathname.replace( | ||
| /^\/experiment\/[^/]+/, | ||
| '/experiment/:experimentName', | ||
| ); | ||
| analytics.page({ | ||
| path: location.pathname, | ||
| path: normalizedPath, | ||
| url: window.location.href, |
Contributor
There was a problem hiding this comment.
Redaction is incomplete: analytics still receives the raw experiment name via url.
path is normalized, but url: window.location.href still includes the real experiment name, so tracking still leaks it.
Proposed fix
analytics.page({
path: normalizedPath,
- url: window.location.href,
search: location.search,
title: document.title,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const normalizedPath = location.pathname.replace( | |
| /^\/experiment\/[^/]+/, | |
| '/experiment/:experimentName', | |
| ); | |
| analytics.page({ | |
| path: location.pathname, | |
| path: normalizedPath, | |
| url: window.location.href, | |
| const normalizedPath = location.pathname.replace( | |
| /^\/experiment\/[^/]+/, | |
| '/experiment/:experimentName', | |
| ); | |
| analytics.page({ | |
| path: normalizedPath, | |
| search: location.search, | |
| title: document.title, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/MainAppPanel.tsx` around lines 51 - 57, The analytics
call in MainAppPanel normalizes the path but still sends a raw URL via
window.location.href (leaking experimentName); update the logic around
normalizedPath and the analytics.page call to also produce a normalizedUrl by
replacing the same /^\/experiment\/[^/]+/ segment in window.location.href (or
reconstruct a safe URL using location.origin + normalizedPath) and pass that
normalizedUrl to analytics.page instead of window.location.href so both path and
url are redacted.
Comment on lines
+319
to
+351
| <Route path="notes" element={<ExperimentNotes />} /> | ||
| <Route | ||
| path="chat" | ||
| element={ | ||
| <Interact | ||
| setRagEngine={setRagEngine} | ||
| mode={selectedInteractSubpage} | ||
| setMode={setSelectedInteractSubpage} | ||
| supports={modelSupports} | ||
| chatHistory={chatHistory} | ||
| setChatHistory={setChatHistory} | ||
| /> | ||
| } | ||
| /> | ||
| <Route | ||
| path="model_architecture_visualization" | ||
| element={ | ||
| <Interact | ||
| setRagEngine={setRagEngine} | ||
| mode="model_layers" | ||
| setMode={setSelectedInteractSubpage} | ||
| supports={modelSupports} | ||
| chatHistory={chatHistory} | ||
| setChatHistory={setChatHistory} | ||
| /> | ||
| } | ||
| /> | ||
| <Route path="training" element={<TrainLoRA />} /> | ||
| <Route path="tasks" element={<Tasks />} /> | ||
| <Route path="interactive" element={<Interactive />} /> | ||
| <Route path="documents" element={<Documents />} /> | ||
| <Route path="settings" element={<Settings />} /> | ||
| </Route> |
Contributor
There was a problem hiding this comment.
Missing embedding-model child route creates a dead navigation path.
src/renderer/components/Experiment/Foundation/CurrentFoundationInfo.tsx navigates to /experiment/${experimentInfo?.name}/embedding-model (Line 376), but this route tree does not define an embedding-model child under /experiment/:experimentName.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/MainAppPanel.tsx` around lines 319 - 351, Add a child
Route for the missing "embedding-model" path under the same parent route that
currently contains ExperimentNotes, Interact, TrainLoRA, etc. Specifically, in
MainAppPanel.tsx add a Route with path="embedding-model" and
element={<EmbeddingModel />} (or the correct component that renders embedding
model details) so that navigation from CurrentFoundationInfo.tsx (which
navigates to /experiment/${experimentInfo?.name}/embedding-model) resolves;
place it alongside the existing Route entries (e.g., near ExperimentNotes and
Interact) and ensure you import the EmbeddingModel component if not already
imported.
Member
There was a problem hiding this comment.
This one seems legit? I think we'll get rid of the component though so not sure if you want to fix it? @aliasaria
Contributor
There was a problem hiding this comment.
@deep1401, thanks for confirming! If you're planning to remove the component, there's no need to add the route—just make sure to clean up the navigation reference in CurrentFoundationInfo.tsx when you remove it. That'll keep things consistent. 👍
…s invalid double slash URLs when state is empty. Build paths conditionally to fix this.
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
2 participants
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.
Store the experiment name in the URL like:
http://localhost:1212/#/experiment/beta/interactive
This allows users to share links with others
Summary by CodeRabbit
New Features
Refactor