feat: resolve 4 issues — ESLint rule, security audit, supabase dir, public sharing#130
Merged
ryota-murakami merged 6 commits intomainfrom Feb 18, 2026
Merged
Conversation
…#129 - Add no-restricted-imports rule for next/cache revalidatePath & revalidateTag - Add _trials/ to ESLint global ignores (pre-existing parse errors)
- Remove unsafe-eval from CSP script-src (only dev tooling needs it) - Add Sentry report-uri for CSP violation tracking - Add upgrade-insecure-requests directive - Add E2E XSS smoke test: script tags, img onerror, javascript: URL - Audit confirms: zero dangerouslySetInnerHTML, all user input escaped
…loses #126 Aligns with Supabase CLI default convention. Updates all CI workflows, scripts, and documentation references accordingly.
- Add is_public/share_slug columns to board table with RLS policies - Add toggleBoardPublic/regenerateShareSlug server actions - Add /public/[slug] route with lightweight read-only kanban view - Add Sharing tab to BoardSettingsDialog with toggle + copy link - Add Globe indicator on public boards in board list - Add IP-based rate limiting for public board views - Add E2E tests for public board 404 handling - Update all mock board factories with new fields
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR implements Phase 1 public board sharing (DB + APIs + UI), moves the Supabase directory from Changes
Sequence Diagram(s)sequenceDiagram
actor Browser
participant PublicPage as /public/[slug] Page
participant ServerAction as getPublicBoardBySlug()
participant RateLimit as RateLimiter
participant DB as Supabase
participant PublicClient as PublicBoardClient
Browser->>PublicPage: GET /public/abc123def456
PublicPage->>ServerAction: request data for slug
ServerAction->>RateLimit: check IP quota
alt rate limit exceeded
RateLimit-->>ServerAction: deny
ServerAction-->>PublicPage: null (429)
PublicPage-->>Browser: 429 / Not allowed
else allowed
RateLimit-->>ServerAction: allow
ServerAction->>DB: SELECT board WHERE share_slug=? AND is_public=true
alt board not found
DB-->>ServerAction: null
ServerAction-->>PublicPage: null
PublicPage-->>Browser: 404
else found
DB-->>ServerAction: board
par fetch lists and cards
ServerAction->>DB: SELECT statusLists WHERE board_id=...
ServerAction->>DB: SELECT repoCards WHERE board_id=...
and
DB-->>ServerAction: statusLists, repoCards
end
ServerAction-->>PublicPage: PublicBoardData
PublicPage->>PublicClient: render read-only board
PublicClient-->>Browser: HTML (Kanban view)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #130 +/- ##
==========================================
- Coverage 71.46% 71.03% -0.44%
==========================================
Files 145 145
Lines 4335 4381 +46
Branches 1130 1148 +18
==========================================
+ Hits 3098 3112 +14
- Misses 1216 1248 +32
Partials 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
123-127:⚠️ Potential issue | 🟡 MinorReplace bare
supabasecommands withpnpm db:stop && pnpm db:start.Running
supabase stop && supabase startfrom the project root skipssource .env, so GitHub OAuth credentials won't be loaded. Thepnpm db:*scripts handle thecdand credential sourcing correctly.📝 Proposed fix
-```bash -supabase stop && supabase start -``` +```bash +pnpm db:stop && pnpm db:start +```Based on learnings: "Always use the npm scripts
pnpm db:start,pnpm db:stop,pnpm db:resetto manage local Supabase; do not run baresupabase startfrom project root as it will NOT load GitHub OAuth credentials."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 123 - 127, Update the README command that restarts Supabase: replace the bare `supabase stop && supabase start` with the project's npm script pair `pnpm db:stop && pnpm db:start` so the .env sourcing and GitHub OAuth credentials are loaded correctly; edit the section containing the current command block and swap the command text and fenced-code language to the proposed `pnpm db:stop && pnpm db:start`.SPEC.md (1)
396-412:⚠️ Potential issue | 🟡 MinorCSP section is stale after the Issue
#127hardening changes.The table on line 403 still shows
Content-Security-Policy-Report-Onlyand the directives block on lines 407–412 still includes'unsafe-eval'inscript-src. This PR's Issue#127explicitly removedunsafe-evaland (per the PR description) addsreport-uriandupgrade-insecure-requests. The spec should be updated to match the actual enforced header name and directives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SPEC.md` around lines 396 - 412, Update the CSP section to match the hardened policy: change the table entry named `Content-Security-Policy-Report-Only` to the enforced `Content-Security-Policy`, remove `'unsafe-eval'` from the `script-src` directive, and add the `report-uri` and `upgrade-insecure-requests` directives to the CSP block; ensure the `script-src`, `style-src`, `img-src`, `connect-src`, and other directives in the code block reflect the post-#127 policy (no `'unsafe-eval'`, include `report-uri=<your-report-endpoint>` and `upgrade-insecure-requests`) so the SPEC matches the actual headers being sent.
🧹 Nitpick comments (9)
next.config.ts (1)
60-62: Misleading comment:unsafe-inlinereason references styles, but this isscript-src.Line 60's comment says "unsafe-inline needed for Next.js inline styles & Tailwind" — that rationale applies to
style-src(line 63), notscript-src. Forscript-src,unsafe-inlineis needed because Next.js emits inline<script>tags for hydration data. Consider updating the comment to reflect the actual reason.Suggested comment fix
- // unsafe-inline needed for Next.js inline styles & Tailwind - // unsafe-eval removed — only dev tooling (code-inspector-plugin) needs it + // unsafe-inline needed for Next.js inline <script> tags (hydration data) + // unsafe-eval removed — only dev tooling (code-inspector-plugin) needs it🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@next.config.ts` around lines 60 - 62, The comment above the CSP entry for "script-src 'self' 'unsafe-inline' https://va.vercel-scripts.com" is misleading: change it to explain that 'unsafe-inline' is present for inline Next.js scripts (e.g., hydration/data scripts) rather than for styles; keep the separate comment for "style-src" (where Tailwind/inline styles require 'unsafe-inline') so the rationale for both "script-src" and "style-src" is accurate and clearly tied to those directives.e2e/logged-in/xss-smoke.spec.ts (1)
59-60: Replace fixedwaitForTimeoutwith a deterministic wait.
page.waitForTimeout(1000)after pressing Enter is a timing guess. If the save round-trip is slower than 1 s the assertion fires prematurely; if it's consistently much faster the suite wastes time. Waiting for thecomment-textelement to update is both faster and reliable:♻️ Proposed fix (same pattern for both comment tests)
- await textarea.press('Enter') - - // Wait for save to process - await page.waitForTimeout(1000) - - // Verify no alert fired - expect(alertFired).toBe(false) - - // Verify the XSS payload is rendered as escaped text, not executed - const commentText = page.locator( - `[data-testid="repo-card-${CARD_IDS.card1}"] [data-testid="comment-text"]`, - ) - await expect(commentText).toContainText('<script>') + await textarea.press('Enter') + + const commentText = page.locator( + `[data-testid="repo-card-${CARD_IDS.card1}"] [data-testid="comment-text"]`, + ) + // Waiting for the element to contain the saved payload is the save-completion signal + await expect(commentText).toContainText('<script>', { timeout: 5000 }) + + expect(alertFired).toBe(false)Also applies to: 92-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/logged-in/xss-smoke.spec.ts` around lines 59 - 60, Replace the brittle page.waitForTimeout(1000) with a deterministic wait that waits for the comment text to update after the Enter press: after the action that triggers save (the Enter key press), use the comment element selector (e.g. locator('.comment-text') or the "comment-text" selector used in the test) and call page.waitForSelector or waitForFunction to assert the element is visible or its text/content has changed to the expected value; do the same replacement for the second test that currently uses waitForTimeout at the other location (the lines referenced as also applies to 92-93).src/app/board/[id]/BoardPageClient.stories.tsx (1)
34-48: Consider exposingis_public/share_slugas overridable and adding a public board story.The
overridesPartialdoesn't includeis_publicorshare_slug, so there's no way to compose a story withis_public: true. Per coding guidelines, stories should cover all component states.♻️ Proposed refactor
const createMockBoard = ( overrides?: Partial<{ id: string name: string subtitle: string | null + is_public: boolean + share_slug: string | null }>, ): Board => ({ id: overrides?.id ?? 'board-1', name: overrides?.name ?? 'My Kanban Board', subtitle: overrides?.subtitle ?? null, settings: null, user_id: 'user-1', is_favorite: false, - is_public: false, - share_slug: null, + is_public: overrides?.is_public ?? false, + share_slug: overrides?.share_slug ?? null, position: 0, created_at: '2024-01-01T00:00:00.000Z', updated_at: '2024-01-01T00:00:00.000Z', })Then add:
export const PublicBoard: Story = { args: { board: createMockBoard({ is_public: true, share_slug: 'my-board-abc123' }), initialData: createMockInitialData(), }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/board/`[id]/BoardPageClient.stories.tsx around lines 34 - 48, The createMockBoard factory currently doesn't allow overriding is_public or share_slug, so add those keys to the overrides Partial and return values (e.g., include is_public?: boolean and share_slug?: string in the overrides type and use overrides?.is_public ?? false and overrides?.share_slug ?? null in the returned object inside createMockBoard) and then add a new Story export (e.g., PublicBoard) that uses createMockBoard({ is_public: true, share_slug: 'my-board-abc123' }) together with createMockInitialData() to cover the public-board state.src/components/Boards/BoardCard.stories.tsx (1)
17-29: Missing story for theis_public: truestate (Globe indicator).
BoardCard.tsxconditionally renders a Globe icon whenboard.is_publicis true — a new visual state with no story coverage. As per coding guidelines, stories should cover all component states.📖 Suggested story
+/** + * Public board card — shows the globe indicator. + */ +export const PublicBoard: Story = { + args: { + board: { + ...mockBoard, + id: 'board-public', + name: 'Public Project Board', + is_public: true, + share_slug: 'my-public-board', + }, + }, +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Boards/BoardCard.stories.tsx` around lines 17 - 29, Add a new story for the public board state so the Globe indicator appears: create a variant of the existing mockBoard (e.g., mockBoardPublic) with is_public: true (and set share_slug to a non-null value if the component uses it) and add a Storybook export (e.g., PublicBoard or BoardCardPublic) that renders BoardCard with that mockBoardPublic; update BoardCard.stories.tsx to include this new story alongside the existing ones so the is_public visual state is covered.src/tests/unit/components/Boards/BoardSettingsDialog.test.tsx (1)
46-54: Sharing tab has no test coverage — consider adding at minimum a smoke test.
defaultPropsomitsisPublicandshareSlug, and no test navigates to the Sharing tab to verify its content. A minimal test for the new tab would look like:it('should switch to Sharing tab when clicked', () => { render( <BoardSettingsDialog {...defaultProps} isPublic={false} shareSlug={null} /> ) fireEvent.click(screen.getByRole('tab', { name: /sharing/i })) expect(screen.getByRole('tab', { name: /sharing/i })).toHaveAttribute('aria-selected', 'true') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tests/unit/components/Boards/BoardSettingsDialog.test.tsx` around lines 46 - 54, Add a minimal smoke test for the new Sharing tab in BoardSettingsDialog by updating the test setup to pass isPublic and shareSlug (either add them to defaultProps or supply them when rendering BoardSettingsDialog) and add a test that renders <BoardSettingsDialog ...>, fires a click on the tab with role 'tab' and name /sharing/i, and asserts that this tab has aria-selected="true" (use existing helpers like render, fireEvent, and screen to locate the tab); reference defaultProps and BoardSettingsDialog to find where to inject the props and where to add the new it('should switch to Sharing tab when clicked') test.supabase/migrations/20260219100000_add_public_board.sql (2)
30-48: RLS EXISTS subqueries onstatuslist/repocardmay benefit from an index onboard(is_public).Both policies perform a correlated
EXISTS (SELECT 1 FROM board WHERE board.id = ... AND board.is_public = true). Without an index coveringis_public, every row evaluation falls back to a primary-key lookup + heap fetch to check the boolean column. A partial index would make this index-only for the commonfalsecase and very fast for thetruecase:CREATE INDEX idx_board_is_public ON board(id) WHERE is_public = true;This is especially relevant as row counts on
statuslistandrepocardgrow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260219100000_add_public_board.sql` around lines 30 - 48, Add a partial index to speed the RLS EXISTS checks against board.is_public by creating an index on board(id) filtered to is_public = true (suggested name: idx_board_is_public) so the policies "Anyone can view public board status lists" and "Anyone can view public board repo cards" that use EXISTS(SELECT 1 FROM board WHERE board.id = ... AND board.is_public = true) can hit an index-only plan; implement this by adding a CREATE INDEX ... ON board(id) WHERE is_public = true statement in the migration so the correlated lookups for statuslist.board_id and repocard.board_id are efficient.
18-18:share_sluglacks a DB-level format constraint — consider adding aCHECK.The application validates slugs with
/^[a-f0-9]{12}$/, but there is no database-level guard. A directUPDATEbypassing the application (e.g., a migration, seed script, or admin panel) could set an arbitrary value that would then fail runtime lookups or expose unexpected behavior.🛠️ Proposed addition
ALTER TABLE board ADD COLUMN share_slug text + CHECK (share_slug ~ '^[a-f0-9]{12}$');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260219100000_add_public_board.sql` at line 18, The new share_slug column on table "board" needs a DB-level CHECK to enforce the same format as the app (hex lowercase 12 chars); add a constraint (named e.g. board_share_slug_format_check) on column share_slug that allows NULL but validates non-null values match the regex /^[a-f0-9]{12}$/ (Postgres text ~ '^[a-f0-9]{12}$' style). Update the migration to add this CHECK (or ALTER TABLE ... ADD CONSTRAINT ...) so application updates and direct SQL both enforce the slug format.src/lib/actions/board.ts (1)
1159-1195:toggleBoardPublicvalidatesboardIdinside the rate-limit callback.Other actions in this file (e.g.,
renameBoardDirect,updateBoardPositions) validate inputs before enteringwithAuthResultRateLimit. Here, a malformedboardIdstill burns a rate-limit token. Minor inconsistency but worth aligning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/actions/board.ts` around lines 1159 - 1195, Validate boardId with boardIdSchema before calling withAuthResultRateLimit in toggleBoardPublic to avoid consuming a rate-limit token for malformed IDs; move the boardIdSchema.safeParse check out of the withAuthResultRateLimit callback (or perform the same validation immediately before invoking withAuthResultRateLimit), throw the same error on failure, and then pass the validated id into the callback so the rest of the logic (Supabase queries and generateSlug usage) remains unchanged.src/app/public/[slug]/PublicBoardClient.tsx (1)
82-96: Hardcoded domain in footer links.
https://gitbox-laststance.vercel.appis repeated twice. If the deployment URL changes, these break. Consider using an environment variable orwindow.location.origin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/public/`[slug]/PublicBoardClient.tsx around lines 82 - 96, The two hardcoded hrefs in the PublicBoardClient component (the <a> rendering "GitBox" and the <a> rendering "Create your own board") should use a configurable origin instead of the literal "https://gitbox-laststance.vercel.app"; replace them to build their hrefs from a runtime-safe value such as process.env.NEXT_PUBLIC_APP_URL with a fallback to window.location.origin when available (e.g. const origin = process.env.NEXT_PUBLIC_APP_URL || (typeof window !== 'undefined' ? window.location.origin : '')); update both anchor hrefs to `${origin}` and ensure you use the unique JSX texts ("GitBox" and "Create your own board") to locate the anchors in PublicBoardClient.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/supabase-production.yml:
- Line 31: The workflow line calling supabase link should quote the project ref
variable to avoid word-splitting (SC2086); update the command that executes
supabase link --project-ref $SUPABASE_PROJECT_REF to use a quoted variable
supabase link --project-ref "$SUPABASE_PROJECT_REF" so the shell treats the
project ref as a single argument and silences the actionlint/shellcheck warning.
In `@e2e/logged-in/xss-smoke.spec.ts`:
- Around line 39-42: The dialog handler currently registered with
page.on('dialog', ...) only sets alertFired = true and never dismisses the
dialog, causing the browser to wait and tests to hang; update the handler
signature to accept the dialog parameter and call dialog.dismiss() (awaiting it
if the handler is async) so the alert is dismissed after setting alertFired;
apply the same change for the three occurrences around the lines referencing
page.on('dialog', ...) and the alertFired variable.
- Around line 28-35: The beforeEach currently calls resetProjectInfoComments()
but doesn't clear edits to the repocard comment field, so XSS payload saved by
one test can persist into the next; add a resetCardComments() helper (similar to
resetProjectInfoComments()) that clears the comment field for CARD_IDS.card1
(and any other cards used by these tests) or alternatively update the tests to
use distinct CARD_IDS for each test, then call resetCardComments() from
test.beforeEach (or ensure each test resets its card) to guarantee a clean
repocard inline-editor state before each spec run.
- Around line 119-146: The test currently uses conditional if-checks that
silently skip XSS assertions when elements are absent; replace those guards with
hard assertions: assert presence/visibility of the edit button or Add button
(use expect(editButton).toBeVisible() or expect(addLink).toBeVisible() and then
click), assert url input is visible (expect(urlInput).toBeVisible()) before
filling with XSS_JS_URL and pressing Tab, and assert the error alert is visible
and contains the expected text (expect(error).toBeVisible();
expect(error).toContainText('http')); remove the .catch(() => false) swallowing
logic so missing elements fail the test instead of skipping, and keep the final
expect(alertFired).toBe(false).
In `@e2e/unauthenticated/public-board.spec.ts`:
- Around line 32-51: The test 'should not expose edit controls on public page'
in e2e/unauthenticated/public-board.spec.ts is asserting against a non-existent
slug that yields a 404, so replace the vacuous check by either (A) creating or
using an existing test fixture board with is_public = true and navigate to that
board's slug (use the fixture's slug in the page.goto call) then assert the
buttons (add column, board settings, open menu) are not visible, or (B) if you
intend to test the 404 behavior, rename the test to reflect "renders 404 for
unknown public slug" and assert the 404 page content rather than absence of edit
controls; update the test title and page.goto target accordingly and ensure you
reference the test name 'should not expose edit controls on public page' when
making the change.
- Line 35: The page navigation in the test uses
page.goto('/public/aabbccddeeff') without the required wait strategy; update the
call to use Playwright's network idle wait (i.e., pass the navigation option
waitUntil: 'networkidle' to page.goto) so the test waits for network quiescence
before asserting UI in public-board.spec.ts (locate the page.goto invocation).
In `@next.config.ts`:
- Around line 71-73: The CSP entry 'upgrade-insecure-requests' in next.config.ts
is ineffective because the CSP is sent as Report-Only; either remove that
directive from the report-only policy or add a clear inline comment next to the
CSP definition (where you assemble the directives such as
'upgrade-insecure-requests' and 'report-uri ...') stating that it is ignored in
Report-Only and will only take effect when you switch to an enforced
Content-Security-Policy header; alternatively move 'upgrade-insecure-requests'
into the enforced CSP configuration when you create one.
In `@package.json`:
- Around line 32-34: The npm scripts db:start and db:reset use the Bash-only
command `source .env`, which fails under POSIX /bin/sh (e.g., dash in CI);
update those scripts (the "db:start" and "db:reset" entries) to use the
POSIX-compatible `. .env` form so the environment file is sourced reliably
across shells and CI environments.
In `@src/app/public/`[slug]/page.tsx:
- Around line 18-46: Both generateMetadata and PublicBoardPage call
getPublicBoardBySlug(slug) causing duplicate work; fix by creating a cached
wrapper around that function using React.cache (e.g., const
getPublicBoardBySlugCached = React.cache(getPublicBoardBySlug)) and replace both
direct calls in generateMetadata and PublicBoardPage to call
getPublicBoardBySlugCached(slug) so the same result is reused within the same
request, preserving the original function and signatures.
In `@src/components/Boards/BoardCard.tsx`:
- Around line 215-220: The Globe SVG used in BoardCard (the JSX block rendering
{board.is_public && <Globe ... aria-label="Public board" />}) is relying on
aria-label without a role which is unreliable for some screen readers; update
the markup to make the icon accessible by either adding role="img" to the Globe
element (preferred minimal change) or replacing the aria-label with a
visually-hidden <span> containing "Public board" adjacent to the Globe and
remove the aria-label, ensuring the visible layout is unchanged; target the
Globe usage inside the BoardCard component to implement one of these fixes.
In `@src/components/Boards/BoardSettingsDialog.tsx`:
- Around line 223-230: The handleCopyShareLink function currently calls
navigator.clipboard.writeText(url) without handling errors; make it async (or
handle the returned promise) and wrap the writeText call in try/catch so
rejections don’t cause unhandled promise errors, only call setCopied(true) and
toast.success when the write succeeds, and in the catch branch log the error and
show a user-facing failure toast (or fallback to a prompt/selection fallback) so
the UI remains consistent; reference the handleCopyShareLink function,
navigator.clipboard.writeText, setCopied, and toast.success when making the
change.
- Around line 193-197: The sharing state variables
boardIsPublic/setBoardIsPublic and boardShareSlug/setBoardShareSlug are only
initialized from initialIsPublic and initialShareSlug and never resync when
props change; add the same sync pattern used for boardName by adding a useEffect
that watches initialIsPublic and initialShareSlug and updates
setBoardIsPublic(initialIsPublic) and setBoardShareSlug(initialShareSlug)
respectively so the dialog reflects prop updates after mount. Ensure the effect
lists initialIsPublic and initialShareSlug as dependencies and does not
overwrite local changes unintentionally (mirror the existing boardName sync
behavior).
In `@src/lib/actions/board.ts`:
- Around line 1205-1229: The regenerateShareSlug function currently treats
Supabase .update() as successful even when no rows match (board missing or not
owned), so modify the function to verify the update affected a row—either check
the update response's row count (e.g., use .update(...).select() and verify
returned data length) or perform .update(...).select().single() and confirm a
non-null result; if no row was updated, capture the condition with Sentry
(similar to existing error handling) and throw an error like 'Board not found or
not owned' instead of returning the new slug. Ensure you reference
regenerateShareSlug, the .update(...) call, and the Sentry.captureException
block when adding the check and error path.
In `@src/lib/actions/public-board.ts`:
- Around line 46-50: The current fallback of 'unknown' for ip in the
publicBoardView rate limiting causes a shared bucket; update the IP resolution
logic around headerStore = await headers(), headerStore.get('x-forwarded-for')
and the ip variable so that if x-forwarded-for is missing you use the request
socket remote address (e.g., request.socket.remoteAddress) as a secondary signal
and only fall back to a truly unique placeholder as last resort; also call
processLogger.warn or similar when the fallback to socket or final placeholder
occurs so we can monitor how often it happens, and then pass that resolved ip
into checkRateLimit('publicBoardView', ip). Ensure you reference and update the
ip variable and the call site for checkRateLimit accordingly.
- Around line 58-65: The handler currently uses
supabase.from('board').select('*') returning a full board object (variable
board) which leaks private fields to PublicBoardData.board; change the query to
select only the public columns required (e.g.,
.select('id,name,subtitle,share_slug,created_at') or the exact column list your
public view needs) and update the PublicBoardData.board type to a narrowed
Pick<Board, 'id' | 'name' | 'subtitle' | ...> so only those fields are
serialized, and apply the same change to the other occurrence around the second
select (the line referenced at 121) to ensure no private fields are returned.
- Around line 97-119: Extract the duplicate mapping logic that converts a
RepoCardRow to RepoCardDomain into a single function toRepoCardDomain(row:
RepoCardRow): RepoCardDomain inside mappers.ts (similar to toStatusListDomain),
moving the object construction (title, description, statusId, boardId,
repoOwner, repoName, order, meta fields, createdAt/updatedAt fallbacks) there;
then replace the inline mapper in public-board.ts (the repoCards mapping) and
the identical mapping in board.ts (getRepoCards) to call toRepoCardDomain(row)
and import it from mappers.ts so both places reuse the shared function.
---
Outside diff comments:
In `@README.md`:
- Around line 123-127: Update the README command that restarts Supabase: replace
the bare `supabase stop && supabase start` with the project's npm script pair
`pnpm db:stop && pnpm db:start` so the .env sourcing and GitHub OAuth
credentials are loaded correctly; edit the section containing the current
command block and swap the command text and fenced-code language to the proposed
`pnpm db:stop && pnpm db:start`.
In `@SPEC.md`:
- Around line 396-412: Update the CSP section to match the hardened policy:
change the table entry named `Content-Security-Policy-Report-Only` to the
enforced `Content-Security-Policy`, remove `'unsafe-eval'` from the `script-src`
directive, and add the `report-uri` and `upgrade-insecure-requests` directives
to the CSP block; ensure the `script-src`, `style-src`, `img-src`,
`connect-src`, and other directives in the code block reflect the post-#127
policy (no `'unsafe-eval'`, include `report-uri=<your-report-endpoint>` and
`upgrade-insecure-requests`) so the SPEC matches the actual headers being sent.
---
Nitpick comments:
In `@e2e/logged-in/xss-smoke.spec.ts`:
- Around line 59-60: Replace the brittle page.waitForTimeout(1000) with a
deterministic wait that waits for the comment text to update after the Enter
press: after the action that triggers save (the Enter key press), use the
comment element selector (e.g. locator('.comment-text') or the "comment-text"
selector used in the test) and call page.waitForSelector or waitForFunction to
assert the element is visible or its text/content has changed to the expected
value; do the same replacement for the second test that currently uses
waitForTimeout at the other location (the lines referenced as also applies to
92-93).
In `@next.config.ts`:
- Around line 60-62: The comment above the CSP entry for "script-src 'self'
'unsafe-inline' https://va.vercel-scripts.com" is misleading: change it to
explain that 'unsafe-inline' is present for inline Next.js scripts (e.g.,
hydration/data scripts) rather than for styles; keep the separate comment for
"style-src" (where Tailwind/inline styles require 'unsafe-inline') so the
rationale for both "script-src" and "style-src" is accurate and clearly tied to
those directives.
In `@src/app/board/`[id]/BoardPageClient.stories.tsx:
- Around line 34-48: The createMockBoard factory currently doesn't allow
overriding is_public or share_slug, so add those keys to the overrides Partial
and return values (e.g., include is_public?: boolean and share_slug?: string in
the overrides type and use overrides?.is_public ?? false and
overrides?.share_slug ?? null in the returned object inside createMockBoard) and
then add a new Story export (e.g., PublicBoard) that uses createMockBoard({
is_public: true, share_slug: 'my-board-abc123' }) together with
createMockInitialData() to cover the public-board state.
In `@src/app/public/`[slug]/PublicBoardClient.tsx:
- Around line 82-96: The two hardcoded hrefs in the PublicBoardClient component
(the <a> rendering "GitBox" and the <a> rendering "Create your own board")
should use a configurable origin instead of the literal
"https://gitbox-laststance.vercel.app"; replace them to build their hrefs from a
runtime-safe value such as process.env.NEXT_PUBLIC_APP_URL with a fallback to
window.location.origin when available (e.g. const origin =
process.env.NEXT_PUBLIC_APP_URL || (typeof window !== 'undefined' ?
window.location.origin : '')); update both anchor hrefs to `${origin}` and
ensure you use the unique JSX texts ("GitBox" and "Create your own board") to
locate the anchors in PublicBoardClient.tsx.
In `@src/components/Boards/BoardCard.stories.tsx`:
- Around line 17-29: Add a new story for the public board state so the Globe
indicator appears: create a variant of the existing mockBoard (e.g.,
mockBoardPublic) with is_public: true (and set share_slug to a non-null value if
the component uses it) and add a Storybook export (e.g., PublicBoard or
BoardCardPublic) that renders BoardCard with that mockBoardPublic; update
BoardCard.stories.tsx to include this new story alongside the existing ones so
the is_public visual state is covered.
In `@src/lib/actions/board.ts`:
- Around line 1159-1195: Validate boardId with boardIdSchema before calling
withAuthResultRateLimit in toggleBoardPublic to avoid consuming a rate-limit
token for malformed IDs; move the boardIdSchema.safeParse check out of the
withAuthResultRateLimit callback (or perform the same validation immediately
before invoking withAuthResultRateLimit), throw the same error on failure, and
then pass the validated id into the callback so the rest of the logic (Supabase
queries and generateSlug usage) remains unchanged.
In `@src/tests/unit/components/Boards/BoardSettingsDialog.test.tsx`:
- Around line 46-54: Add a minimal smoke test for the new Sharing tab in
BoardSettingsDialog by updating the test setup to pass isPublic and shareSlug
(either add them to defaultProps or supply them when rendering
BoardSettingsDialog) and add a test that renders <BoardSettingsDialog ...>,
fires a click on the tab with role 'tab' and name /sharing/i, and asserts that
this tab has aria-selected="true" (use existing helpers like render, fireEvent,
and screen to locate the tab); reference defaultProps and BoardSettingsDialog to
find where to inject the props and where to add the new it('should switch to
Sharing tab when clicked') test.
In `@supabase/migrations/20260219100000_add_public_board.sql`:
- Around line 30-48: Add a partial index to speed the RLS EXISTS checks against
board.is_public by creating an index on board(id) filtered to is_public = true
(suggested name: idx_board_is_public) so the policies "Anyone can view public
board status lists" and "Anyone can view public board repo cards" that use
EXISTS(SELECT 1 FROM board WHERE board.id = ... AND board.is_public = true) can
hit an index-only plan; implement this by adding a CREATE INDEX ... ON board(id)
WHERE is_public = true statement in the migration so the correlated lookups for
statuslist.board_id and repocard.board_id are efficient.
- Line 18: The new share_slug column on table "board" needs a DB-level CHECK to
enforce the same format as the app (hex lowercase 12 chars); add a constraint
(named e.g. board_share_slug_format_check) on column share_slug that allows NULL
but validates non-null values match the regex /^[a-f0-9]{12}$/ (Postgres text ~
'^[a-f0-9]{12}$' style). Update the migration to add this CHECK (or ALTER TABLE
... ADD CONSTRAINT ...) so application updates and direct SQL both enforce the
slug format.
🧪 E2E Coverage Report (Sharded: 12 parallel jobs)
📊 Full report available in workflow artifacts |
- Quote shell variable in CI workflow (shellcheck) - Fix XSS smoke test dialog handlers and ensure assertion coverage - Remove vacuous public board "no edit controls" test on 404 page - Use POSIX `. .env` instead of bash-only `source .env` - Deduplicate getPublicBoardBySlug with React.cache() - Add role="img" to Globe icon for screen readers - Sync sharing state on prop changes in BoardSettingsDialog - Add try/catch to clipboard.writeText - Verify row exists in regenerateShareSlug (prevent silent no-op) - Use explicit column selection in public board query (exclude user_id) - Extract shared toRepoCardDomain mapper to mappers.ts (DRY) - Add CSP comment clarifying upgrade-insecure-requests in report-only
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Boards/BoardSettingsDialog.tsx (1)
757-764:⚠️ Potential issue | 🟡 MinorInclude
isTogglingPublicin the footer Close button'sdisabledguard.The Close button can be clicked mid-toggle, dismissing the dialog before the in-flight
toggleBoardPubliccall completes. WhileonPublicChangewill still fire asynchronously, the user loses visual feedback of the outcome.🛠️ Proposed fix
disabled={isRenamePending || isSettingsPending || isDeletePending} + disabled={isRenamePending || isSettingsPending || isDeletePending || isTogglingPublic}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Boards/BoardSettingsDialog.tsx` around lines 757 - 764, The Close footer Button currently disables only when isRenamePending, isSettingsPending, or isDeletePending are true, allowing the dialog to be dismissed while a public toggle is in-flight; update the Button's disabled guard to also include isTogglingPublic so the Close button is disabled during toggleBoardPublic/onPublicChange operations (locate the Button with onClick={handleClose} in BoardSettingsDialog and add isTogglingPublic to the boolean OR list).
🧹 Nitpick comments (1)
src/lib/actions/public-board.ts (1)
18-28: Confirmsettingsis intentionally exposed to public consumers.
settingscontains card-display preferences (showGitHubDescription,showComment, font size/weight). If the public board view doesn't render these preferences, the field is unnecessary and should be dropped from both the select query and the interface.♻️ If unused by the public view
interface PublicBoard { id: string name: string subtitle: string | null created_at: string | null updated_at: string | null is_public: boolean share_slug: string | null - settings: unknown } // and in the query: - 'id, name, subtitle, created_at, updated_at, is_public, share_slug, settings', + 'id, name, subtitle, created_at, updated_at, is_public, share_slug',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/actions/public-board.ts` around lines 18 - 28, The PublicBoard interface currently exposes the settings field; confirm whether the public board view uses settings (e.g., card display prefs like showGitHubDescription, showComment, font settings). If the public view does not use these preferences, remove settings from the PublicBoard interface and from the SELECT that returns board rows (search for PublicBoard and the code that constructs the public board payload) so it is not returned to public consumers; if the public view does use it, add a comment documenting which keys are required and validate/whitelist those specific settings before exposing them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/logged-in/xss-smoke.spec.ts`:
- Around line 58-70: Reorder and remove flaky timeouts: remove the explicit
page.waitForTimeout(1000) and instead first assert the DOM renders the escaped
payload using the existing locator/expect (the commentText locator and
expect(commentText).toContainText('<script>') for CARD_IDS.card1), then check
the alertFired flag (expect(alertFired).toBe(false)) after that assertion; apply
the same change symmetrically to the second test (the textarea, commentText
locator and alertFired usage there) and remove the redundant
page.waitForTimeout(500) in test 3 where expect(error).toBeVisible({ timeout:
3000 }) already retries. This ensures Playwright's built-in retries drive
stability and eliminates flaky sleeps.
In `@src/lib/actions/board.ts`:
- Around line 1153-1156: The update call that sets is_public/share_slug on the
board currently only filters by id, so add a secondary filter .eq('user_id',
user.id) to the supabase update chain (the block that assigns updateError) for
defense-in-depth; mirror the same two-attribute filtering used by
regenerateShareSlug to ensure the update is scoped to the owning user.
---
Outside diff comments:
In `@src/components/Boards/BoardSettingsDialog.tsx`:
- Around line 757-764: The Close footer Button currently disables only when
isRenamePending, isSettingsPending, or isDeletePending are true, allowing the
dialog to be dismissed while a public toggle is in-flight; update the Button's
disabled guard to also include isTogglingPublic so the Close button is disabled
during toggleBoardPublic/onPublicChange operations (locate the Button with
onClick={handleClose} in BoardSettingsDialog and add isTogglingPublic to the
boolean OR list).
---
Duplicate comments:
In `@e2e/logged-in/xss-smoke.spec.ts`:
- Around line 28-35: The beforeEach hook currently calls
resetProjectInfoComments() but not resetCardComments(), so CARD_IDS.card1 can
retain the XSS payload between tests; update the beforeEach in
e2e/logged-in/xss-smoke.spec.ts to call resetCardComments() (or a combined reset
that clears repo-card comments) before navigating to BOARD_URL so both TEST 1
and TEST 2 start from a clean state and avoid cross-test races when Playwright
runs concurrently; ensure the call is placed alongside
resetProjectInfoComments() and targets the same setup scope used by
CARD_IDS.card1.
- Around line 122-138: The test silently swallows visibility timeouts with
.catch(() => false) causing poor diagnostics; replace the soft-fail checks for
editButton and addLink with explicit visibility expectations so failures
surface: remove the .catch(() => false) calls and use expect(await
editButton.isVisible({ timeout: 3000 })).toBe(true) (or
expect(editButton).toBeVisible() per your test utilities) before clicking,
otherwise assert expect(addLink).toBeVisible() prior to clicking; update
references to enteredEditMode, editButton, addLink and the modal.locator calls
accordingly and then click only after the explicit expectation so the test fails
with a clear message when the button is missing.
In `@src/lib/actions/public-board.ts`:
- Around line 52-53: The current ip variable
(headerStore.get('x-forwarded-for')... || 'unknown') causes all missing-XFF
requests to share the 'unknown' bucket; change the fallback to use other
per-connection sources so clients are distinct: compute ip using
headerStore.get('x-forwarded-for')?.split(',')[0]?.trim() ||
headerStore.get('x-real-ip') || request.socket?.remoteAddress ||
request.connection?.remoteAddress, updating the ip variable assignment in
public-board.ts (reference: headerStore.get and the ip const) so non-proxied
clients are bucketed by their actual remote address instead of the literal
'unknown'.
---
Nitpick comments:
In `@src/lib/actions/public-board.ts`:
- Around line 18-28: The PublicBoard interface currently exposes the settings
field; confirm whether the public board view uses settings (e.g., card display
prefs like showGitHubDescription, showComment, font settings). If the public
view does not use these preferences, remove settings from the PublicBoard
interface and from the SELECT that returns board rows (search for PublicBoard
and the code that constructs the public board payload) so it is not returned to
public consumers; if the public view does use it, add a comment documenting
which keys are required and validate/whitelist those specific settings before
exposing them.
- xss-smoke.spec.ts: remove flaky waitForTimeout, reorder alertFired
assertion after DOM auto-wait assertion (toContainText)
- board.ts toggleBoardPublic: add .eq('user_id', user.id) for
defense-in-depth, matching regenerateShareSlug pattern
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves all 4 open issues in a single PR:
no-restricted-importsrule to banrevalidatePath/revalidateTag(they cause unnecessary page reloads when using Supabase + Redux)supabasedirectory fromsrc/supabaseto the project root #126 — Movesupabase/directory fromsrc/supabaseto project root (Supabase CLI convention)Issue #129: ESLint Rule
no-restricted-importsrule forrevalidatePathandrevalidateTagfromnext/cacheIssue #127: Security Audit
Issue #126: Move Supabase Directory
src/supabase/→supabase/(project root)Issue #38: Public Board Sharing
is_publicboolean +share_slugcolumns on board tabletoggleBoardPublic,regenerateShareSlug,getPublicBoardBySlug/public/[slug]route with lightweight read-only kanban view (~200 lines vs 825-line full KanbanBoard)Closes #129, Closes #127, Closes #126, Closes #38
Test plan
pnpm typecheck— passespnpm lint— passes (0 warnings)pnpm test— 89 files, 1282 tests passingpnpm build— successful production buildpnpm e2e:parallel— E2E tests (CI will run)Summary by CodeRabbit
New Features
Security
Database