feat(notes): domain access share-modal tab (issue #663)#792
Conversation
Wires the existing `note_domain_access` backend (POST/GET/DELETE /notes/:id/domain-access) into the share modal as a new "domains" tab. Adds the React Query hook, API client methods, client-side free-email validation mirror, and Japanese / English i18n strings. Adding an `editor` rule prompts a confirmation since it grants edit access to the entire domain; `verifiedAt` stays null in v1, so each row carries an "unverified" badge until DNS-TXT verification ships.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughAdds domain-access rule management for notes via a new React Query hook set, API endpoints, UI component within the share modal, client-side domain validation utilities, internationalization strings, and synchronization tests to maintain consistency between server and client domain lists. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Component as ShareModalDomainTab
participant Hooks as React Query Hooks
participant API as API Client
participant Server as Server
User->>Component: Opens domains tab (enabled, noteId)
Component->>Hooks: Call useDomainAccessForNote(noteId)
Hooks->>API: GET /api/notes/{noteId}/domain-access
API->>Server: Query request
Server-->>API: Return domain-access rules
API-->>Hooks: Return rules data
Hooks-->>Component: Render rules with status
User->>Component: Add new domain (role: viewer)
Component->>Component: Validate domain (normalizeDomainInput)
Component->>Hooks: Call useCreateDomainAccess().mutateAsync()
Hooks->>API: POST /api/notes/{noteId}/domain-access
API->>Server: Create request
Server-->>API: Return created rule
API-->>Hooks: Success → Invalidate list query
Hooks->>API: GET /api/notes/{noteId}/domain-access (refetch)
API->>Server: Refetch request
Server-->>API: Return updated rules
API-->>Component: Update UI with new rule
Component->>User: Show success toast
User->>Component: Delete domain rule
Component->>Hooks: Call useDeleteDomainAccess().mutateAsync(accessId)
Hooks->>API: DELETE /api/notes/{noteId}/domain-access/{accessId}
API->>Server: Delete request
Server-->>API: Confirm removal
API-->>Hooks: Success → Invalidate list query
Hooks->>API: GET /api/notes/{noteId}/domain-access (refetch)
API-->>Component: Update UI with removed rule
Component->>User: Show success toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements domain-based access control for notes, allowing owners to grant access to users within specific email domains. It adds API client methods, React Query hooks, client-side domain validation, and a new tab in the share modal. Feedback suggests improving error handling by using machine-readable codes instead of string parsing and handling API fetch errors in the UI to avoid misleading 'no rules' messages.
| const lower = error.message.toLowerCase(); | ||
| if (lower.includes("free email")) { | ||
| message = t("notes.domainTabCreateFailedFreeEmail", { domain }); | ||
| } else if (lower.includes("invalid format")) { | ||
| message = t("notes.domainTabCreateFailedInvalid"); | ||
| } else if (lower.includes("required")) { | ||
| message = t("notes.domainTabCreateFailedEmpty"); | ||
| } |
There was a problem hiding this comment.
The error handling logic here relies on parsing human-readable error messages (e.g., lower.includes("free email")), which is fragile. If the backend changes its error strings, this logic will break. It is recommended to use machine-readable error codes (e.g., error.code) to determine the type of error and display the appropriate message.
| {isLoading ? ( | ||
| <p className="text-muted-foreground text-sm">{t("notes.domainTabLoading")}</p> | ||
| ) : !rules || rules.length === 0 ? ( | ||
| <p className="text-muted-foreground text-sm">{t("notes.domainTabNoRules")}</p> | ||
| ) : ( |
There was a problem hiding this comment.
The component does not handle the error state of the useDomainAccessForNote query. If the API request fails, isLoading will become false and rules will be undefined, which results in the UI showing the "No domain rules yet" message. This is misleading for the user. You should check the isError state from the hook and display an appropriate error message if the fetch fails.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5efd7ca0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) : !rules || rules.length === 0 ? ( | ||
| <p className="text-muted-foreground text-sm">{t("notes.domainTabNoRules")}</p> |
There was a problem hiding this comment.
When useDomainAccessForNote fails (network/500/403), rules stays undefined and this branch renders the empty-state text, so users are told "No domain rules yet" even though loading actually failed. In that scenario owners can make decisions based on a false view of access policy (e.g., adding broad rules while existing ones are just hidden by an error), so the tab should handle isError/error separately instead of treating undefined as an empty list.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Drop fragile substring parsing of server error messages in `submitCreate`; surface the server's `ApiError.message` verbatim as the toast description instead. - Distinguish load failures from empty rule lists by reading `isError` from `useDomainAccessForNote` and rendering a dedicated message so owners aren't told "no rules" when the request actually failed. - Reset `activeTab` to "members" if `showDomainsTab` flips to false while the domains tab is active, preventing the Tabs control from holding a non-existent value and rendering an empty panel.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pages/NoteView/ShareModal/ShareModalDomainTab.tsx (1)
192-195: Add TSDoc to the exportedShareModalDomainTabfunction.As per coding guidelines, exported functions should have TSDoc/JSDoc comments. The component has a bilingual header comment at the file level, but the exported function itself lacks direct documentation.
Suggested addition
+/** + * 共有モーダルのドメインタブ。ドメインルールの追加・一覧・削除を扱う。 + * Domain tab — handles add / list / remove for domain-access rules. + * + * `@param` props - Component props including noteId and enabled flag. + */ export function ShareModalDomainTab({ noteId, enabled }: ShareModalDomainTabProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/NoteView/ShareModal/ShareModalDomainTab.tsx` around lines 192 - 195, Add a TSDoc block immediately above the exported function declaration for ShareModalDomainTab describing its purpose (handles add/list/remove of domain-access rules), annotate the function parameters (document the props parameter and any important fields it uses) with `@param`, and annotate the return type with `@returns` (e.g., JSX.Element or React.ReactElement); ensure the comment follows the project's style (bilingual if preferred) and sits directly above the ShareModalDomainTab function declaration so tooling and docs pick it up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pages/NoteView/ShareModal/ShareModalDomainTab.tsx`:
- Around line 192-195: Add a TSDoc block immediately above the exported function
declaration for ShareModalDomainTab describing its purpose (handles
add/list/remove of domain-access rules), annotate the function parameters
(document the props parameter and any important fields it uses) with `@param`, and
annotate the return type with `@returns` (e.g., JSX.Element or
React.ReactElement); ensure the comment follows the project's style (bilingual
if preferred) and sits directly above the ShareModalDomainTab function
declaration so tooling and docs pick it up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee57d510-1188-4458-bf7a-5597064d8485
📒 Files selected for processing (6)
src/i18n/locales/en/notes.jsonsrc/i18n/locales/ja/notes.jsonsrc/pages/NoteView/ShareModal/NoteShareModal.test.tsxsrc/pages/NoteView/ShareModal/NoteShareModal.tsxsrc/pages/NoteView/ShareModal/ShareModalDomainTab.test.tsxsrc/pages/NoteView/ShareModal/ShareModalDomainTab.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/i18n/locales/ja/notes.json
- src/i18n/locales/en/notes.json
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…test Per AGENTS.md "Sharing constants between server and client" the canonical copy of values duplicated between client and server must live in `packages/shared` with a CI drift detector that reads the server source via `fs.readFileSync` and asserts equality (existing pattern: `src/lib/tagCharacterClassSync.test.ts`). - Move `FREE_EMAIL_DOMAINS`, `DOMAIN_REGEX`, `normalizeDomainInput`, and validation types to `packages/shared/src/freeEmailDomains.ts`. - Convert `src/lib/domainValidation.ts` to a thin re-export from `@zedi/shared/freeEmailDomains` so existing imports keep working. - Move the unit tests for `normalizeDomainInput` to `packages/shared/src/freeEmailDomains.test.ts` (alongside source of truth). - Add `src/lib/freeEmailDomainsSync.test.ts` to detect drift between `@zedi/shared` and the server-side duplicate (`server/api/src/lib/freeEmailDomains.ts`) for both the deny-list and the `DOMAIN_REGEX` pattern. - Document the sync obligation in the server file's header.
2 participants
Wires the existing
note_domain_accessbackend (POST/GET/DELETE/notes/:id/domain-access) into the share modal as a new "domains" tab.
Adds the React Query hook, API client methods, client-side free-email
validation mirror, and Japanese / English i18n strings. Adding an
editorrule prompts a confirmation since it grants edit access tothe entire domain;
verifiedAtstays null in v1, so each row carriesan "unverified" badge until DNS-TXT verification ships.
Summary by CodeRabbit
Release Notes
New Features
Localization