feat(stories): MVP UI (feed/create/card) + auth flow resilience#92
feat(stories): MVP UI (feed/create/card) + auth flow resilience#92ArchILLtect merged 8 commits intomainfrom
Conversation
…-assisted content guard
…auth improvements
✅ Deploy Preview for app-sameboat ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the Stories MVP feature, enabling users to view and create personal stories with content warning support, plus critical auth flow resilience improvements.
Key Changes:
- Stories MVP: Complete implementation with feed page, create form, and trigger-based content shields
- Auth hardening: Login/register flows now gracefully handle edge cases (proxy quirks, empty responses)
- Comprehensive test coverage for new features with unit and integration tests
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/state/auth/store.ts |
Improved auth flow resilience: replaced throwing errors on inflight checks with silent returns, added defensive /api/me fallback for login proxy issues, removed auto-refresh from register to avoid false errors |
src/lib/stories.ts |
New stories library with TypeScript types, runtime guards, and API client functions (listStories, createStory, getStory) |
src/pages/StoriesFeed.tsx |
Public feed page displaying story cards with pagination, error handling for 404/500 responses, and friendly error messages |
src/pages/StoryCreate.tsx |
Protected story creation form with title, content, and optional trigger selection using Chakra UI components |
src/components/StoryCard.tsx |
Story card component with trigger badges and reveal/shield mechanism for sensitive content |
src/routes/AppRoutes.tsx |
Added routes for /stories (public feed) and /stories/new (protected create page) |
src/pages/Home.tsx |
Added "Browse Stories" button in new Explore section linking to stories feed |
src/__tests__/StoryCreate.test.tsx |
Unit test for story creation form submission flow |
src/__tests__/StoryCard.test.tsx |
Unit tests for story card rendering and shield reveal behavior |
src/__tests__/StoriesFeed.test.tsx |
Integration tests for feed loading, shield presence, and 404 error handling |
src/__tests__/Stories.types.test.ts |
Type guard validation tests for Story and StoriesResponse |
src/__tests__/Register.flow.success.test.tsx |
Integration test verifying successful registration without auto-login navigates correctly |
src/__tests__/Login.fallback.me-recovery.test.tsx |
Integration test for login recovery via /api/me when POST fails due to proxy issues |
docs/rfcs/stories-content-guard.md |
New RFC documenting Stories feature design, MVP implementation, and future AI-assisted moderation roadmap |
docs/weekly-reports/week-5/week-5-frontend-plan.md |
Comprehensive weekly plan for Stories v1 implementation with architecture, contracts, and acceptance criteria |
docs/weekly-reports/week-5/week-5-plan-frontend-draft.md |
Added note about type definition inconsistency in draft Story type |
docs/weekly-reports/week-5/week-5-issues.md |
Renumbered issue templates from 1-13 to 77-90 for sequential tracking |
TTD.md |
Added section on auth polish including post-register redirect and email verification future work |
CHANGELOG.md |
Documented Stories MVP UI addition, auth flow fixes, new tests, and RFC in Unreleased section |
| @@ -1,7 +1,6 @@ | |||
| import { create } from 'zustand'; | |||
| import type { AuthStore } from './types'; | |||
| import { isBackendAuthErrorPayload, mapAuthError } from './errors'; | |||
There was a problem hiding this comment.
The AUTH_IN_FLIGHT_ERROR constant is no longer imported but still exists in src/state/auth/constants.ts. Since this constant is not used anywhere in the codebase after these changes, consider removing it from the constants file to avoid dead code.
| try { | ||
| const me = await api<unknown>(ME_PATH, { method: 'GET', credentials: 'include' }); | ||
| const u2 = extractRawUser(me); | ||
| if (u2) { | ||
| set({ user: u2, status: 'authenticated', lastFetched: Date.now() }); | ||
| emit('auth:login', { user: u2 }); | ||
| return true; | ||
| } | ||
| } catch { | ||
| // ignore and fall through to mapped error handling below | ||
| } | ||
| // If still no user, treat as soft failure and map below | ||
| throw new Error('Login completed but user session not found'); | ||
| } catch (e) { | ||
| // Fallback: In some production setups a proxy/redirect can cause the POST to appear as a 400, | ||
| // even though the server created the session cookie. Try a defensive /api/me to recover. | ||
| try { | ||
| const me = await api<unknown>(ME_PATH, { method: 'GET', credentials: 'include' }); | ||
| const u = extractRawUser(me); | ||
| if (u) { | ||
| set({ user: u, status: 'authenticated', lastFetched: Date.now() }); | ||
| emit('auth:login', { user: u }); | ||
| return true; | ||
| } | ||
| } catch { | ||
| // ignore; we will map the original error | ||
| } |
There was a problem hiding this comment.
Code duplication: The /api/me fetch logic (lines 142-148 and 158-164) is duplicated with identical implementation. Consider extracting this into a helper function to improve maintainability. For example:
const tryFetchUser = async (): Promise<RawUser | null> => {
try {
const me = await api<unknown>(ME_PATH, { method: 'GET', credentials: 'include' });
return extractRawUser(me) ?? null;
} catch {
return null;
}
};Then use it in both places: const u2 = await tryFetchUser();
|
|
||
| useEffect(() => { | ||
| // initial load | ||
| void fetchPage(); |
There was a problem hiding this comment.
The useEffect has an empty dependency array but references fetchPage which is defined in the component body. This violates React Hook rules because fetchPage is redefined on every render. While this works here since fetchPage is only called once on mount, it would be safer to either:
- Wrap
fetchPageinuseCallbackwith proper dependencies, or - Define the effect body inline and extract
fetchPageonly if needed elsewhere
This ensures ESLint's react-hooks/exhaustive-deps rule is satisfied.
| void fetchPage(); | |
| setLoading(true); | |
| setError(null); | |
| listStories({ limit: 10 }) | |
| .then((res: StoriesResponse) => { | |
| setItems(res.items); | |
| setNextCursor(res.nextCursor); | |
| }) | |
| .catch((e) => { | |
| const raw = e instanceof Error ? e.message : String(e); | |
| const m = raw.match(/^(\d{3})\b/); | |
| setError({ code: m ? Number(m[1]) : undefined, message: raw }); | |
| }) | |
| .finally(() => { | |
| setLoading(false); | |
| }); |
| const g = globalThis as unknown as { fetch: typeof fetch }; | ||
| const originalFetch = g.fetch; | ||
| const fetchMock = vi.fn(async (url: string, init?: RequestInit) => { | ||
| const method = (init?.method || 'GET').toString().toUpperCase(); | ||
| if (url.toString().endsWith('/api/auth/register') && method === 'POST') { | ||
| // Backend creates the user but does not start a session nor return a body | ||
| return new Response('', { status: 201, statusText: 'Created' }) as unknown as Response; | ||
| } | ||
| // /api/me should not be called by register() in this scenario, but return 401 if probed | ||
| if (url.toString().endsWith('/api/me')) { | ||
| return new Response('unauthorized', { status: 401, statusText: 'Unauthorized' }) as unknown as Response; | ||
| } | ||
| return new Response('not found', { status: 404, statusText: 'Not Found' }) as unknown as Response; | ||
| }) as unknown as typeof fetch; | ||
| g.fetch = fetchMock; | ||
|
|
||
| render( | ||
| <AuthProvider> | ||
| <MemoryRouter initialEntries={[{ pathname: '/register' }] }> | ||
| <Routes> | ||
| <Route path="/" element={<div data-testid="home">HOME</div>} /> | ||
| <Route path="/register" element={<Register />} /> | ||
| </Routes> | ||
| </MemoryRouter> | ||
| </AuthProvider> | ||
| ); | ||
|
|
||
| // Fill and submit | ||
| fireEvent.change(screen.getByLabelText(/email/i), { target: { value: 'email2@email.com' } }); | ||
| fireEvent.change(screen.getByLabelText(/password/i), { target: { value: 'secret1' } }); | ||
| fireEvent.click(screen.getByRole('button', { name: /sign up/i })); | ||
|
|
||
| // We should navigate to "/" (fallback) and not see an error alert | ||
| await waitFor(() => expect(screen.getByTestId('home')).toBeInTheDocument()); | ||
| expect(screen.queryByRole('alert')).toBeNull(); | ||
|
|
||
| // restore | ||
| g.fetch = originalFetch; |
There was a problem hiding this comment.
Inconsistent indentation: lines 9-23 and 46 have misaligned indentation (not using 2-space indent consistently). Lines 9-10 start with no indentation when they should be indented 2 spaces, and line 46 is over-indented. This should be corrected to match the project's consistent 2-space indentation pattern used throughout the rest of the test file.
Inconsistent indentation: lines are not indented correctly. This inconsistency affects readability and should be corrected. Removed redundant check. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
docs/weekly-reports/week-5/week-5-issues.md:7
- The issue numbering jumps from sequential (1-13) to 77-90 without explanation. This creates confusion about whether issues 14-76 exist elsewhere or if the numbering is arbitrary. Consider adding a comment explaining the numbering scheme or using a consistent sequential numbering within this document.
|
|
||
| async login(email: string, password: string) { | ||
| if (get().inFlight) throw new Error(AUTH_IN_FLIGHT_ERROR); | ||
| if (get().inFlight) return false; |
There was a problem hiding this comment.
The login method now returns false when another auth operation is in flight, but callers may not distinguish this from actual authentication failure. Consider returning a more descriptive result (e.g., { success: false, reason: 'busy' }) or documenting this behavior explicitly in the return type.
| if (get().inFlight) return false; | ||
| set({ inFlight: true, status: 'loading', errorCode: undefined, errorMessage: undefined }); | ||
| try { | ||
| // Primary request: attempt login |
There was a problem hiding this comment.
[nitpick] The comment 'Primary request: attempt login' is redundant as it simply restates what the following code obviously does. Consider removing or making it more substantive (e.g., explaining why there's a primary/fallback pattern).
| // Primary request: attempt login |
| // Some backends may not return a body; attempt explicit /api/me fetch using the new session | ||
| try { | ||
| const me = await api<unknown>(ME_PATH, { method: 'GET', credentials: 'include' }); | ||
| const u2 = extractRawUser(me); | ||
| if (u2) { | ||
| set({ user: u2, status: 'authenticated', lastFetched: Date.now() }); | ||
| emit('auth:login', { user: u2 }); | ||
| return true; | ||
| } | ||
| } catch { | ||
| // ignore and fall through to mapped error handling below | ||
| } |
There was a problem hiding this comment.
The login method now has three separate blocks that fetch /api/me and set user state (lines 142-148, 158-163, and originally in the refresh call). This duplicated logic should be extracted into a helper function to improve maintainability and reduce the risk of inconsistent behavior.
| // ignore and fall through to mapped error handling below | ||
| } | ||
| // If still no user, treat as soft failure and map below | ||
| throw new Error('Login completed but user session not found'); |
There was a problem hiding this comment.
The error message 'Login completed but user session not found' may confuse developers debugging this path, as it's unclear whether the login API succeeded or failed. Consider a message that clarifies this is a validation error, e.g., 'Login API succeeded but returned no user data and /api/me also failed'.
| throw new Error('Login completed but user session not found'); | |
| throw new Error('Login API succeeded but returned no user data, and /api/me also failed to return user data'); |
|
|
||
| async register(email: string, password: string) { | ||
| if (get().inFlight) throw new Error(AUTH_IN_FLIGHT_ERROR); | ||
| if (get().inFlight) return false; |
There was a problem hiding this comment.
The register method now returns false when another auth operation is in flight, but this is indistinguishable from actual registration failure. Consider returning a more descriptive result or documenting this silent failure mode in the return type.
| const res = await api<unknown>(REGISTER_PATH, { method: 'POST', body: JSON.stringify({ email, password }), credentials: 'include' }); | ||
| const u = extractRawUser(res); | ||
| if (u) { | ||
| // Some backends auto-login on register and return the user |
There was a problem hiding this comment.
[nitpick] This comment clarifies behavior but uses the informal term 'auto-login'. Consider using 'automatically authenticate' or 'create session' for consistency with technical documentation.
| // Some backends auto-login on register and return the user | |
| // Some backends automatically authenticate on register and return the user |
| await get().refresh(); | ||
| // Many backends create the account without authenticating. Treat success as non-authenticated success. | ||
| // Do NOT call refresh() here; it may legitimately 401 and surface as an error. | ||
| set({ status: 'idle' }); |
There was a problem hiding this comment.
Setting status: 'idle' after successful registration leaves inFlight: true until the finally block runs. This could cause race conditions if other code checks the state between line 195 and the finally block. Consider also setting inFlight: false here or restructuring to ensure consistent state.
| set({ status: 'idle' }); | |
| set({ status: 'idle', inFlight: false }); |
| <Text opacity={0.7}>Loading…</Text> | ||
| )} | ||
| {error && ( | ||
| <Alert status={error.code && error.code >= 500 ? 'error' : error.code === 404 ? 'warning' : 'error'}> |
There was a problem hiding this comment.
The ternary expression for Alert status is complex and hard to read. The logic evaluates to 'error' in most cases except 404. Consider extracting this into a helper function or simplifying to error.code === 404 ? 'warning' : 'error' since both >= 500 and no code result in 'error'.
| <Alert status={error.code && error.code >= 500 ? 'error' : error.code === 404 ? 'warning' : 'error'}> | |
| <Alert status={error.code === 404 ? 'warning' : 'error'}> |
| type Story = { id: string; authorId: string; content: string; createdAt: string; updatedAt?: string | null }; | ||
|
|
||
| // Type definition inconsistency: this minimal Story type lacks title and triggers fields that are present in week-5-frontend-plan.md lines 51-62. This will cause type mismatches when implementing the components described in lines 54-68 which expect these fields. | ||
|
|
There was a problem hiding this comment.
This inline comment in a markdown planning document notes a known inconsistency. Since the actual implementation in src/lib/stories.ts has the correct type with title and triggers, consider removing this draft file or updating it to reflect the final implementation to avoid confusion.
| type Story = { id: string; authorId: string; content: string; createdAt: string; updatedAt?: string | null }; | |
| // Type definition inconsistency: this minimal Story type lacks title and triggers fields that are present in week-5-frontend-plan.md lines 51-62. This will cause type mismatches when implementing the components described in lines 54-68 which expect these fields. | |
| type Story = { | |
| id: string; | |
| authorId: string; | |
| title: string; | |
| content: string; | |
| triggers: string[]; | |
| createdAt: string; | |
| updatedAt?: string | null; | |
| }; |
This pull request introduces the MVP for the Stories feature, including its public and protected routes, UI components, documentation, and tests. It also improves the authentication flow for registration and login resilience, and adds RFC documentation for AI-assisted content guard. Several supporting files and issue templates are updated to reflect these changes.
Stories Feature Implementation
/stories) renderingStoryCarditems with trigger shields and badges./stories/new) with a form for title, content, and optional triggers, posting viacreateStory.StoryCardcomponent with a reveal flow for shielded content.docs/rfcs/stories-content-guard.md.StoryCardreveal, feed shield presence, and create form submission.Authentication Flow Improvements
/api/mecalls./api/auth/loginfails but session is created, using defensive/api/meto finalize auth.Documentation & Planning
Type Consistency
Storyin planning docs, highlighting a mismatch between minimal and full types expected in UI components.This PR closes issues:
#77 – feat(stories): Types + client helpers (src/lib/stories.ts)
#78 – feat(stories): StoriesFeed page (/stories) with friendly 404/500
#79 – feat(stories): StoryCreate page (/stories/new), protected, client
#82 – chore(routes): wire stories routes + guard /stories/new
#84 – test(stories): StoriesFeed tests
#85 – test(stories): StoryCreate tests
#90 – ci: gates remain green (this PR is lint/typecheck/tests/build green)