fix(ui): several improvement part 3#2617
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## new-navigation #2617 +/- ##
=================================================
Coverage ? 43.87%
=================================================
Files ? 634
Lines ? 15247
Branches ? 4469
=================================================
Hits ? 6689
Misses ? 7411
Partials ? 1147
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
RemiBonnet
changed the title
…nality with navigation redirect
…tion in production environment
…rmatSource function and clean up state management
…grate it into ServiceOverview for job service status notifications
RemiBonnet
force-pushed
the
fix/new-nav/ui-feedback-3
branch
from
2ed7183 to
b10c4ed
Compare
|
@RemiBonnet Quick feedback, but can you put the callout with clear status just above the instances table (and below the "instances" title?). Would be more logical since it's written "Look at the table below to..." in the callout |
| const [router] = useState(() => { | ||
| const nextRouter = createRouter({ | ||
| routeTree, | ||
| context: { auth, queryClient }, | ||
| defaultNotFoundComponent: NotFoundPage, | ||
| }) | ||
|
|
||
| if (!isSentryInitialized && NODE_ENV === 'production') { | ||
| Sentry.init({ | ||
| release: GIT_SHA, | ||
| dsn: SENTRY_DSN, | ||
| integrations: [Sentry.tanstackRouterBrowserTracingIntegration(nextRouter), Sentry.replayIntegration()], | ||
| tracesSampleRate: 1.0, | ||
| replaysSessionSampleRate: 0.1, | ||
| replaysOnErrorSampleRate: 1.0, | ||
| }) | ||
|
|
||
| isSentryInitialized = true | ||
| } | ||
|
|
||
| return nextRouter | ||
| }) |
There was a problem hiding this comment.
Why do we need a local state here?
There was a problem hiding this comment.
The goal was just to keep a stable router instance across renders, not really to use state. I switched it to useRef since it’s clearer, especially because Sentry is attached to that router instance !
| useTerraformVariablesContext() | ||
| const { environmentId = '' } = useParams({ strict: false }) | ||
| const [isVariablePopoverOpen, setIsVariablePopoverOpen] = useState(false) | ||
| const [, setIsVariablePopoverOpen] = useState(false) |
There was a problem hiding this comment.
I think we can remove this local state altogether
…n and update Sentry integration
… popover management
|
View your CI Pipeline Execution ↗ for commit e80205a ☁️ Nx Cloud last updated this comment at |
1 similar comment
|
View your CI Pipeline Execution ↗ for commit e80205a ☁️ Nx Cloud last updated this comment at |
…e appropriate section for job services
3 participants
Summary
Screenshots / Recordings
Testing
yarn testoryarn test -u(if you need to regenerate snapshots)yarn formatyarn lintPR Checklist
.cursor/rules)feat(service): add new Terraform service) - required for semantic-release