fix: improve breadcrumbs#4142
Open
Mohamed-Hacene wants to merge 8 commits into
Open
Conversation
Mohamed-Hacene
requested review from
ab-smith,
eric-intuitem,
monsieurswag and
nas-tabchiche
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR refactors breadcrumb synchronization from trimming-based logic to URL-driven recomputation that preserves query strings and updates only when breadcrumbs change. ModelTable updates history before syncing the last crumb. One edit page stops mutating breadcrumbs and resets a loading flag. Some server loads now return a localized title, and two TreeView anchors receive label props. ChangesBreadcrumb URL Synchronization
Server load: add title to returns
Tree view anchor labels
Sequence DiagramsequenceDiagram
participant User as User Navigation
participant Page as Page (afterNavigate)
participant ModelTable as ModelTable.svelte
participant Breadcrumbs as Breadcrumbs.svelte
participant Store as $breadcrumbs
User->>Page: navigate / URL change
Page->>Breadcrumbs: afterNavigate -> sync(getPageTitle())
ModelTable->>ModelTable: history.replaceState(pathname+search)
ModelTable->>Breadcrumbs: breadcrumbs.update(...) to adjust last crumb href
Breadcrumbs->>Breadcrumbs: compute normalized pathname, match or append fallback
Breadcrumbs->>Store: update only if length or href/label changed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/lib/components/Breadcrumbs/Breadcrumbs.svelte (2)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix Prettier formatting issues before merge.
The pipeline reports Prettier formatting errors. Run
pnpm exec prettier --write .to fix formatting issues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/lib/components/Breadcrumbs/Breadcrumbs.svelte` at line 1, Run Prettier to fix formatting errors reported by the pipeline for the Breadcrumbs component: execute `pnpm exec prettier --write .`, reformat the file that contains the <script lang="ts"> block in Breadcrumbs.svelte (component Breadcrumbs) and commit the changes so the CI passes.
101-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove dead code: unused return value.
The
onclickhandler callsbreadcrumbs.slice(i)but doesn't use or assign the returned value, making this call ineffective. If the intention was to trim the breadcrumbs, it should bebreadcrumbs.set(breadcrumbs.get().slice(0, i + 1))or similar.🐛 Proposed fix or removal
If the intent is to trim breadcrumbs when clicking a crumb, use:
-onclick={() => breadcrumbs.slice(i)} +onclick={() => breadcrumbs.set($breadcrumbs.slice(0, i + 1))}Otherwise, if this is no longer needed with the new sync logic, remove it:
-onclick={() => breadcrumbs.slice(i)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/lib/components/Breadcrumbs/Breadcrumbs.svelte` at line 101, The onclick handler in Breadcrumbs.svelte currently calls breadcrumbs.slice(i) and discards the return value; either remove this dead call or update it to actually trim the store value—e.g., replace the no-op with a store update like breadcrumbs.set(breadcrumbs.get().slice(0, i + 1)) (or the equivalent API your store uses) so clicking a crumb reduces the breadcrumbs, and ensure you reference the local variables used in the template (breadcrumbs and index i) when making the change.
🧹 Nitpick comments (2)
frontend/src/lib/components/ModelTable/ModelTable.svelte (1)
356-368: ⚡ Quick winReuse the
hrefPathnamehelper to avoid logic duplication.Line 361 duplicates the pathname extraction logic from
hrefPathname()in Breadcrumbs.svelte (line 9-13). Consider importing and reusing the helper for consistency and to avoid divergence if the logic needs to evolve (e.g., handling fragment identifiers).♻️ Proposed refactor to reuse helper
First, export
hrefPathnamefrom Breadcrumbs.svelte or move it to a shared utility module. Then:+import { hrefPathname } from '$lib/components/Breadcrumbs/Breadcrumbs.svelte'; +// or: import { hrefPathname } from '$lib/utils/breadcrumbs'; breadcrumbs.update((crumbs) => { if (crumbs.length < 2) return crumbs; const last = crumbs[crumbs.length - 1]; - const lastPath = last.href?.split('?')[0]; + const lastPath = hrefPathname(last.href); if (lastPath !== page.url.pathname) return crumbs;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/lib/components/ModelTable/ModelTable.svelte` around lines 356 - 368, The breadcrumb update block duplicates pathname extraction; import and use the shared hrefPathname helper (export it from Breadcrumbs.svelte or move it to a shared util) and replace the inline split logic in the breadcrumbs.update callback with hrefPathname(last.href), keeping the rest of the logic unchanged and ensuring hrefPathname handles query and fragment normalization the same way as Breadcrumbs.svelte.frontend/src/lib/components/Breadcrumbs/Breadcrumbs.svelte (1)
9-13: ⚡ Quick winConsider using URL parsing for robustness.
The current string-based approach using
indexOfandsliceworks for typical cases but may fail on edge cases like URLs with fragment identifiers (#anchor) or malformed hrefs. Using the URL constructor would handle these cases more reliably.♻️ Proposed refactor using URL parsing
function hrefPathname(href: string | undefined): string | undefined { if (!href) return undefined; - const queryIdx = href.indexOf('?'); - return queryIdx === -1 ? href : href.slice(0, queryIdx); + try { + // Handle relative URLs by using a dummy base + const url = new URL(href, 'http://dummy'); + return url.pathname; + } catch { + // Fallback for malformed URLs + const queryIdx = href.indexOf('?'); + const hashIdx = href.indexOf('#'); + const endIdx = Math.min( + queryIdx === -1 ? Infinity : queryIdx, + hashIdx === -1 ? Infinity : hashIdx + ); + return endIdx === Infinity ? href : href.slice(0, endIdx); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/lib/components/Breadcrumbs/Breadcrumbs.svelte` around lines 9 - 13, Replace the fragile indexOf/slice logic in hrefPathname with the URL parser: in hrefPathname(href) wrap new URL(href, document.baseURI) in a try/catch to support absolute and relative hrefs and malformed input, then return url.pathname (omit url.search and url.hash) so you reliably strip query and fragment; if URL construction throws, return undefined to preserve current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/lib/components/Breadcrumbs/Breadcrumbs.svelte`:
- Around line 74-77: The effect uses getPageTitle() (which can call
getBreadcrumbTitle() and read $breadcrumbs) and then calls sync(), creating a
circular read/write between $pageTitle and $breadcrumbs; break this by
decoupling the responsibilities: either change getPageTitle() so it never reads
$breadcrumbs/getBreadcrumbTitle() (compute breadcrumb-derived fallback
elsewhere), or change sync() so it never mutates $breadcrumbs based on the
pageTitle/fallbackLabel; implement this by moving breadcrumb-fallback
computation into a derived/standalone function or store and updating only
pageTitle in the $effect that calls getPageTitle(), while running sync() from a
separate effect that listens solely to $breadcrumbs (or vice versa), referencing
the existing symbols $effect, getPageTitle, sync, $pageTitle,
getBreadcrumbTitle, $breadcrumbs, and fallbackLabel to locate the appropriate
logic to refactor.
---
Outside diff comments:
In `@frontend/src/lib/components/Breadcrumbs/Breadcrumbs.svelte`:
- Line 1: Run Prettier to fix formatting errors reported by the pipeline for the
Breadcrumbs component: execute `pnpm exec prettier --write .`, reformat the file
that contains the <script lang="ts"> block in Breadcrumbs.svelte (component
Breadcrumbs) and commit the changes so the CI passes.
- Line 101: The onclick handler in Breadcrumbs.svelte currently calls
breadcrumbs.slice(i) and discards the return value; either remove this dead call
or update it to actually trim the store value—e.g., replace the no-op with a
store update like breadcrumbs.set(breadcrumbs.get().slice(0, i + 1)) (or the
equivalent API your store uses) so clicking a crumb reduces the breadcrumbs, and
ensure you reference the local variables used in the template (breadcrumbs and
index i) when making the change.
---
Nitpick comments:
In `@frontend/src/lib/components/Breadcrumbs/Breadcrumbs.svelte`:
- Around line 9-13: Replace the fragile indexOf/slice logic in hrefPathname with
the URL parser: in hrefPathname(href) wrap new URL(href, document.baseURI) in a
try/catch to support absolute and relative hrefs and malformed input, then
return url.pathname (omit url.search and url.hash) so you reliably strip query
and fragment; if URL construction throws, return undefined to preserve current
behavior.
In `@frontend/src/lib/components/ModelTable/ModelTable.svelte`:
- Around line 356-368: The breadcrumb update block duplicates pathname
extraction; import and use the shared hrefPathname helper (export it from
Breadcrumbs.svelte or move it to a shared util) and replace the inline split
logic in the breadcrumbs.update callback with hrefPathname(last.href), keeping
the rest of the logic unchanged and ensuring hrefPathname handles query and
fragment normalization the same way as Breadcrumbs.svelte.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fc3aff6-d4c6-49bb-aa16-2b8a81d543fb
📒 Files selected for processing (2)
frontend/src/lib/components/Breadcrumbs/Breadcrumbs.sveltefrontend/src/lib/components/ModelTable/ModelTable.svelte
Mohamed-Hacene
removed request for
ab-smith,
eric-intuitem,
monsieurswag and
nas-tabchiche
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/lib/components/Breadcrumbs/Breadcrumbs.svelte (1)
9-13: ⚡ Quick winImprove
hrefPathnameto handle hash fragments for defensive robustness.The function currently extracts the pathname by splitting on
?but ignores hash fragments (#). While the codebase doesn't currently use hash fragments in breadcrumb hrefs, the function would incorrectly include them if such hrefs were added in the future. The proposed fix is simple and improves robustness:function hrefPathname(href: string | undefined): string | undefined { if (!href) return undefined; - const queryIdx = href.indexOf('?'); - return queryIdx === -1 ? href : href.slice(0, queryIdx); + return href.split(/[?#]/)[0]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/lib/components/Breadcrumbs/Breadcrumbs.svelte` around lines 9 - 13, The hrefPathname function currently only strips query strings and will leave hash fragments in the returned pathname; update hrefPathname to also detect '#' and treat it like '?' by finding the earliest index of either '?' or '#', then slice href up to that index (or return href if neither exists). Keep the same signature and behavior when href is undefined, and reference the hrefPathname function when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/lib/components/Breadcrumbs/Breadcrumbs.svelte`:
- Around line 9-13: The hrefPathname function currently only strips query
strings and will leave hash fragments in the returned pathname; update
hrefPathname to also detect '#' and treat it like '?' by finding the earliest
index of either '?' or '#', then slice href up to that index (or return href if
neither exists). Keep the same signature and behavior when href is undefined,
and reference the hrefPathname function when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8fae84b-be29-4da9-a661-be0109d2bb79
📒 Files selected for processing (4)
frontend/src/lib/components/Breadcrumbs/Breadcrumbs.sveltefrontend/src/routes/(app)/(internal)/ebios-rm/[id=uuid]/workshop-1/ebios-rm-study/edit/+page.server.tsfrontend/src/routes/(app)/(third-party)/[model=thirdparty_urlmodels]/[id=uuid]/edit/+layout.server.tsfrontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/TreeViewItemContent.svelte
✅ Files skipped from review due to trivial changes (1)
- frontend/src/routes/(app)/(third-party)/[model=thirdparty_urlmodels]/[id=uuid]/edit/+layout.server.ts
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.
Breadcrumbs now keep query params (filters, search) so clicking back returns to the filtered list. Pages reached directly are also appended instead of being dropped from the trail.
title: m.edit()on edit pages that were missing it (audits, evidences, ebios-rm study, generic third-party edits)Summary by CodeRabbit
Bug Fixes
UI Improvements
New Features