Skip to content

Rework app navigation and shared page shells - #13

Merged
Berget1411 merged 2 commits into
mainfrom
dev
Apr 12, 2026
Merged

Rework app navigation and shared page shells#13
Berget1411 merged 2 commits into
mainfrom
dev

Conversation

@Berget1411

Copy link
Copy Markdown
Owner
  • Add shared page, surface, and row action components
  • Restructure app navigation into track, review, and manage sections
  • Refresh client, tracker, reports, and home layouts

- Add shared page, surface, and row action components
- Restructure app navigation into track, review, and manage sections
- Refresh client, tracker, reports, and home layouts
@greptile-apps

greptile-apps Bot commented Apr 12, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR restructures the app navigation into Track / Review / Manage sections, moves several routes under /app/manage/* with legacy redirects, introduces shared AppPage / AppSurface shell components and a unified RowActions component, and refreshes the layouts of the tracker, reports, clients, projects, tags, and home pages.

  • settings-dialog.tsx: When authClient.listAccounts() returns an error, canChangePassword is set to true, incorrectly showing the password-change form to OAuth-only users who can't actually use it.
  • email.ts: inviteLink is interpolated into the HTML email body without HTML-escaping, while inviterName and orgName are properly escaped — this inconsistency creates a potential injection point.

Confidence Score: 4/5

Safe to merge after addressing the two P1 issues — the canChangePassword error-fallback and the unescaped inviteLink in the email template.

Two P1 issues exist: the OAuth-user password-form regression in settings-dialog and the raw inviteLink interpolation in the HTML email. All other findings are P2 (stale selection counter, ephemeral star state, floating promise). The navigation restructuring and new shell components are well-executed.

packages/auth/src/lib/email.ts and apps/web/src/features/auth/components/settings-dialog.tsx

Security Review

  • XSS via unescaped inviteLink in HTML email (packages/auth/src/lib/email.ts): inviterName and orgName are HTML-escaped before injection, but inviteLink is interpolated raw. A tainted link containing " or > could break out of the href attribute and inject arbitrary HTML/script content into the email. Escaping the link with the existing escapeHtml helper would close this gap.

Important Files Changed

Filename Overview
packages/auth/src/lib/email.ts Invitation email sender; inviteLink is interpolated raw into HTML while other user fields are escaped — potential XSS injection point in outbound email
apps/web/src/features/auth/components/settings-dialog.tsx Profile/password/theme settings dialog; error fallback in account-type check defaults to showing password form, which is wrong for OAuth-only users
apps/web/src/hooks/use-table-selection.ts Generic table selection hook; selectedIds is never cleared when items list changes, causing stale selection counts across filter changes
apps/web/src/features/projects/pages/projects-page.tsx Projects management page; star state in ProjectTableRow is local-only with no persistence or mutation, resets on every remount
apps/web/src/components/app-page-shell.tsx New shared page/surface shell components (AppPage, AppPageHeader, AppSurface, etc.) — clean composition, correctly follows design tokens
apps/web/src/features/navigation/app-navigation.ts Centralized nav config with track/review/manage sections, route matching utilities, and getSidebarSections — clean and well-structured
apps/web/src/features/navigation/components/app-layout-shell.tsx App layout shell with sidebar provider, breadcrumb header derived from route meta, and loading skeleton — correct implementation
apps/web/src/features/clients/pages/clients-page.tsx Client management page with filtering, search, and inline editing — uses new shell components correctly
apps/web/src/components/row-actions.tsx Shared archive/delete row actions component; AlertDialog+DropdownMenu nesting is correct, but delete button lacks a pending/loading state to prevent double-submit
apps/web/src/features/time-tracker/pages/reports-page.tsx Reports page with range filtering, export, and chart layout — well-structured with useReducer for filter state

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["/app"] --> B["/_app layout\n(AppLayoutShell)"]
    B --> C["/ → Time Tracker"]
    B --> D["/overview → Dashboard"]
    B --> E["/reports → Reports"]
    B --> F["/calendar → Calendar"]
    B --> G["/tasks → Tasks"]
    B --> H["/manage"]
    H --> I["/manage/projects"]
    H --> J["/manage/clients"]
    H --> K["/manage/teams"]
    H --> L["/manage/tags"]
    subgraph Redirects
        R1["/tracker"] -->|redirect| C
        R2["/dashboard"] -->|redirect| D
        R3["/projects"] -->|redirect| I
        R4["/clients"] -->|redirect| J
        R5["/teams"] -->|redirect| K
        R6["/tags"] -->|redirect| L
    end
Loading

Comments Outside Diff (3)

  1. packages/auth/src/lib/email.ts, line 47-49 (link)

    P1 security inviteLink not HTML-escaped in email template

    inviterName and orgName are run through escapeHtml, but inviteLink is interpolated raw. If inviteLink ever contains a double-quote, an attacker could break out of the href attribute and inject arbitrary HTML into the email. Wrap inviteLink with the existing escapeHtml helper for consistency:

    <a href="${escapeHtml(inviteLink)}"

    Fix in Codex Fix in Claude Code Fix in Cursor

  2. apps/web/src/features/projects/pages/projects-page.tsx, line 336-337 (link)

    P2 Star state is ephemeral and not persisted

    starred is local component state with no backing mutation. It resets whenever the row re-mounts (filter change, sort, etc.), so the star icon visually toggles but never persists. Either remove the star button until the feature is implemented, or disable it with a placeholder to make the intent clear.

    Fix in Codex Fix in Claude Code Fix in Cursor

  3. apps/web/src/features/navigation/components/team-switcher.tsx, line 179 (link)

    P2 Floating promise in onKeyDown handler

    handleCreateOrg is async but the returned Promise is not handled in the keyboard event handler. While the function has an internal try/catch, add void to make the intent explicit and avoid a no-floating-promises lint warning: void handleCreateOrg().

    Fix in Codex Fix in Claude Code Fix in Cursor

Fix All in Codex Fix All in Claude Code Fix All in Cursor

Reviews (1): Last reviewed commit: "Rework app navigation and shared page sh..." | Re-trigger Greptile

Comment on lines +77 to +79
if (error) {
setCanChangePassword(true);
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Wrong fallback exposes password form to OAuth users

When listAccounts fails (network error, server error, etc.), canChangePassword is set to true, which renders the password-change form. An OAuth-only user (no credential provider) would see the form and receive a confusing error when they try to use it. The safe default on error is false — if the check can't complete, don't show a form that's likely to fail.

Suggested change
if (error) {
setCanChangePassword(true);
return;
setCanChangePassword(false);

Fix in Codex Fix in Claude Code Fix in Cursor

export function useTableSelection<T extends { id: number }>({
items,
}: UseTableSelectionOptions<T>): UseTableSelectionReturn {
const [selectedIds, setSelectedIds] = useState<Set<number>>(new Set());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Stale selection state across filter changes

selectedIds is never reset when items changes. If a user selects 5 clients then switches to the "archived" filter, the "Selected: 5" counter in AppPageHeaderMeta still shows 5 even though none of those items are visible. Consider resetting on items change via a useEffect, or intersect selectedIds with visible item IDs before computing the displayed count.

Fix in Codex Fix in Claude Code Fix in Cursor

…p UI

- Fix settings dialog: set canChangePassword to false (not true) on error
- Fix invitation email: escape invite link URL to prevent XSS
- Swap static hero image for light/dark theme-aware PNG variants with a gradient fade overlay
- Remove unimplemented star-project button from projects table row
- Sync table selection with visible items using useEffect to avoid stale selections
- Void async handleCreateOrg in team-switcher onKeyDown to satisfy lint
- Strip bg-background from auth form divider spans (cosmetic fix)
- Regenerate routeTree with single-quote style (formatter output)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Berget1411
Berget1411 merged commit aa164ff into main Apr 12, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant