Feat/localization tests docker #371
Merged
Conversation
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
…d complete zh-TW support
…tability - Remove unused useTranslation import from ConnectionGuard - Add ref-based checking state to prevent dependency cycles - Fix useTranslation hook to return empty string for undefined translations - Add comment for backward compatibility on ExtractedReference interface - Ensure .replace() string methods work safely with nested translation keys
…deployment - Add LanguageLoadingOverlay component for smooth language transitions - Update all translation files (en-US, zh-CN, zh-TW) with improved terminology - Optimize Docker configuration for better performance - Update version check and config handling for i18n support - Fix route handling for language-specific content - Add comprehensive task documentation
- fix: add missing id, name, autocomplete attributes to dialog inputs - fix: add aria labels and DialogDescription for accessibility - fix: resolve uncontrolled component warning in SettingsForm - fix: correct duplicate 'Traditional Chinese' label in zh-TW locale - feat: add i18n support for podcast template names - chore: fix lint errors in Dialogs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
21 issues found across 123 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/layout/__tests__/AppSidebar.test.tsx">
<violation number="1" location="frontend/src/components/layout/__tests__/AppSidebar.test.tsx:45">
P1: Test is incomplete - clicks the toggle button but never asserts that `toggleCollapse` was called. Add an assertion like `expect(toggleCollapse).toHaveBeenCalled()` after the click event.</violation>
<violation number="2" location="frontend/src/components/layout/__tests__/AppSidebar.test.tsx:62">
P2: Incorrect text pattern - the first test uses `/Open Notebook/i` as the app name, but this test checks for `/AppName/i`. The assertion will always pass for the wrong reason. Should check for the actual app name.</violation>
</file>
<file name="frontend/src/components/common/ConnectionGuard.tsx">
<violation number="1" location="frontend/src/components/common/ConnectionGuard.tsx:20">
P1: Re-entry prevention logic is ineffective. The condition guards only the state update, but the function continues executing all async operations regardless. To actually prevent re-entry, add an early return when already checking.</violation>
</file>
<file name="frontend/src/app/(dashboard)/sources/page.tsx">
<violation number="1" location="frontend/src/app/(dashboard)/sources/page.tsx:280">
P2: String splitting by `.` will fail for Chinese locales which use `。` (full-width period). This will display the full description instead of just the first sentence. Consider adding a separate translation key for the short description (e.g., `t.sources.noSourcesShortDesc`) rather than manipulating translated strings.</violation>
</file>
<file name="frontend/src/app/(dashboard)/transformations/components/TransformationPlayground.tsx">
<violation number="1" location="frontend/src/app/(dashboard)/transformations/components/TransformationPlayground.tsx:60">
P2: Using `.replace('s', '')` to derive singular form from a translation is an i18n anti-pattern. In Chinese, `navigation.transformations` = "转换" has no 's' to remove. Create a dedicated translation key like `t.transformations.singular` or `t.navigation.transformation` for the singular form.</violation>
<violation number="2" location="frontend/src/app/(dashboard)/transformations/components/TransformationPlayground.tsx:81">
P2: Using `t.settings.embeddingOptionPlaceholder` ("Select embedding option") is semantically incorrect here since this is a language model selector (`modelType="language"`). Consider creating a more generic translation key like `t.transformations.selectModel` or use a model-specific placeholder.</violation>
</file>
<file name="frontend/src/components/common/CommandPalette.tsx">
<violation number="1" location="frontend/src/components/common/CommandPalette.tsx:47">
P2: Concatenating translated strings is an i18n anti-pattern. In Chinese, this will produce "新建 来源" with an unnatural space. Use dedicated translation keys like `common.newSource`, `common.newNotebook`, `common.newPodcast` that can be properly localized for each language.</violation>
</file>
<file name="frontend/next.config.ts">
<violation number="1" location="frontend/next.config.ts:7">
P1: Disabling ESLint and TypeScript error checks during builds is a dangerous practice that allows broken code to be deployed. These flags mask errors rather than fix them, which contradicts the PR's stated goal of fixing issues.
If there are legitimate build errors, they should be fixed at the source. If certain rules need temporary exceptions, use inline comments (e.g., `// eslint-disable-next-line`) for specific cases rather than project-wide suppression.
Please remove these flags and fix any underlying ESLint/TypeScript errors instead.</violation>
</file>
<file name="frontend/src/components/podcasts/SpeakerProfilesPanel.tsx">
<violation number="1" location="frontend/src/components/podcasts/SpeakerProfilesPanel.tsx:109">
P2: Pluralization is lost: will show "Used by 1 episodes" instead of "Used by 1 episode". Consider using react-i18next's built-in pluralization with `_one`/`_other` suffixes, or add separate singular/plural translation keys.</violation>
</file>
<file name="frontend/src/components/common/LanguageLoadingOverlay.tsx">
<violation number="1" location="frontend/src/components/common/LanguageLoadingOverlay.tsx:45">
P2: Hardcoded English string in an i18n component. The "Loading..." text should be translated using the `t` function from the `useTranslation` hook that's already imported.</violation>
</file>
<file name="frontend/src/components/podcasts/GeneratePodcastDialog.tsx">
<violation number="1" location="frontend/src/components/podcasts/GeneratePodcastDialog.tsx:733">
P2: Locale handling for `zh-TW` is incorrect. Traditional Chinese users will see English date formatting instead of Chinese. The ternary only checks for `zh-CN`, causing `zh-TW` to fall through to `en-US`.</violation>
</file>
<file name="frontend/src/app/(dashboard)/notebooks/components/NotebookHeader.tsx">
<violation number="1" location="frontend/src/app/(dashboard)/notebooks/components/NotebookHeader.tsx:115">
P1: The `.replace('{time}', ...)` calls will have no effect because the translation strings `t.common.created` and `t.common.updated` don't contain a `{time}` placeholder. The locale file has `created: "Created"` and `updated: "Updated"` without placeholders, so the formatted time will not be displayed.
The translation strings should be updated to include the placeholder, e.g., `created: "Created {time}"` and `updated: "Updated {time}"`.</violation>
</file>
<file name="frontend/src/app/(dashboard)/notebooks/components/SourcesColumn.tsx">
<violation number="1" location="frontend/src/app/(dashboard)/notebooks/components/SourcesColumn.tsx:165">
P2: Incorrect translation key used: the dropdown menu item was originally 'Add New Source' but now uses the same key as the button ('Add Source'). Use `t.sources.addNew` instead to preserve the original distinction.</violation>
</file>
<file name="frontend/src/app/(dashboard)/transformations/components/TransformationCard.tsx">
<violation number="1" location="frontend/src/app/(dashboard)/transformations/components/TransformationCard.tsx:91">
P1: Runtime error: `t.notebooks.addDescription` is undefined in the English locale, causing `.replace()` to crash. The `addDescription` key only exists in zh-CN and zh-TW locales. Use the existing `t.common.description` key instead.</violation>
</file>
<file name="frontend/src/app/(dashboard)/notebooks/components/NotebookCard.tsx">
<violation number="1" location="frontend/src/app/(dashboard)/notebooks/components/NotebookCard.tsx:64">
P1: Translation keys `t.notebooks.archived`, `t.notebooks.archive`, `t.notebooks.unarchive`, `t.notebooks.deleteNotebook`, and `t.notebooks.deleteNotebookDesc` do not exist in the locale files. The notebooks section only defines `title`, `newNotebook`, and `searchPlaceholder`. These missing keys need to be added to all locale files.</violation>
</file>
<file name="frontend/src/app/(dashboard)/notebooks/page.tsx">
<violation number="1" location="frontend/src/app/(dashboard)/notebooks/page.tsx:69">
P2: Missing translation key `searchNotebooks` in accessibility object causes fallback to hardcoded English string for all locales. Consider adding this key to all locale files or using an existing translation key like `t.notebooks.searchPlaceholder`.</violation>
</file>
<file name="frontend/src/components/source/ChatPanel.tsx">
<violation number="1" location="frontend/src/components/source/ChatPanel.tsx:248">
P2: Lost pluralization handling. When `sources.length` is 1, this will display "1 Sources" instead of "1 source". Consider using a pluralization helper or separate translation keys for singular/plural forms.</violation>
<violation number="2" location="frontend/src/components/source/ChatPanel.tsx:254">
P2: Lost pluralization handling. When `insights.length` is 1, this will display "1 Insights" instead of "1 insight". Consider using a pluralization helper or separate translation keys for singular/plural forms.</violation>
<violation number="3" location="frontend/src/components/source/ChatPanel.tsx:260">
P2: Lost pluralization handling. When `notes.length` is 1, this will display "1 Notes" instead of "1 note". Consider using a pluralization helper or separate translation keys for singular/plural forms.</violation>
</file>
<file name="frontend/src/components/source/MessageActions.tsx">
<violation number="1" location="frontend/src/components/source/MessageActions.tsx:23">
P2: Using generic `t.common.error` ("Error") instead of specific error messages loses important context for users. The original message "Cannot save note: notebook ID not available" was much more informative. Consider creating a specific translation key like `t.sources.cannotSaveNoteNoNotebook` to preserve the helpful error context.</violation>
</file>
<file name="frontend/src/app/(dashboard)/advanced/components/SystemInfo.tsx">
<violation number="1" location="frontend/src/app/(dashboard)/advanced/components/SystemInfo.tsx:69">
P2: Translation key `t.advanced.updateAvailable` expects a `{version}` placeholder but is used without interpolation. This will display "Version {version} Available" literally. Use `.replace('{version}', config.latestVersion)` to substitute the actual version.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
frontend/src/app/(dashboard)/transformations/components/TransformationPlayground.tsx
Outdated
Show resolved
Hide resolved
frontend/src/app/(dashboard)/advanced/components/SystemInfo.tsx
Outdated
Show resolved
Hide resolved
Configuration:
- Remove ignoreDuringBuilds flags from next.config.ts
Testing:
- Fix AppSidebar.test.tsx regex pattern and add missing assertion
Logic:
- Fix ConnectionGuard.tsx re-entry prevention logic
Internationalization (I18n) - Translations:
- Add missing keys: notebooks.archived, common.note/insight, accessibility keys
- Add specific keys: sources.allSourcesDescShort, transformations.selectModel
- Add singular/plural keys: podcasts.usedByCount_one/other, common.note/notes
- Add common.created/updated with {time} placeholder
Internationalization (I18n) - Usage:
- SourcesPage: use allSourcesDescShort instead of string splitting
- TransformationPlayground: use navigation.transformation and selectModel
- CommandPalette: use dedicated keys instead of string concatenation
- GeneratePodcastDialog: fix zh-TW date locale handling
- NotebookHeader: correctly interpolate {time} placeholder
- TransformationCard: use common.description instead of undefined key
- ChatPanel/SpeakerProfilesPanel: implement proper pluralization
- SystemInfo: correctly interpolate {version} placeholder
- LanguageLoadingOverlay: use t.common.loading instead of hardcoded string
- MessageActions: use specific error key cannotSaveNoteNoNotebook
Other:
- Fix SessionManager.tsx exhaustive-deps warning
Contributor
There was a problem hiding this comment.
10 issues found across 17 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/lib/locales/en-US/index.ts">
<violation number="1" location="frontend/src/lib/locales/en-US/index.ts:16">
P1: Duplicate `loading` key in `common` object. The second occurrence on line 59 will overwrite this value. Either remove this line or consolidate the two values.</violation>
<violation number="2" location="frontend/src/lib/locales/en-US/index.ts:268">
P2: Duplicate `addNew` key in `sources` object. This key already exists earlier in the same object (line 244). Remove this duplicate entry.</violation>
</file>
<file name="frontend/src/lib/locales/zh-CN/index.ts">
<violation number="1" location="frontend/src/lib/locales/zh-CN/index.ts:27">
P1: Duplicate key `loading` in the `common` object. This key was already added earlier in this same PR. The second occurrence will override the first. Remove this duplicate block (lines with `loading`, `note`, `insight`, `newSource`, `newNotebook`, `newPodcast`).</violation>
<violation number="2" location="frontend/src/lib/locales/zh-CN/index.ts:122">
P2: Duplicate key `searchNotebooks` in the `accessibility` object. This key was already added earlier in the same object. Remove this duplicate entry.</violation>
<violation number="3" location="frontend/src/lib/locales/zh-CN/index.ts:229">
P2: Duplicate key `transformation` in the `navigation` object. This key was already added earlier in the same object. Remove this duplicate entry.</violation>
<violation number="4" location="frontend/src/lib/locales/zh-CN/index.ts:283">
P2: Duplicate key `addNew` in the `sources` object. This key already exists earlier in the object. Remove this duplicate entry.</violation>
</file>
<file name="frontend/src/lib/locales/zh-TW/index.ts">
<violation number="1" location="frontend/src/lib/locales/zh-TW/index.ts:27">
P1: Duplicate keys in `common` object: `loading`, `note`, `insight`, `newSource`, `newNotebook`, `newPodcast` are defined twice. In JavaScript objects, duplicate keys cause the later value to silently override the earlier one. This appears to be a copy-paste error. Remove these 6 duplicate lines.</violation>
<violation number="2" location="frontend/src/lib/locales/zh-TW/index.ts:122">
P2: Duplicate key `searchNotebooks` in `common.accessibility` object. This key is defined twice within the same object block. Remove this duplicate line.</violation>
<violation number="3" location="frontend/src/lib/locales/zh-TW/index.ts:282">
P2: Duplicate key `addNew` in `sources` object. This key already exists earlier in the same object. Remove this duplicate line.</violation>
</file>
<file name="frontend/src/components/podcasts/SpeakerProfilesPanel.tsx">
<violation number="1" location="frontend/src/components/podcasts/SpeakerProfilesPanel.tsx:109">
P1: Missing translation keys `usedByCount_one` and `usedByCount_other` in zh-CN locale. Simplified Chinese users will see untranslated key paths like "podcasts.usedByCount_one" instead of proper translations. Add these keys to the zh-CN locale file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| add: "添加来源", | ||
| addNew: "添加新来源", | ||
| addExisting: "添加现有来源", | ||
| addNew: "添加新来源", |
Contributor
There was a problem hiding this comment.
P2: Duplicate key addNew in the sources object. This key already exists earlier in the object. Remove this duplicate entry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/lib/locales/zh-CN/index.ts, line 283:
<comment>Duplicate key `addNew` in the `sources` object. This key already exists earlier in the object. Remove this duplicate entry.</comment>
<file context>
@@ -260,6 +280,9 @@ export const zhCN = {
add: "添加来源",
addNew: "添加新来源",
addExisting: "添加现有来源",
+ addNew: "添加新来源",
+ allSourcesDescShort: "在此查看所有来源。",
+ cannotSaveNoteNoNotebook: "无法保存笔记：缺少笔记本 ID",
</file context>
✅ Addressed in 93146c7
| navigation: "导航", | ||
| transformationViews: "转换视图", | ||
| searchKB: "向知识库提问或搜索", | ||
| searchNotebooks: "搜索笔记本", |
Contributor
There was a problem hiding this comment.
P2: Duplicate key searchNotebooks in the accessibility object. This key was already added earlier in the same object. Remove this duplicate entry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/lib/locales/zh-CN/index.ts, line 122:
<comment>Duplicate key `searchNotebooks` in the `accessibility` object. This key was already added earlier in the same object. Remove this duplicate entry.</comment>
<file context>
@@ -108,13 +119,15 @@ export const zhCN = {
navigation: "导航",
transformationViews: "转换视图",
searchKB: "向知识库提问或搜索",
+ searchNotebooks: "搜索笔记本",
enterQuestion: "输入您的问题以询问知识库",
enterSearch: "输入搜索词",
</file context>
✅ Addressed in 93146c7
| {usageCount > 0 | ||
| ? `Used by ${usageCount} episode${usageCount === 1 ? '' : 's'}` | ||
| : 'Unused'} | ||
| ? (usageCount === 1 ? t.podcasts.usedByCount_one : t.podcasts.usedByCount_other.replace('{count}', usageCount.toString())) |
Contributor
There was a problem hiding this comment.
P1: Missing translation keys usedByCount_one and usedByCount_other in zh-CN locale. Simplified Chinese users will see untranslated key paths like "podcasts.usedByCount_one" instead of proper translations. Add these keys to the zh-CN locale file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/podcasts/SpeakerProfilesPanel.tsx, line 109:
<comment>Missing translation keys `usedByCount_one` and `usedByCount_other` in zh-CN locale. Simplified Chinese users will see untranslated key paths like "podcasts.usedByCount_one" instead of proper translations. Add these keys to the zh-CN locale file.</comment>
<file context>
@@ -106,7 +106,7 @@ export function SpeakerProfilesPanel({
>
{usageCount > 0
- ? t.podcasts.usedByCount.replace('{count}', usageCount.toString())
+ ? (usageCount === 1 ? t.podcasts.usedByCount_one : t.podcasts.usedByCount_other.replace('{count}', usageCount.toString()))
: t.podcasts.unused}
</Badge>
</file context>
- en-US: remove duplicate loading key (line 59) and addNew key (sources) - zh-CN: remove duplicate common keys (loading, note, insight, newSource, newNotebook, newPodcast) - zh-CN: remove duplicate accessibility.searchNotebooks key - zh-CN: remove duplicate sources.addNew key - zh-CN: remove duplicate navigation.transformation key - zh-CN: add missing usedByCount_one and usedByCount_other keys in podcasts - zh-TW: remove duplicate common keys (loading, note, insight, newSource, newNotebook, newPodcast) - zh-TW: remove duplicate accessibility.searchNotebooks key - zh-TW: remove duplicate sources.addNew key
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="info.md">
<violation number="1" location="info.md:1">
P1: This file appears to be an accidentally committed GitHub email notification. It contains email headers, bot review comments about other files, and email addresses. This file should be removed from the PR - it's not actual documentation and exposes email addresses like `[email protected]`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- zh-CN: remove duplicate notebooks keys (archived, archive, unarchive, deleteNotebook, deleteNotebookDesc) - zh-TW: remove duplicate notebooks keys (archived, archive, unarchive, deleteNotebook, deleteNotebookDesc) - GeneratePodcastDialog: remove unused @ts-expect-error directive
- Replace <Label> with role='group' + aria-labelledby for search type section - Replace <Label> with role='group' + aria-labelledby for search in section - Follows WAI-ARIA best practices for labeling form field groups
- search/page.tsx: use role='group' + aria-labelledby for search type and search in sections - RebuildEmbeddings.tsx: use role='group' + aria-labelledby for include checkboxes - TransformationPlayground.tsx: replace Label with span for non-form output label
Owner
|
@MisonL wow.. this is such a great work. I am really excited to push this change to production, but we had a lot of new pushes to the codebase after the PR was created. Do you mind refreshing your branch with the latest main version and going through the conflicts so we can test and approve this PR? I promise to be quick once you do it :) |
…ts-docker # Conflicts: # Dockerfile # frontend/next.config.ts # frontend/package-lock.json # frontend/src/app/(dashboard)/notebooks/components/NotebookList.tsx # frontend/src/app/(dashboard)/notebooks/page.tsx # frontend/src/components/common/AddButton.tsx # frontend/src/components/sources/steps/SourceTypeStep.tsx # frontend/tsconfig.json
… resource loading
Contributor
There was a problem hiding this comment.
37 issues found across 275 files (changes from recent commits).
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="CLAUDE.md">
<violation number="1">
P2: Broken documentation link: `frontend/CLAUDE.md` does not exist (file is under `frontend/src/CLAUDE.md`).</violation>
</file>
<file name="docs/3-USER-GUIDE/adding-sources.md">
<violation number="1">
P2: Conflicting documentation on when embeddings complete vs readiness, misinforming users about semantic search availability.</violation>
</file>
<file name="api/routers/podcasts.py">
<violation number="1" location="api/routers/podcasts.py:232">
P2: Error response leaks internal exception details to clients via `detail=f"...{str(e)}"`.</violation>
</file>
<file name="docs/1-INSTALLATION/from-source.md">
<violation number="1">
P2: Unpinned dependency installed outside uv lock: `python-magic` is not in pyproject/uv.lock, so manual `uv pip install` makes environments drift from the locked set after `uv sync`.</violation>
</file>
<file name=".github/workflows/build-dev.yml">
<violation number="1">
P2: Arm64/multi-arch builds are exposed but the workflow never installs QEMU/binfmt; on amd64 GitHub runners the arm64 build path will fail when RUN steps execute.</violation>
<violation number="2">
P2: CI no longer builds/tests Dockerfile.single even though the single-container image is still produced elsewhere (Makefile, docker-compose.single.yml), so coverage for that supported artifact was removed.</violation>
</file>
<file name="docs/3-USER-GUIDE/chat-effectively.md">
<violation number="1">
P3: Documentation uses inconsistent terminology for the third context level (Not in Context vs Excluded/Not included); align to one label to avoid user confusion.</violation>
<violation number="2">
P2: Hard-coded model pricing is inaccurate/stale relative to provider pricing (misleads users on costs)</violation>
</file>
<file name="api/auth.py">
<violation number="1" location="api/auth.py:80">
P2: `check_api_password` cannot be used as a FastAPI dependency as documented: credentials are never injected, so it will always 401 when a password is set.</violation>
</file>
<file name="api/routers/speaker_profiles.py">
<violation number="1" location="api/routers/speaker_profiles.py:42">
P2: HTTP 500 responses include raw exception text in the response body, leaking internal error details.</violation>
</file>
<file name="docs/3-USER-GUIDE/index.md">
<violation number="1">
P3: Heading says eight core features but only seven feature sections (1–7) are listed, leading to inconsistent documentation/missing section.</violation>
</file>
<file name="docs/3-USER-GUIDE/citations.md">
<violation number="1">
P2: Documentation incorrectly claims every AI response always includes citations, contradicting later sections that acknowledge responses can lack citations.</violation>
</file>
<file name="commands/CLAUDE.md">
<violation number="1">
P2: Documentation misstates retry behavior; retries also occur on ConnectionError/TimeoutError, not just RuntimeError.</violation>
<violation number="2">
P2: Retry policy documented inconsistently (max 5× vs 15 attempts) within the same file.</violation>
</file>
<file name="api/routers/commands.py">
<violation number="1" location="api/routers/commands.py:70">
P2: HTTP 500 responses expose raw exception messages via `str(e)`, risking information disclosure.</violation>
</file>
<file name="api/routers/episode_profiles.py">
<violation number="1" location="api/routers/episode_profiles.py:50">
P1: Internal exception details are returned in the 500 response, potentially leaking sensitive information.</violation>
</file>
<file name=".github/workflows/claude.yml">
<violation number="1">
P2: Third-party action is referenced by mutable tag (`@v1`) instead of a pinned commit SHA, creating supply-chain risk for a workflow using secrets.</violation>
</file>
<file name="api/routers/insights.py">
<violation number="1" location="api/routers/insights.py:82">
P1: 500 error response leaks raw exception text to clients via detail, risking information disclosure</violation>
</file>
<file name="docs/1-INSTALLATION/index.md">
<violation number="1">
P2: Production deployment links point to non-existent docs paths (`docs/deployment/...`), resulting in dead links.</violation>
</file>
<file name="README.dev.md">
<violation number="1">
P2: SURREAL_URL example omits required /rpc path; documented value will cause SurrealDB connection failures.</violation>
</file>
<file name="docs/0-START-HERE/quick-start-openai.md">
<violation number="1">
P2: Troubleshooting instructs restarting non-existent compose service `api` instead of the defined `open_notebook` service.</violation>
<violation number="2">
P1: Insecure SurrealDB quick-start: weak default credentials and binding to all interfaces with published port, exposing DB to host/LAN.</violation>
</file>
<file name="docs/0-START-HERE/quick-start-cloud.md">
<violation number="1">
P2: Troubleshooting instructs restarting nonexistent docker service `api`; compose defines `open_notebook` instead</violation>
</file>
<file name="api/routers/settings.py">
<violation number="1" location="api/routers/settings.py:27">
P2: 500 error response returns raw exception text, potentially leaking internal details</violation>
<violation number="2" location="api/routers/settings.py:88">
P2: 500 error response returns raw exception text, potentially leaking internal details</violation>
</file>
<file name="README.md">
<violation number="1">
P2: Citations capability description is inconsistent (table says basic references while later sections claim full citations/enhanced citations completed).</violation>
<violation number="2">
P2: README link to docs/0-START-HERE/first-notebook.md is broken because the target file is missing</violation>
<violation number="3">
P2: README links to docs/0-START-HERE/quick-start.md, but that file does not exist (link will 404)</violation>
</file>
<file name="docs/1-INSTALLATION/docker-compose.md">
<violation number="1">
P2: Documentation commands reference a non-existent `api` service; compose file defines `open_notebook`, so log/restart instructions will fail.</violation>
<violation number="2">
P2: Hardcoded container name likely wrong; use `docker compose exec ollama …` to avoid failure</violation>
</file>
<file name="docs/2-CORE-CONCEPTS/chat-vs-transformations.md">
<violation number="1">
P2: Documentation incorrectly claims transformations are single-source only, contradicting existing batch support documented elsewhere.</violation>
</file>
<file name="docs/0-START-HERE/quick-start-local.md">
<violation number="1">
P2: Model pull commands use a hard-coded container name that doesn’t match the default Compose name, causing `docker exec` to fail for most users.</violation>
<violation number="2">
P1: Documentation instructs running SurrealDB with default root/password exposed on 0.0.0.0:8000 and published to the host, enabling trivial remote access especially in the advertised remote-server setup.</violation>
</file>
<file name="api/main.py">
<violation number="1">
P2: Custom HTTPException handler discards exc.headers, stripping required error headers (e.g., WWW-Authenticate, Retry-After). Merge exc.headers instead of overwriting.</violation>
</file>
<file name="api/routers/config.py">
<violation number="1" location="api/routers/config.py:70">
P2: Async /config endpoint performs blocking requests.get during version check, potentially freezing the event loop for up to 10s on cache miss.</violation>
</file>
<file name="Dockerfile">
<violation number="1">
P2: Runtime image copies entire builder `/app`, bringing frontend node_modules/build artifacts and duplicating later targeted frontend copies, inflating image size and risk of overlay conflicts.</violation>
<violation number="2">
P2: `COPY . /app` can overwrite the freshly built /app/.venv because .dockerignore is missing; host .venv or other build artifacts will be copied into the image, making builds non-deterministic.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| # Install uv using the official method | ||
| COPY --from=ghcr.io/astral-sh/uv:latest /uv /uvx /bin/ | ||
|
|
Contributor
There was a problem hiding this comment.
P2: COPY . /app can overwrite the freshly built /app/.venv because .dockerignore is missing; host .venv or other build artifacts will be copied into the image, making builds non-deterministic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Dockerfile, line 36:
<comment>`COPY . /app` can overwrite the freshly built /app/.venv because .dockerignore is missing; host .venv or other build artifacts will be copied into the image, making builds non-deterministic.</comment>
<file context>
@@ -1,36 +1,57 @@
-# Stage 3: Runtime
+# Copy the rest of the application code
+COPY . /app
+
+# Install frontend dependencies and build
</file context>
Contributor
Author
Comprehensive Improvements: i18n, Accessibility, and Backend RemediationThis PR provides a full internationalization (i18n) framework, extensive accessibility (A11y) fixes, and critical architectural remediations based on recent code review findings. Key Changes1. Internationalization (i18n) & Localization
2. Accessibility (A11y) & UI Consistency
3. Backend Architecture & Security (PR Remediations)
4. DevOps, Build & Testing
Verification Status
|
lfnovo
previously requested changes
Jan 15, 2026
Owner
lfnovo
left a comment
lfnovo
left a comment
There was a problem hiding this comment.
This is an absolutely amazing contribution. I am very grateful for your help. I made some comments here that should be pretty straightforward to address. Can we make these changes?
Owner
There was a problem hiding this comment.
This variable should be inside docker.env
|
|
||
| # Install only runtime system dependencies (no build tools) | ||
| # Add Node.js 20.x LTS for running frontend | ||
| RUN apt-get update && apt-get upgrade -y && apt-get install -y \ |
Owner
There was a problem hiding this comment.
I am concerned that removing upgrade could expose the app to vulnerability. Why did you propose this? Any special reason?
Dockerfile.single
Outdated
| # Stage 2: Backend Builder | ||
| FROM python:3.12-slim-bookworm AS backend-builder | ||
| # Install build dependencies | ||
| RUN apt-get update && apt-get install -y --no-install-recommends build-essential && rm -rf /var/lib/apt/lists/* |
Owner
There was a problem hiding this comment.
I am concerned that removing upgrade could expose the app to vulnerability. Why did you propose this? Any special reason?
frontend/src/app/(dashboard)/notebooks/components/NotebookCard.tsx
Outdated
Show resolved
Hide resolved
Owner
There was a problem hiding this comment.
Mock of useTranslation is different from the one used in ConfirmDialog.test.tsx - inconsistency. Risk: Global mocks can mask bugs (tests pass but real code fails)
Owner
There was a problem hiding this comment.
Suggestion: simplify to just one effect.
export function useVersionCheck() {
const { t } = useTranslation()
const hasChecked = useRef(false)
useEffect(() => {
if (hasChecked.current) return
hasChecked.current = true
getConfig()
.then(config => {
if (!config.hasUpdate || !config.latestVersion) return
const dismissKey = `version_notification_dismissed_${config.latestVersion}`
if (sessionStorage.getItem(dismissKey)) return
toast.info(t.advanced.updateAvailable.replace('{version}', config.latestVersion), {
description: t.advanced.updateAvailableDesc,
duration: Infinity,
closeButton: true,
action: {
label: t.advanced.viewOnGithub,
onClick: () => window.open('https://github.com/lfnovo/open-notebook', '_blank'),
},
onDismiss: () => sessionStorage.setItem(dismissKey, 'true'),
})
})
.catch(() => {}) // Silently fail
}, [t]) // t ainda é dependência mas só executa uma vez pelo ref guard
}
Benefits:
- A single useEffect, linear flow easy to follow
- No need for state (useState) - less re-renders
- hasChecked.current guarantees single execution even if t changes
- Less code (~30 lines vs ~50 lines of PR)
Accepted trade-off: Toast does not update language in real time (not a real problem).
- Docker: consolidate SURREAL_URL to docker.env, add single-container override - Security: restore apt-get upgrade in Dockerfile and Dockerfile.single - Create centralized getDateLocale helper (lib/utils/date-locale.ts) - Refactor 7 files to use getDateLocale helper - Revert config/route.ts to origin/main version - Move test files to co-located pattern (3 files) - Remove local useTranslation mock from ConfirmDialog.test.tsx - Simplify use-version-check to single useEffect pattern - Fix test import paths after moving to co-located pattern
- Add apt-get upgrade -y to Dockerfile.single backend-builder stage - Refactor ChatColumn.test.tsx: use 'as unknown as ReturnType<typeof hook>' instead of 'as any' - Use toBeInTheDocument() assertions instead of toBeDefined()
Contributor
Author
PR #371 Review Feedback - Changes Summary1. Docker Environment Consolidation ✅
2. Security: Restored
|
Contributor
Author
|
Note on |
Owner
|
Thank you so much @MisonL .. this was an amazing addition to the project |
3 tasks
Owner
|
@MisonL do you have a Reddit account? I'd love to mention you there if you would like to be mentioned |
Contributor
Author
|
Thank you so much! I really saw the potential of this project and wanted my friends—who aren't as comfortable with English—to be able to enjoy a smooth experience. That's what motivated me to contribute this PR. I should also give credit to Antigravity and Codex, which were incredibly helpful throughout the process. Oh, and I don't have a Reddit account, so no need to mention me there—haha! 😄 Thanks again for the warm welcome! 🙏 |
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.
Description
This PR addresses multiple frontend issues related to Accessibility, Internationalization (i18n), and UI consistency.
Key Changes:
id,name, andautoCompleteattributes to various input fields (Transformation Dialog, Notebook Dialog, etc.) to resolve accessibility warnings and improve screen reader support.MarkdownEditorcomponent by introducing atextareaIdprop.DialogDescriptionto dialogs to satisfy Radix UI accessibility requirements.podcastProfilesin all locale files (en-US,zh-CN,zh-TW).GeneratePodcastDialogto use these translated strings.zh-TW(Correctedcommon.chineseto "简体中文").SettingsFormby correctly initializing state.DialogDescriptionimport,@ts-expect-errorusage).Related Issue
Fixes # (Please insert issue number if available, otherwise delete)
Type of Change
How Has This Been Tested?
bun run lintpassed)Test Details:
SettingsFormno longer throws React state warnings.next build.Design Alignment
Which design principles does this PR support?
Checklist
Code Quality
Testing
bun run lint(Equivalent tomake rufffor frontend)Documentation
Database Changes
Breaking Changes
Pre-Submission Verification