Floating green bars #2952
Floating green bars #2952
Conversation
|
Warning Rate limit exceeded@Arpan-206 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughA floating green bar has been added to the desktop header navigation to visually highlight the active route. This involves new logic and properties in the header component for positioning and animating the bar, dynamic class assignment to navigation links based on route activity, and route awareness in link components. No changes were made to exported or public entity declarations beyond new tracked properties and actions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RouterService
participant HeaderComponent
participant LinkComponent
User->>RouterService: Navigates to a new route
RouterService-->>HeaderComponent: Notify route change
HeaderComponent->>HeaderComponent: handleRouteChange()
HeaderComponent->>HeaderComponent: findAndPositionActiveLink()
HeaderComponent->>LinkComponent: Query active link element (via data-route)
LinkComponent->>RouterService: Check if link is active
RouterService-->>LinkComponent: Return route match
LinkComponent-->>HeaderComponent: Return active link element
HeaderComponent->>HeaderComponent: calculateFloatingBarPosition()
HeaderComponent-->>User: Animate floating bar to active link position
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/components/header/index.ts (2)
123-137: Critical: Fix null pointer error and formatting issuesThis method has the exact null pointer issue identified in the previous review, plus multiple formatting problems.
The method attempts to call
getBoundingClientRect()onactiveLinkwithout null checking, and has several formatting issues:private positionFloatingBar(activeLink: HTMLElement, container: HTMLElement) { + if (!activeLink) { + this.floatingBarStyle = 'opacity: 0;'; + return; + } + const containerRect = container.getBoundingClientRect(); const linkRect = activeLink.getBoundingClientRect(); - + const left = linkRect.left - containerRect.left; const width = linkRect.width; const bottom = -18; - + this.floatingBarStyle = ` left: ${left}px; width: ${width}px; bottom: ${bottom}px; opacity: 1; `; }Additionally, move this method before
updateFloatingBarPositionto fix member ordering.
116-121: Fix formatting and improve error handlingThe method has formatting issues and needs better error handling for when no active link is found.
private updateFloatingBarPosition(containerElement: HTMLElement) { const currentRoute = this.router.currentRouteName; const activeLink = containerElement.querySelector(`[data-route="${currentRoute}"]`) as HTMLElement; - + + if (!activeLink) { + this.floatingBarStyle = 'opacity: 0;'; + return; + } + this.positionFloatingBar(activeLink, containerElement); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/components/header/index.hbs(1 hunks)app/components/header/index.ts(3 hunks)app/components/header/link.hbs(1 hunks)app/components/header/link.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/components/**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/formatting-component-files.mdc)
app/components/**/*.ts: Use TypeScript (.ts) files for components in app/components
Import from@glimmer/componentwhen creating components
Useinterface Signature { ... }to define the component's signature
classdoesn't have to be inArgs, Ember automatically forwards native args likeclass,idetc. to the component
Ensure the component is registered with Glint usingdeclare module ...
Files:
app/components/header/link.tsapp/components/header/index.ts
**/*.{js,ts,hbs,gts,gjs}
📄 CodeRabbit Inference Engine (.rules/no-bugs.md)
**/*.{js,ts,hbs,gts,gjs}: Avoid typos
Don't have really obvious bugs
Follow reasonable conventions of the language we're programming in
Files:
app/components/header/link.tsapp/components/header/link.hbsapp/components/header/index.hbsapp/components/header/index.ts
🧠 Learnings (2)
app/components/header/link.ts (4)
Learnt from: CR
PR: codecrafters-io/frontend#0
File: .cursor/rules/formatting-component-files.mdc:0-0
Timestamp: 2025-07-25T04:24:47.305Z
Learning: Applies to app/components/**/*.ts : Use interface Signature { ... } to define the component's signature
Learnt from: CR
PR: codecrafters-io/frontend#0
File: .cursor/rules/formatting-component-files.mdc:0-0
Timestamp: 2025-07-25T04:24:47.305Z
Learning: Applies to app/components/**/*.ts : Import from @glimmer/component when creating components
Learnt from: CR
PR: codecrafters-io/frontend#0
File: .cursor/rules/formatting-component-files.mdc:0-0
Timestamp: 2025-07-25T04:24:47.305Z
Learning: Applies to app/components/**/*.ts : class doesn't have to be in Args, Ember automatically forwards native args like class, id etc. to the component
Learnt from: CR
PR: codecrafters-io/frontend#0
File: .cursor/rules/formatting-component-files.mdc:0-0
Timestamp: 2025-07-25T04:24:47.305Z
Learning: Applies to app/components/**/*.ts : Ensure the component is registered with Glint using declare module ...
app/components/header/index.ts (1)
Learnt from: CR
PR: codecrafters-io/frontend#0
File: .rules/no-bugs.md:0-0
Timestamp: 2025-07-25T04:24:57.955Z
Learning: Applies to **/*.{js,ts,hbs,gts,gjs} : Don't have really obvious bugs
🪛 GitHub Actions: Test
app/components/header/link.ts
[error] 22-22: Prettier formatting error: Delete extra spaces (prettier/prettier).
[warning] Prettier formatting warning: Code style issues found. Run Prettier with --write to fix.
app/components/header/index.hbs
[error] 49-49: ember-template-lint: elements cannot have inline styles (no-inline-styles) and Concatenated styles must be marked as htmlSafe (style-concatenation).
[error] 49-49: ember-template-lint: Concatenated styles must be marked as htmlSafe (style-concatenation).
[warning] Prettier formatting warning: Code style issues found. Run Prettier with --write to fix.
app/components/header/index.ts
[error] 90-90: ESLint: Expected blank line before this statement (padding-line-between-statements).
[error] 106-106: ESLint: Member setupFloatingBar should be declared before member toggleMobileMenu (@typescript-eslint/member-ordering).
[error] 119-119: Prettier formatting error: Delete extra spaces (prettier/prettier).
[error] 123-123: ESLint: Member positionFloatingBar should be declared before member updateFloatingBarPosition (@typescript-eslint/member-ordering).
[error] 126-126: Prettier formatting error: Delete extra spaces (prettier/prettier).
[error] 130-130: Prettier formatting error: Delete extra spaces (prettier/prettier).
[warning] Prettier formatting warning: Code style issues found. Run Prettier with --write to fix.
🔇 Additional comments (9)
app/components/header/link.ts (2)
2-3: LGTM: Proper service importsThe service injection import and RouterService type import follow the expected patterns for Ember components.
16-16: LGTM: Correct service declarationThe router service is properly injected using the documented pattern with proper typing.
app/components/header/link.hbs (2)
4-6: LGTM: Dynamic class assignment based on active stateThe conditional class assignment correctly applies darker text colors for active links and lighter colors for inactive ones, while preserving hover states. The
relativepositioning class is appropriate for the floating bar feature.
8-8: LGTM: Added data-route attribute for DOM queryingThe
data-routeattribute enables the floating bar positioning logic to identify the active link element, which is essential for the feature implementation.app/components/header/index.hbs (2)
41-41: LGTM: Proper lifecycle modifier usageThe container wrapper with
did-insertanddid-updatemodifiers correctly triggers floating bar setup and updates on route changes.
42-44: LGTM: Navigation links structure preservedThe existing navigation link iteration remains unchanged, maintaining the component's functionality while adding the necessary container for floating bar positioning.
app/components/header/index.ts (3)
28-28: LGTM: Proper tracked property initializationThe
floatingBarStyletracked property is correctly initialized with hidden state (opacity: 0).
111-114: LGTM: Floating bar update actionThe update action correctly delegates to the position update method.
88-94: Fix formatting and add error handlingThe route change handling logic is sound but has formatting issues and needs error handling.
@action handleRouteChange() { this.mobileMenuIsExpanded = false; + setTimeout(() => { const container = document.querySelector('[data-test-header] .relative') as HTMLElement; if (container) { this.updateFloatingBarPosition(container); } }, 10); }⛔ Skipped due to learnings
Learnt from: CR PR: codecrafters-io/frontend#0 File: .rules/no-bugs.md:0-0 Timestamp: 2025-07-25T04:24:57.955Z Learning: Applies to **/*.{js,ts,hbs,gts,gjs} : Don't have really obvious bugs
Arpan-206
force-pushed
the
floating-green-bars
branch
from
c379280 to
47a67e2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Bundle ReportChanges will increase total bundle size by 2.38kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-array-pushAssets Changed:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Arpan-206
force-pushed
the
floating-green-bars
branch
from
2cb34cd to
bc09447
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
app/components/header/index.hbs
Outdated
| <div | ||
| class="absolute -bottom-[18px] h-0.5 bg-teal-500 transition-all duration-200 ease-out" | ||
| style={{this.floatingBarStyle}} | ||
| data-test-floating-bar |
There was a problem hiding this comment.
If we aren't using data-... selectors, remove them
app/components/header/index.ts
Outdated
| } | ||
| }); | ||
| } | ||
|
|
||
| @action | ||
| handleRouteChange() { |
There was a problem hiding this comment.
It could be confusing as to why we have two methods for detecting route changes. Let's make this more clear:
- Rename
handleRouteChange->handleRouteWillChange(this makes it clear that it fires before a transition( - Don't do the floating bar update here, it isn't needed and will be triggered in
DidUpdateCurrentRouteNameanyway.
Closing the mobile menu is something we want to do early (before a page starts loading). Moving the highlight bar though is something that can only be done after a loading state so doesn't need to be done here.
app/components/header/link.hbs
Outdated
| data-test-header-link | ||
| data-route={{@route}} |
There was a problem hiding this comment.
@Arpan-206 arpan let's pay more attention to detail + re-review properly when refactoring - this data-route=... is not needed anymore
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| if (!currentRoute || currentRoute.includes('loading')) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: Floating Bar Visibility Issue During Navigation
The updateFloatingBarPosition method's early return condition currentRoute.includes('loading') is too broad. This prevents the floating bar from appearing on initial page load and causes it to remain visible under a previously active header link instead of hiding when navigating to routes that should not have an active header link (e.g., course pages) if an intermediate loading state is encountered.
Locations (1)
There was a problem hiding this comment.
Feels too janky w/o this.
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit
New Features
Style
Bug Fixes