Fix: Redirecting to Store Link from Notification #1525
Conversation
WalkthroughThis pull request involves systematically replacing Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/components/notifications-page/messages/nf-str-url-upldfailpay2/index.hbs (1)
13-23: Consider adding title attribute for better accessibility.While the change from
<AkLink>to<AkButton>maintains the core functionality and security attributes, consider adding a title attribute to preserve accessibility information for screen readers.<AkButton @tag='a' @variant='text' @color='primary' @underline='always' + @title={{t 'notificationModule.viewAppOnStore'}} target='_blank' href={{@context.store_url}} rel='noopener noreferrer' >app/components/notifications-page/messages/nf-str-url-vldtn-err/index.hbs (1)
28-38: Maintain consistent accessibility attributes.The implementation is consistent with other files, but consider adding the title attribute here as well for accessibility.
<AkButton @tag='a' @variant='text' @color='primary' @underline='always' + @title={{t 'notificationModule.viewAppOnStore'}} target='_blank' href={{@context.store_url}} rel='noopener noreferrer' >app/components/notifications-page/messages/nf-str-url-upldfailnprjdeny2/index.hbs (1)
41-51: Add missing @color attribute for consistency.The
@color='primary'attribute is present in other similar button components but missing here. Consider adding it for consistency.<AkButton data-test-nf-str-url-upldfailnprjdeny2-viewAppOnStorelink @tag='a' @variant='text' + @color='primary' @underline='always' target='_blank' href={{@context.store_url}} rel='noopener noreferrer' >app/components/notifications-page/messages/nf-str-url-upldfailnsunaprv1/index.hbs (1)
11-21: Consider extracting the repeated button pattern into a shared component.This button pattern is repeated across multiple notification templates. Consider creating a shared component to improve maintainability:
Usage:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/components/notifications-page/messages/nf-str-url-upldfailnprjdeny1/index.hbs(1 hunks)app/components/notifications-page/messages/nf-str-url-upldfailnprjdeny2/index.hbs(1 hunks)app/components/notifications-page/messages/nf-str-url-upldfailnscreatd1/index.hbs(1 hunks)app/components/notifications-page/messages/nf-str-url-upldfailnsunaprv1/index.hbs(1 hunks)app/components/notifications-page/messages/nf-str-url-upldfailpay2/index.hbs(1 hunks)app/components/notifications-page/messages/nf-str-url-upldfailpayrq1/index.hbs(1 hunks)app/components/notifications-page/messages/nf-str-url-upload-success/index.hbs(1 hunks)app/components/notifications-page/messages/nf-str-url-vldtn-err/index.hbs(1 hunks)app/components/notifications-page/namespace-message/index.hbs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Run E2E Tests (5)
- GitHub Check: Run E2E Tests (4)
- GitHub Check: Run E2E Tests (3)
- GitHub Check: Run E2E Tests (2)
- GitHub Check: Run E2E Tests (1)
- GitHub Check: Setup & Build Application
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
app/components/notifications-page/messages/nf-str-url-upload-success/index.hbs (1)
38-48: Verify the selective component replacement pattern.I notice that internal route links still use
<AkLink>while external store links are changed to<AkButton>. This seems intentional but should be verified.✅ Verification successful
The component replacement pattern is consistent with codebase conventions
The codebase consistently uses
<AkLink>for internal routing and<AkButton @tag='a'>for external links. The change in this file aligns with this established pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for remaining AkLink components and their usage patterns echo "Analyzing AkLink usage patterns..." rg --type hbs '<AkLink' -B 2 -A 5 # Search for new AkButton patterns echo -e "\nAnalyzing new AkButton patterns..." rg --type hbs '<AkButton.*@tag=.a.' -B 2 -A 5Length of output: 41549
app/components/notifications-page/namespace-message/index.hbs (2)
59-70: Consider accessibility implications of removing the title attribute.While the change from
<AkLink>to<AkButton>maintains functionality, the removal of the@titleattribute might affect users relying on tooltips for additional context.Let's verify if this is a consistent pattern across the application:
59-70: Verify consistent button styling across notification templates.The change to
<AkButton @tag='a' @variant='text'>appears correct. Let's ensure this pattern is consistently applied across all notification templates.✅ Verification successful
Button styling is consistently applied across notification templates
All link-style buttons in notification messages consistently use
@tag='a'and@variant='text'. Other button variations (approve/reject actions, header actions) correctly use different styling appropriate for their purposes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent usage of AkButton with @tag='a' and @variant='text' echo "Searching for AkButton usage patterns in notification templates..." rg -U "<AkButton[^>]*@tag='a'[^>]*@variant='text'" app/components/notifications-page/Length of output: 3082
Script:
#!/bin/bash # Check for any AkButton usages in notification templates that might have different styling echo "Searching for all AkButton usages in notification templates..." rg -U "<AkButton" app/components/notifications-page/ -A 5Length of output: 7264
app/components/notifications-page/messages/nf-str-url-upldfailpayrq1/index.hbs (1)
11-21: LGTM! Secure implementation of external link.The button implementation maintains proper security attributes for external links (
target='_blank'withrel='noopener noreferrer').Consider if removing the
@titleattribute impacts accessibility for screen readers. If the link text is self-descriptive, this should be fine.app/components/notifications-page/messages/nf-str-url-upldfailnscreatd1/index.hbs (1)
11-21: LGTM! Consistent implementation.The changes follow the same secure pattern as other files in the PR.
app/components/notifications-page/messages/nf-str-url-upldfailnprjdeny1/index.hbs (1)
12-22: Verify the impact of these changes across the application.The implementation is consistent and secure. Let's verify:
- The button styling matches the design system
- The changes work correctly with the router
Run the following script to check for any custom styling or router handling that might be affected:
✅ Verification successful
Changes are consistent with other store URL implementations
The button implementation:
- Follows the same pattern used across all notification components
- Maintains proper security attributes for external navigation
- Uses the design system component as intended with no custom styling conflicts
- Has no impact on router handling as it's for external navigation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for custom styles or router handling for AkLink rg -g '*.{scss,css}' 'AkLink' rg -g '*.{js,ts}' 'AkLink.*click|router.*transition'Length of output: 9202
Script:
#!/bin/bash # Search for store URL button patterns rg -g '*.{hbs,js,ts}' 'store_url.*href|href.*store_url'Length of output: 1068
Deploying irenestaging with
|
| Latest commit: |
021d32f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2a3902d1.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1669-fix-notification-sto.irenestaging.pages.dev |
Irene
|
||||||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1669-Fix-Notification-Store-Link
|
| Run status |
|
| Run duration | 10m 19s |
| Commit |
|
| Committer | SmitGala |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
31
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/tests/dynamic-scan.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Dynamic Scan > it tests dynamic scan for an apk file: 58062 |
Test Replay
Screenshots
|
|
future-pirate-king
force-pushed
the
PD-1669-Fix-Notification-Store-Link
branch
from
c670e17 to
021d32f
Compare
|
No description provided.