feat: #72 — feat: Replace Data nav icon with map marker; extract Alerts into dedicated screen#107
feat: #72 — feat: Replace Data nav icon with map marker; extract Alerts into dedicated screen#107luandro wants to merge 1 commit into
Conversation
…tract Alerts into dedicated screen
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| loading: { | ||
| id: 'alerts.loading', | ||
| defaultMessage: 'Loading...', | ||
| }, | ||
| alertsError: { |
There was a problem hiding this comment.
messages.loading definition is never consumed in this component — alertsQuery.isPending renders <Skeleton> nodes directly, not an intl.formatMessage(messages.loading) call. The dead key also propagated unnecessary additions to all three i18n JSON files.
| loading: { | |
| id: 'alerts.loading', | |
| defaultMessage: 'Loading...', | |
| }, | |
| alertsError: { | |
| alertsError: { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/screens/AlertsScreen.tsx
Line: 35-39
Comment:
The `messages.loading` definition is never consumed in this component — `alertsQuery.isPending` renders `<Skeleton>` nodes directly, not an `intl.formatMessage(messages.loading)` call. The dead key also propagated unnecessary additions to all three i18n JSON files.
```suggestion
alertsError: {
```
How can I resolve this? If you propose a fix, please make it concise.| const shellSlot = useMemo( | ||
| () => ({ | ||
| topbarWorkspaceName: selectedProjectId ? topbarWorkspaceName : undefined, | ||
| topbarModeLabel: intl.formatMessage(messages.title), | ||
| }), | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [selectedProjectId, topbarWorkspaceName], | ||
| ); |
There was a problem hiding this comment.
intl is used inside the useMemo callback (intl.formatMessage(messages.title)) but is excluded from the dependency array. If the active locale changes, topbarModeLabel will stay stale until selectedProjectId or topbarWorkspaceName also changes. Adding intl to the array removes the need for the suppression comment and keeps the label reactive to locale switches.
| const shellSlot = useMemo( | |
| () => ({ | |
| topbarWorkspaceName: selectedProjectId ? topbarWorkspaceName : undefined, | |
| topbarModeLabel: intl.formatMessage(messages.title), | |
| }), | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| [selectedProjectId, topbarWorkspaceName], | |
| ); | |
| const shellSlot = useMemo( | |
| () => ({ | |
| topbarWorkspaceName: selectedProjectId ? topbarWorkspaceName : undefined, | |
| topbarModeLabel: intl.formatMessage(messages.title), | |
| }), | |
| [selectedProjectId, topbarWorkspaceName, intl], | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/screens/AlertsScreen.tsx
Line: 61-68
Comment:
`intl` is used inside the `useMemo` callback (`intl.formatMessage(messages.title)`) but is excluded from the dependency array. If the active locale changes, `topbarModeLabel` will stay stale until `selectedProjectId` or `topbarWorkspaceName` also changes. Adding `intl` to the array removes the need for the suppression comment and keeps the label reactive to locale switches.
```suggestion
const shellSlot = useMemo(
() => ({
topbarWorkspaceName: selectedProjectId ? topbarWorkspaceName : undefined,
topbarModeLabel: intl.formatMessage(messages.title),
}),
[selectedProjectId, topbarWorkspaceName, intl],
);
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
Automated implementation of #72.
Closes #72
Implemented by the agent implementation worker.
Greptile Summary
This PR extracts the Alerts tab from
DataScreeninto a dedicated/alertsroute with its ownAlertsScreen, and replaces the Data nav icon with a map-marker SVG. All three i18n locale files are updated and the router/nav are wired up correctly./alertsroute:AlertsScreenmirrorsDataScreen's no-project guard, skeleton loading, error, and empty states, and links alert cards to the existing/data/alerts/$alertIddetail route.DataScreencleanup: Alerts tab,useAlertshook call,activeTabstate, theTabsimport, and all associated messages are removed; the observations flow is unchanged.Confidence Score: 4/5
Safe to merge; the refactor is well-contained and the routing, nav, and i18n wiring are all correct.
The main screen logic, routing, and locale JSON updates are solid. Two small issues in the new AlertsScreen: an unused messages.loading definition that leaked into all three i18n files, and intl missing from a useMemo dependency array (suppressed with an eslint comment). Neither affects current behavior in a meaningful way, but both are worth cleaning up before the next locale-change scenario.
src/screens/AlertsScreen.tsx and the three i18n JSON files (the unused alerts.loading key).
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD NAV[AuthenticatedLayout nav] NAV -->|/| HOME[HomeScreen] NAV -->|/data| DATA[DataScreen\nobservations only] NAV -->|/alerts| ALERTS[AlertsScreen\nnew] NAV -->|/settings| SETTINGS[SettingsScreen] ALERTS -->|Add Alert| CREATE[CreateAlertScreen\n/data/alerts/new] ALERTS -->|card click| DETAIL[AlertDetailScreen\n/data/alerts/:id] DATA -->|obs row| OBSDETAIL[ObservationDetailScreen\n/data/observations/:id]Comments Outside Diff (1)
tests/unit/screens/AlertsScreen.test.tsx, line 1024-1033 (link)<a href={to}>using the rawtotemplate string, sohrefis always'/data/alerts/$alertId'regardless of what is passed inparams. The test does not verify that thealertIdparam is wired toalert.localId. Consider extending the mock to substituteparamsinto the path so the assertion can confirm the resolved href.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat: implement #72 — feat: Replace Data..." | Re-trigger Greptile