chore: align service shells + shared @tale/webui utilities#1701
Closed
yannickmonney wants to merge 2 commits into
Closed
chore: align service shells + shared @tale/webui utilities#1701yannickmonney wants to merge 2 commits into
yannickmonney wants to merge 2 commits into
Conversation
Cross-service alignment + bundle of pre-existing WIP. Reduces boilerplate across services/platform, services/web, services/docs and pulls common patterns into @tale/ui and @tale/webui. Service-shell alignment - One-liner services/*/app/globals.css (all three import @tale/webui/globals.css) - Service-local CSS moved to sibling app/locals.css imported alongside globals - main.tsx files share the same shape (router import, providers around RouterProvider, identical root/error string, same import order). Platform's eight-provider stack lifted out of __root.tsx into main.tsx - router.tsx files share the same module-level export pattern; platform's QueryClient + ConvexQueryClient construction lifted to module scope - tailwind.config.ts, postcss.config.mjs, tsr.config.json, tsconfig.json byte-identical across the three services - services/*/lib/i18n/i18n.ts collapsed via new initServiceI18n helper Shared utilities pulled into packages - @tale/ui/globals.css is the canonical stylesheet; tokens, base layer, animations, json-diff-kit and react-flow overrides all live here - @fontsource/inter bundled at @tale/ui (one source of truth for the body font); removed per-service @fontsource imports - @tale/ui/i18n/init-service composes initI18n + collectRegionalBundles - @tale/webui/server exports startSimpleServer used by services/web and services/docs; covers locale negotiation, /api/health (with optional graceful-drain marker), shared security headers via defaultSimpleSecurityHeaders, and an extraRoutes hook for service-only endpoints (web's Discord form proxy, etc.). Platform's Hono-based server keeps its specifics (CSP nonce injection, file-events SSE, branding, canvas preview) but its /api/health body shape was aligned to match Other - components.json moved to repo root; aliases now point at packages/ui - Plop templates updated to emit the aligned shape (locals.css, router.tsx with declare-module block, initServiceI18n-based i18n.ts, identical tsconfig/tsr/tailwind/postcss) - Pre-existing WIP bundled in: webui search test suite, rehype-preserve- code-meta plugin, theme-asset-sync component + ThemeAssets refactor, storybook cleanup, favicon rename, translation edits Pre-PR checklist - [x] Ran \`bun run check\` (format, lint, typecheck, all tests) - [x] Updated services/platform/messages/{en,de,fr}.json — N/A (no user-facing platform copy changed) - [x] Updated /docs/{en,de,fr}/ for every user-visible change — translation edits bundled - [x] Ran \`bun run --filter @tale/docs lint\` and \`bun run --filter @tale/docs test\` — covered by \`bun run check\` - [x] Updated README.md, README.de.md, README.fr.md — N/A
Contributor
📝 WalkthroughWalkthroughUnifies global styling by importing shared globals.css across services, introduces ThemeAssetSync, and switches Tailwind presets. Refactors i18n initialization to initServiceI18n. Overhauls docs search: index options, markdown stripping, reranking, UI behaviors, and extensive tests. Adds a shared Bun SPA server utility and adopts it in docs/web servers. Updates Storybook/Vitest configs (multi-project, addon-vitest), router setup in platform, and scaffolding templates. Minor docs localization/formatting tweaks. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/docs/app/components/docs/docs-toc.tsx (1)
95-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit list semantics to TOC
<ul>.Please add
role="list"to the<ul>so VoiceOver/Safari consistently exposes list semantics under Tailwind Preflight.Suggested patch
- <ul className="flex flex-col"> + <ul role="list" className="flex flex-col">Based on learnings: “do not remove the role="list" attribute from ul elements… Safari/VoiceOver requires an explicit role="list" for proper semantics.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/docs/app/components/docs/docs-toc.tsx` at line 95, The TOC unordered list element (<ul className="flex flex-col">) in the docs-toc.tsx component is missing explicit list semantics; update that <ul> (in the Docs TOC component) to include role="list" so VoiceOver/Safari expose list semantics consistently under Tailwind Preflight—add the role attribute to the existing <ul className="flex flex-col"> element.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/globals.css`:
- Around line 263-264: Normalize the CSS to satisfy stylelint by removing the
unnecessary quotes around the Inter family and lowercasing the text-rendering
keyword: change font-family: 'Inter', ui-sans-serif, system-ui, sans-serif; to
font-family: Inter, ui-sans-serif, system-ui, sans-serif; and change
text-rendering: optimizeLegibility; to text-rendering: optimizelegibility;
(update these in globals.css where the font-family and text-rendering
declarations appear).
In `@packages/ui/src/i18n/init-service.ts`:
- Line 4: Change the Bundle type from using unknown to a concrete allowed type
(e.g., type Bundle = Record<string, Record<string, string>>) so public typings
do not use unknown, and add an explicit return type annotation to the exported
initializer function in this module (the exported initializer API) to fully
annotate its public signature; update any internal places that rely on Bundle or
the initializer return type to satisfy the new stricter types.
In `@packages/ui/src/markdown/components/code-group.tsx`:
- Around line 145-155: The extractCode function currently returns an empty
string when it can't find code which leads HighlightedCode to render an empty
panel; change extractCode (and its usages) to return null (or undefined) instead
of '' so callers can skip rendering the panel, and update the caller that
renders HighlightedCode to check for a null/undefined result and omit the
HighlightedCode panel (or render a clear placeholder). Specifically, modify
extractCode (referenced here) to return null when no code element is found (use
findCodeElement and stringifyChildren as before), and update the component that
consumes extractCode/HighlightedCode to guard on the returned value and not
render the HighlightedCode UI when it's null.
- Around line 180-197: The extractFilename function is using the raw data-meta
string as the tab label which surfaces noisy attributes like `{1,3-5}` or
`title="server.ts" showLineNumbers`; update extractFilename (and the spot where
it reads dataMeta via findCodeElement) to first detect attribute-shaped metas
and only accept simple single-token labels: if dataMeta contains braces `{`, an
equals `=`, or whitespace-separated tokens, attempt to extract a title attribute
by matching title="..."/title='...' and return that value trimmed; if no title
is present, bail out and return undefined so only pure simple labels (e.g.,
"Python") are used as tab text.
In `@packages/webui/package.json`:
- Around line 7-10: The package exports "./server" which exposes ./src/server.ts
that imports from `@tale/i18n` (e.g., its cookie and negotiate modules), but
`@tale/i18n` is not declared—add `@tale/i18n` to the package.json dependencies (not
devDependencies) so consumers installing `@tale/webui` get the required runtime
package; ensure you pick the correct semver version used by the repo, update
package.json "dependencies" to include "@tale/i18n": "<appropriate-version>",
and reinstall/lock to validate the fix.
In `@packages/webui/src/search/client.ts`:
- Around line 146-150: The slug bonus currently uses substring matching
(lowerUrl.includes(t)) which causes false positives; update the logic around
lowerUrl, lowerTokens, slugHit and slugBonus to split the URL path into discrete
slug segments (split on '/' and '-' after stripping protocol/query) and then set
slugHit only if a token exactly matches a segment or appears as a whole slug
component (e.g., segment === token or segment.startsWith(token) depending on
desired strictness), rather than using includes; ensure you compute segments
once, iterate lowerTokens against those segments, and preserve the slugBonus =
slugHit ? 1.25 : 1 assignment.
In `@packages/webui/src/search/dialog.tsx`:
- Around line 132-139: The catch handler for the loadIndex(...) prefetch
currently swallows all errors with console.debug; change it to distinguish
aborts from real failures and log appropriately: in the catch for the promise
returned by loadIndex(locale, baseUrl) detect an AbortError (or error.name ===
'AbortError' / fetch aborted shape) and keep a debug/ignore path for those, but
use console.warn or console.error for other errors so non-abort failures (CORS,
5xx, parse errors) are visible; update the anonymous catch callback that
references loadIndex, locale, baseUrl and replace console.debug with conditional
logging based on the error shape.
In `@packages/webui/src/search/highlight.tsx`:
- Around line 22-26: The term filtering pipeline uses
terms.filter(...).slice().sort(...).map(escapeRegex) but doesn't trim individual
terms, so whitespace-surrounded terms can produce incorrect regexes; update the
pipeline that builds filtered (operating on the variable terms) to trim each
term before filtering and escaping (e.g., apply t = t.trim() in the initial map
or use terms.map(t => t.trim()).filter(...).sort(...).map(escapeRegex)) so that
the variable filtered contains trimmed, escaped terms; reference symbols:
filtered, terms, escapeRegex.
In `@packages/webui/src/search/search-results.tsx`:
- Around line 238-257: The span that renders the breadcrumb currently sets
aria-label={result.url}, which overrides the accessible name (causing screen
readers to read the raw URL); change the span to remove aria-label and instead
add title={result.url} (for hover tooltip) or data-url={result.url} (for
tests/automation) while keeping the visible breadcrumb text intact; update the
test selector in search-results.test.tsx from
[aria-label="/self-hosted/configuration/retention"] to the corresponding
[data-url="/self-hosted/configuration/retention"] (or adapt to title if you
chose title) and ensure the span with className "text-fg-subtle mt-1 flex..." is
the element receiving the data-url/title.
In `@packages/webui/src/server.ts`:
- Around line 223-238: The resolver currently falls back to dist/index.html for
every missing path; change it to return 404 for asset-like requests while
keeping the SPA fallback for extensionless routes: detect whether the incoming
pathname is an asset (e.g., has a file extension via path.extname(pathname) or a
'.' after the last '/'), and if an asset-path candidate (resolved or routeHtml)
does not exist return a 404 Response instead of serving index.html; only when
the pathname is extensionless should you continue to return the SPA shell by
returning file(join(distDir, 'index.html')). Use the existing symbols (resolve,
distDir, distPrefix, file(...), contentTypeFor(pathname), pathname, routeHtml)
to locate and implement this conditional.
In `@services/docs/index.html`:
- Around line 91-93: The catch block that currently swallows all errors during
theme bootstrapping (catch (_e) { /* localStorage unavailable; fall through to
React default */ }) should log the caught error and context before continuing
with the fallback; update that catch to capture the error variable (e or err)
and call console.error or the existing logger with a clear message like "Theme
bootstrap failed, falling back to defaults" plus the error object so regressions
are visible, then allow the function to proceed with the React default behavior.
In `@services/web/lib/i18n/i18n.ts`:
- Around line 8-13: The alias Bundle uses Record<string, Record<string,
unknown>> which uses `unknown` and weakens the glob contract; update the type
used for the regional import to a concrete recursive message-tree type (or
import the exported message-tree type from the shared i18n package) and replace
Bundle with that type where referenced (e.g., the Bundle alias, the
import.meta.glob<Bundle> call and the initServiceI18n bundles object) so the
glob contract is strongly typed without `unknown` (use a recursive shape like a
map of keys to string or nested maps or the shared package's MessageTree type).
In `@tools/plop/templates/react-package/vitest.config.ts.hbs`:
- Around line 1-7: The template currently always imports node:path even though
it's only used when the storybook flag is true (path.dirname(...) in the
storybook block); update the Handlebars template so the import "import path from
'node:path';" is wrapped inside the {{`#if` storybook}}...{{/if}} block to avoid
an unused import when storybook is false, keeping the existing imports for
react, defineConfig, and the conditional storybookTest/playwright imports
unchanged; ensure the path usage (path.dirname(...)) remains inside the same
{{`#if` storybook}} block so there are no dangling references.
---
Outside diff comments:
In `@services/docs/app/components/docs/docs-toc.tsx`:
- Line 95: The TOC unordered list element (<ul className="flex flex-col">) in
the docs-toc.tsx component is missing explicit list semantics; update that <ul>
(in the Docs TOC component) to include role="list" so VoiceOver/Safari expose
list semantics consistently under Tailwind Preflight—add the role attribute to
the existing <ul className="flex flex-col"> element.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08fdf0b9-ea34-4e87-ac02-b3f965798a81
⛔ Files ignored due to path filters (4)
bun.lockis excluded by!**/*.lockservices/docs/public/favicon-dark.pngis excluded by!**/*.pngservices/docs/public/favicon-light.pngis excluded by!**/*.pngservices/docs/public/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (109)
components.jsondocs/de-CH/platform/admin/governance.mddocs/de-CH/platform/chat/attachments.mddocs/de/platform/admin/governance.mddocs/de/platform/chat/attachments.mddocs/de/platform/member/preferences.mddocs/en/platform/chat/attachments.mddocs/en/platform/member/preferences.mddocs/fr/platform/chat/attachments.mddocs/fr/platform/member/preferences.mdpackage.jsonpackages/ui/.storybook/preview.tsxpackages/ui/package.jsonpackages/ui/src/globals.csspackages/ui/src/i18n/init-service.tspackages/ui/src/markdown/components/code-group.tsxpackages/ui/src/markdown/globals.csspackages/ui/src/markdown/markdown.tsxpackages/ui/src/markdown/plugins/rehype-preserve-code-meta.tspackages/ui/src/markdown/shiki.tspackages/ui/src/theme/index.tspackages/ui/src/theme/theme-asset-sync.tsxpackages/ui/vitest.config.tspackages/webui/.storybook/main.tspackages/webui/package.jsonpackages/webui/src/globals.csspackages/webui/src/search/__fixtures__/sample-docs.tspackages/webui/src/search/build-index.test.tspackages/webui/src/search/build-index.tspackages/webui/src/search/client.test.tspackages/webui/src/search/client.tspackages/webui/src/search/dialog.test.tsxpackages/webui/src/search/dialog.tsxpackages/webui/src/search/highlight.test.tsxpackages/webui/src/search/highlight.tsxpackages/webui/src/search/recent-searches.test.tspackages/webui/src/search/search-empty.tsxpackages/webui/src/search/search-results.test.tsxpackages/webui/src/search/search-results.tsxpackages/webui/src/search/snippet.test.tspackages/webui/src/search/snippet.tspackages/webui/src/search/types.tspackages/webui/src/search/use-search.test.tspackages/webui/src/search/use-search.tspackages/webui/src/server.tspackages/webui/src/test/setup.tspackages/webui/vitest.config.tsservices/docs/app/components/docs/docs-toc.tsxservices/docs/app/globals.cssservices/docs/app/locals.cssservices/docs/app/main.tsxservices/docs/app/routes/__root.tsxservices/docs/index.htmlservices/docs/lib/i18n/i18n.tsservices/docs/messages/de.jsonservices/docs/messages/en.jsonservices/docs/messages/fr.jsonservices/docs/package.jsonservices/docs/postcss.config.mjsservices/docs/scripts/build-search-index.tsservices/docs/server.tsservices/docs/tailwind.config.tsservices/docs/tsconfig.jsonservices/docs/tsr.config.jsonservices/platform/.storybook/manager.tsservices/platform/.storybook/preview.tsxservices/platform/.storybook/theme.tsservices/platform/.storybook/vitest.setup.tsservices/platform/app/components/theme/theme-assets.tsxservices/platform/app/components/theme/theme-color-meta.tsxservices/platform/app/globals.cssservices/platform/app/locals.cssservices/platform/app/main.tsxservices/platform/app/router.tsxservices/platform/app/routes/__root.tsxservices/platform/components.jsonservices/platform/index.htmlservices/platform/lib/i18n/i18n.tsservices/platform/package.jsonservices/platform/server.tsservices/platform/tailwind.config.tsservices/platform/tsconfig.jsonservices/platform/tsr.config.jsonservices/platform/vitest.config.tsservices/web/app/globals.cssservices/web/app/locals.cssservices/web/app/main.tsxservices/web/components.jsonservices/web/lib/i18n/i18n.tsservices/web/server.tsservices/web/tailwind.config.tsservices/web/tsconfig.jsonservices/web/tsr.config.jsontools/plop/generators/react-package.tstools/plop/generators/react-service.tstools/plop/templates/react-package/src/globals.csstools/plop/templates/react-package/src/test/setup.tstools/plop/templates/react-package/vitest.config.ts.hbstools/plop/templates/react-service/.storybook/vitest.setup.tstools/plop/templates/react-service/app/globals.csstools/plop/templates/react-service/app/locals.csstools/plop/templates/react-service/app/main.tsx.hbstools/plop/templates/react-service/app/router.tsx.hbstools/plop/templates/react-service/components.jsontools/plop/templates/react-service/lib/i18n/i18n.ts.hbstools/plop/templates/react-service/tailwind.config.ts.hbstools/plop/templates/react-service/tsconfig.json.hbstools/plop/templates/react-service/tsr.config.jsontools/plop/templates/react-service/vitest.config.ts.hbs
💤 Files with no reviewable changes (12)
- services/platform/.storybook/vitest.setup.ts
- services/platform/components.json
- tools/plop/templates/react-service/components.json
- services/docs/app/globals.css
- tools/plop/templates/react-service/.storybook/vitest.setup.ts
- packages/webui/src/globals.css
- services/platform/vitest.config.ts
- services/web/components.json
- services/platform/.storybook/theme.ts
- services/web/app/globals.css
- services/docs/package.json
- services/platform/app/components/theme/theme-color-meta.tsx
Comment on lines
+263
to
+264
| font-family: 'Inter', ui-sans-serif, system-ui, sans-serif; | ||
| text-rendering: optimizeLegibility; |
Contributor
There was a problem hiding this comment.
Normalize these body styles to the current CSS lint rules.
Stylelint is already flagging the quoted Inter family and optimizeLegibility casing here, so this hunk won't lint clean as-is.
Suggested fix
- font-family: 'Inter', ui-sans-serif, system-ui, sans-serif;
- text-rendering: optimizeLegibility;
+ font-family: Inter, ui-sans-serif, system-ui, sans-serif;
+ text-rendering: optimizelegibility;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| font-family: 'Inter', ui-sans-serif, system-ui, sans-serif; | |
| text-rendering: optimizeLegibility; | |
| font-family: Inter, ui-sans-serif, system-ui, sans-serif; | |
| text-rendering: optimizelegibility; |
🧰 Tools
🪛 Stylelint (17.10.0)
[error] 263-263: Expected no quotes around "Inter" (font-family-name-quotes)
(font-family-name-quotes)
[error] 264-264: Expected "optimizeLegibility" to be "optimizelegibility" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/globals.css` around lines 263 - 264, Normalize the CSS to
satisfy stylelint by removing the unnecessary quotes around the Inter family and
lowercasing the text-rendering keyword: change font-family: 'Inter',
ui-sans-serif, system-ui, sans-serif; to font-family: Inter, ui-sans-serif,
system-ui, sans-serif; and change text-rendering: optimizeLegibility; to
text-rendering: optimizelegibility; (update these in globals.css where the
font-family and text-rendering declarations appear).
| import { initI18n } from './init'; | ||
| import { collectRegionalBundles } from './regional-bundles'; | ||
|
|
||
| type Bundle = Record<string, Record<string, unknown>>; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read it
cat -n packages/ui/src/i18n/init-service.tsRepository: tale-project/tale
Length of output: 1584
🏁 Script executed:
# Also check for any related files that might contain initI18n
fd "init" packages/ui/src/i18n/ -type fRepository: tale-project/tale
Length of output: 232
🏁 Script executed:
# Check the initI18n function and its return type
cat -n packages/ui/src/i18n/init.ts | head -100Repository: tale-project/tale
Length of output: 3159
🏁 Script executed:
# Also check if there are other type definitions in the i18n directory
ls -la packages/ui/src/i18n/Repository: tale-project/tale
Length of output: 745
Align public typing with TS guidelines (unknown + missing return type).
Please avoid unknown in Bundle and add an explicit return type on the exported initializer API.
♻️ Proposed fix
-type Bundle = Record<string, Record<string, unknown>>;
+type MessageValue =
+ | string
+ | number
+ | boolean
+ | null
+ | MessageValue[]
+ | { [key: string]: MessageValue };
+type Bundle = Record<string, Record<string, MessageValue>>;
@@
export function initServiceI18n({
bundles,
regional,
global,
-}: InitServiceParams) {
+}: InitServiceParams): ReturnType<typeof initI18n> {
return initI18n({
bundles: { ...bundles, ...collectRegionalBundles(regional) },
global,
});
}As per coding guidelines: "Never use as, any, or unknown in TypeScript" and "annotate public APIs, exported functions...".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/i18n/init-service.ts` at line 4, Change the Bundle type from
using unknown to a concrete allowed type (e.g., type Bundle = Record<string,
Record<string, string>>) so public typings do not use unknown, and add an
explicit return type annotation to the exported initializer function in this
module (the exported initializer API) to fully annotate its public signature;
update any internal places that rely on Bundle or the initializer return type to
satisfy the new stricter types.
Comment on lines
145
to
155
| function extractCode(child: ReactElement<CodeGroupChildProps>): string { | ||
| if (typeof child.props.code === 'string') return child.props.code; | ||
| if (typeof child.props.children === 'string') return child.props.children; | ||
| return ''; | ||
| // Markdown path: child is a <pre> (rendered by react-markdown's pre map) | ||
| // whose subtree contains <code class="language-X">…</code>. Stringify the | ||
| // nested code element's children to recover the source text. | ||
| const codeEl = findCodeElement(child.props.children); | ||
| if (!codeEl) return ''; | ||
| const codeProps = codeEl.props as { children?: ReactNode }; | ||
| return stringifyChildren(codeProps.children); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
extractCode returning '' silently renders an empty highlighted block.
When neither a direct code prop nor a nested <code> element is found, the fallback is an empty string passed to <HighlightedCode>, which will still render the panel chrome. Returning null/undefined and skipping the panel (or rendering a small placeholder) would fail more loudly when an authoring mistake strips the code. Optional — current behavior is at least non-crashing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/markdown/components/code-group.tsx` around lines 145 - 155,
The extractCode function currently returns an empty string when it can't find
code which leads HighlightedCode to render an empty panel; change extractCode
(and its usages) to return null (or undefined) instead of '' so callers can skip
rendering the panel, and update the caller that renders HighlightedCode to check
for a null/undefined result and omit the HighlightedCode panel (or render a
clear placeholder). Specifically, modify extractCode (referenced here) to return
null when no code element is found (use findCodeElement and stringifyChildren as
before), and update the component that consumes extractCode/HighlightedCode to
guard on the returned value and not render the HighlightedCode UI when it's
null.
Comment on lines
+180
to
+197
| function extractFilename( | ||
| child: ReactElement<CodeGroupChildProps>, | ||
| ): string | undefined { | ||
| if ( | ||
| typeof child.props.filename === 'string' && | ||
| child.props.filename.length > 0 | ||
| ) { | ||
| return child.props.filename; | ||
| } | ||
| const codeEl = findCodeElement(child.props.children); | ||
| const dataMeta = (codeEl?.props as { 'data-meta'?: string } | undefined)?.[ | ||
| 'data-meta' | ||
| ]; | ||
| if (typeof dataMeta === 'string' && dataMeta.trim().length > 0) { | ||
| return dataMeta.trim(); | ||
| } | ||
| return undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
Raw metastring may produce noisy tab labels.
extractFilename uses the entire data-meta string as the tab label. For Mintlify-style fences (```python Python) that's exactly right, but real-world fences often carry attribute-shaped meta — e.g. ```ts {1,3-5}, ```ts title="server.ts" showLineNumbers — and those will surface verbatim as tab text ({1,3-5} or title="server.ts" showLineNumbers). If you only want to honour the simple-label form, consider bailing out (or extracting title="…") when the meta contains {, =, or whitespace-separated tokens.
♻️ One possible refinement
const codeEl = findCodeElement(child.props.children);
const dataMeta = (codeEl?.props as { 'data-meta'?: string } | undefined)?.[
'data-meta'
];
- if (typeof dataMeta === 'string' && dataMeta.trim().length > 0) {
- return dataMeta.trim();
+ if (typeof dataMeta === 'string' && dataMeta.trim().length > 0) {
+ const meta = dataMeta.trim();
+ // Prefer an explicit `title="…"` attribute when present.
+ const titleMatch = /title=(?:"([^"]+)"|'([^']+)'|(\S+))/.exec(meta);
+ if (titleMatch) return titleMatch[1] ?? titleMatch[2] ?? titleMatch[3];
+ // Otherwise only accept a single bareword label (Mintlify-style).
+ if (/^[^\s{=]+$/.test(meta)) return meta;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/markdown/components/code-group.tsx` around lines 180 - 197,
The extractFilename function is using the raw data-meta string as the tab label
which surfaces noisy attributes like `{1,3-5}` or `title="server.ts"
showLineNumbers`; update extractFilename (and the spot where it reads dataMeta
via findCodeElement) to first detect attribute-shaped metas and only accept
simple single-token labels: if dataMeta contains braces `{`, an equals `=`, or
whitespace-separated tokens, attempt to extract a title attribute by matching
title="..."/title='...' and return that value trimmed; if no title is present,
bail out and return undefined so only pure simple labels (e.g., "Python") are
used as tab text.
Comment on lines
7
to
10
| "exports": { | ||
| ".": "./src/index.ts", | ||
| "./server": "./src/server.ts", | ||
| "./layout/site-container": "./src/layout/site-container.tsx", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json
from pathlib import Path
pkg = json.loads(Path("packages/webui/package.json").read_text())
declared = {}
declared.update(pkg.get("dependencies", {}))
declared.update(pkg.get("peerDependencies", {}))
print("@tale/i18n declared in package.json:", declared.get("@tale/i18n"))
PY
rg -n "from '@tale/i18n/(cookie|negotiate)'" packages/webui/src/server.tsRepository: tale-project/tale
Length of output: 199
Declare @tale/i18n before exporting ./server.
./src/server.ts imports @tale/i18n/cookie and @tale/i18n/negotiate (lines 14, 18), but @tale/webui doesn't declare @tale/i18n as a dependency. Adding ./server as a public entrypoint exposes this undeclared dependency to consumers, breaking installs and builds.
Suggested fix
"dependencies": {
"@radix-ui/react-dialog": "1.1.15",
+ "@tale/i18n": "workspace:*",
"@tale/ui": "workspace:*",
"@tanstack/react-router": "1.168.10",
"framer-motion": "12.38.0",
"lucide-react": "1.8.0",
"minisearch": "7.2.0"
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/webui/package.json` around lines 7 - 10, The package exports
"./server" which exposes ./src/server.ts that imports from `@tale/i18n` (e.g., its
cookie and negotiate modules), but `@tale/i18n` is not declared—add `@tale/i18n` to
the package.json dependencies (not devDependencies) so consumers installing
`@tale/webui` get the required runtime package; ensure you pick the correct semver
version used by the repo, update package.json "dependencies" to include
"@tale/i18n": "<appropriate-version>", and reinstall/lock to validate the fix.
Comment on lines
+238
to
257
| <span | ||
| className="text-fg-subtle mt-1 flex min-w-0 items-center gap-1 truncate text-[10.5px]" | ||
| aria-label={result.url} | ||
| > | ||
| {breadcrumb.length > 0 ? ( | ||
| breadcrumb.map((segment, i) => ( | ||
| <span | ||
| key={`${segment}-${i}`} | ||
| className="inline-flex shrink-0 items-center gap-1" | ||
| > | ||
| {i > 0 ? ( | ||
| <ChevronRight aria-hidden className="size-2.5 opacity-60" /> | ||
| ) : null} | ||
| <span className="truncate">{segment}</span> | ||
| </span> | ||
| )) | ||
| ) : ( | ||
| <span className="font-mono">{result.url}</span> | ||
| )} | ||
| </span> |
Contributor
There was a problem hiding this comment.
aria-label={result.url} on a span replaces friendly breadcrumb text in the AT announcement.
aria-label overrides an element's accessible name. On this span the visible content is the humanised breadcrumb (e.g., "Self Hosted › Configuration"), but the accessibility tree will substitute the raw URL /self-hosted/configuration/retention — so screen-reader users hear the URL while sighted users see the breadcrumb. The option's overall accessible name (button content concatenation) is also degraded.
If the goal is a hover/tooltip with the URL, use title. If the goal is a programmatic data hook for the test, use data-url. Either lets the visible breadcrumb keep its accessible name.
🛡️ Suggested fix
<span
className="text-fg-subtle mt-1 flex min-w-0 items-center gap-1 truncate text-[10.5px]"
- aria-label={result.url}
+ title={result.url}
+ data-url={result.url}
>The search-results.test.tsx selector at line 278 ([aria-label="/self-hosted/configuration/retention"]) needs to follow — switch it to [data-url="…"].
As per coding guidelines: "Accessibility must meet WCAG 2.1 Level AA standards — use real HTML elements, ensure keyboard reachability, maintain visible focus, label controls, and verify AA contrast".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <span | |
| className="text-fg-subtle mt-1 flex min-w-0 items-center gap-1 truncate text-[10.5px]" | |
| aria-label={result.url} | |
| > | |
| {breadcrumb.length > 0 ? ( | |
| breadcrumb.map((segment, i) => ( | |
| <span | |
| key={`${segment}-${i}`} | |
| className="inline-flex shrink-0 items-center gap-1" | |
| > | |
| {i > 0 ? ( | |
| <ChevronRight aria-hidden className="size-2.5 opacity-60" /> | |
| ) : null} | |
| <span className="truncate">{segment}</span> | |
| </span> | |
| )) | |
| ) : ( | |
| <span className="font-mono">{result.url}</span> | |
| )} | |
| </span> | |
| <span | |
| className="text-fg-subtle mt-1 flex min-w-0 items-center gap-1 truncate text-[10.5px]" | |
| title={result.url} | |
| data-url={result.url} | |
| > | |
| {breadcrumb.length > 0 ? ( | |
| breadcrumb.map((segment, i) => ( | |
| <span | |
| key={`${segment}-${i}`} | |
| className="inline-flex shrink-0 items-center gap-1" | |
| > | |
| {i > 0 ? ( | |
| <ChevronRight aria-hidden className="size-2.5 opacity-60" /> | |
| ) : null} | |
| <span className="truncate">{segment}</span> | |
| </span> | |
| )) | |
| ) : ( | |
| <span className="font-mono">{result.url}</span> | |
| )} | |
| </span> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/webui/src/search/search-results.tsx` around lines 238 - 257, The
span that renders the breadcrumb currently sets aria-label={result.url}, which
overrides the accessible name (causing screen readers to read the raw URL);
change the span to remove aria-label and instead add title={result.url} (for
hover tooltip) or data-url={result.url} (for tests/automation) while keeping the
visible breadcrumb text intact; update the test selector in
search-results.test.tsx from [aria-label="/self-hosted/configuration/retention"]
to the corresponding [data-url="/self-hosted/configuration/retention"] (or adapt
to title if you chose title) and ensure the span with className "text-fg-subtle
mt-1 flex..." is the element receiving the data-url/title.
Comment on lines
+223
to
+238
| const resolved = resolve(distDir, rel); | ||
| if (resolved === distDir || resolved.startsWith(distPrefix)) { | ||
| const candidate = file(resolved); | ||
| if (await candidate.exists()) { | ||
| const ct = contentTypeFor(pathname); | ||
| return new Response(candidate, { | ||
| headers: ct ? { 'content-type': ct } : undefined, | ||
| }); | ||
| } | ||
| // Try the prerendered route HTML (e.g. /pricing → dist/pricing/index.html). | ||
| const routeHtml = file(join(resolved, 'index.html')); | ||
| if (await routeHtml.exists()) { | ||
| return new Response(routeHtml); | ||
| } | ||
| } | ||
| return new Response(file(join(distDir, 'index.html'))); |
Contributor
There was a problem hiding this comment.
Return 404 for missing asset paths instead of the SPA shell.
This falls back to index.html for every unresolved request, including /assets/*.js, CSS, and image URLs. After a deploy or a broken asset reference, browsers will get HTML with 200 at a JS/CSS URL, which is much harder to diagnose and can be cached incorrectly. Keep the SPA fallback for extensionless routes, but return 404 for missing asset-like paths.
Suggested fix
const resolved = resolve(distDir, rel);
+ const isAssetLikePath = /\.[^/]+$/.test(rel);
if (resolved === distDir || resolved.startsWith(distPrefix)) {
const candidate = file(resolved);
if (await candidate.exists()) {
const ct = contentTypeFor(pathname);
return new Response(candidate, {
headers: ct ? { 'content-type': ct } : undefined,
});
}
// Try the prerendered route HTML (e.g. /pricing → dist/pricing/index.html).
const routeHtml = file(join(resolved, 'index.html'));
if (await routeHtml.exists()) {
return new Response(routeHtml);
}
}
+ if (isAssetLikePath) {
+ return new Response('Not found', { status: 404 });
+ }
return new Response(file(join(distDir, 'index.html')));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const resolved = resolve(distDir, rel); | |
| if (resolved === distDir || resolved.startsWith(distPrefix)) { | |
| const candidate = file(resolved); | |
| if (await candidate.exists()) { | |
| const ct = contentTypeFor(pathname); | |
| return new Response(candidate, { | |
| headers: ct ? { 'content-type': ct } : undefined, | |
| }); | |
| } | |
| // Try the prerendered route HTML (e.g. /pricing → dist/pricing/index.html). | |
| const routeHtml = file(join(resolved, 'index.html')); | |
| if (await routeHtml.exists()) { | |
| return new Response(routeHtml); | |
| } | |
| } | |
| return new Response(file(join(distDir, 'index.html'))); | |
| const resolved = resolve(distDir, rel); | |
| const isAssetLikePath = /\.[^/]+$/.test(rel); | |
| if (resolved === distDir || resolved.startsWith(distPrefix)) { | |
| const candidate = file(resolved); | |
| if (await candidate.exists()) { | |
| const ct = contentTypeFor(pathname); | |
| return new Response(candidate, { | |
| headers: ct ? { 'content-type': ct } : undefined, | |
| }); | |
| } | |
| // Try the prerendered route HTML (e.g. /pricing → dist/pricing/index.html). | |
| const routeHtml = file(join(resolved, 'index.html')); | |
| if (await routeHtml.exists()) { | |
| return new Response(routeHtml); | |
| } | |
| } | |
| if (isAssetLikePath) { | |
| return new Response('Not found', { status: 404 }); | |
| } | |
| return new Response(file(join(distDir, 'index.html'))); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/webui/src/server.ts` around lines 223 - 238, The resolver currently
falls back to dist/index.html for every missing path; change it to return 404
for asset-like requests while keeping the SPA fallback for extensionless routes:
detect whether the incoming pathname is an asset (e.g., has a file extension via
path.extname(pathname) or a '.' after the last '/'), and if an asset-path
candidate (resolved or routeHtml) does not exist return a 404 Response instead
of serving index.html; only when the pathname is extensionless should you
continue to return the SPA shell by returning file(join(distDir, 'index.html')).
Use the existing symbols (resolve, distDir, distPrefix, file(...),
contentTypeFor(pathname), pathname, routeHtml) to locate and implement this
conditional.
Comment on lines
91
to
93
| } catch (_e) { | ||
| /* localStorage unavailable; fall through to React default */ | ||
| } |
Contributor
There was a problem hiding this comment.
Do not silently swallow bootstrap errors in this catch.
This catch currently hides all failures in theme bootstrapping; log and continue fallback so regressions are visible.
Proposed fix
- } catch (_e) {
- /* localStorage unavailable; fall through to React default */
+ } catch (error) {
+ console.warn('Theme bootstrap fallback:', error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (_e) { | |
| /* localStorage unavailable; fall through to React default */ | |
| } | |
| } catch (error) { | |
| console.warn('Theme bootstrap fallback:', error); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/docs/index.html` around lines 91 - 93, The catch block that
currently swallows all errors during theme bootstrapping (catch (_e) { /*
localStorage unavailable; fall through to React default */ }) should log the
caught error and context before continuing with the fallback; update that catch
to capture the error variable (e or err) and call console.error or the existing
logger with a clear message like "Theme bootstrap failed, falling back to
defaults" plus the error object so regressions are visible, then allow the
function to proceed with the React default behavior.
Comment on lines
8
to
+13
| type Bundle = Record<string, Record<string, unknown>>; | ||
|
|
||
| // Auto-discover regional variants (e.g. `de-CH.json`, future `fr-CA.json`). | ||
| // Drop a `xx-YY.json` file in `messages/` and Vite picks it up here. | ||
| // Vite requires `import.meta.glob` patterns to be literal relative or | ||
| // absolute paths — aliases (`@/...`) are rejected at build time. | ||
| const regionalBundles = collectRegionalBundles( | ||
| import.meta.glob<Bundle>('../../messages/*-*.json', { | ||
| export const i18n = initServiceI18n({ | ||
| bundles: { en: enMessages, de: deMessages, fr: frMessages }, | ||
| // Vite requires the glob pattern to be a literal at the call site. | ||
| regional: import.meta.glob<Bundle>('../../messages/*-*.json', { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Avoid unknown in the bundle shape.
This local alias weakens the glob contract and violates the repo's TS rule. Use a concrete recursive message-tree type here, or reuse one exported by the shared i18n package.
♻️ Proposed typing change
-type Bundle = Record<string, Record<string, unknown>>;
+type Bundle = Record<string, string | Bundle>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type Bundle = Record<string, Record<string, unknown>>; | |
| // Auto-discover regional variants (e.g. `de-CH.json`, future `fr-CA.json`). | |
| // Drop a `xx-YY.json` file in `messages/` and Vite picks it up here. | |
| // Vite requires `import.meta.glob` patterns to be literal relative or | |
| // absolute paths — aliases (`@/...`) are rejected at build time. | |
| const regionalBundles = collectRegionalBundles( | |
| import.meta.glob<Bundle>('../../messages/*-*.json', { | |
| export const i18n = initServiceI18n({ | |
| bundles: { en: enMessages, de: deMessages, fr: frMessages }, | |
| // Vite requires the glob pattern to be a literal at the call site. | |
| regional: import.meta.glob<Bundle>('../../messages/*-*.json', { | |
| type Bundle = Record<string, string | Bundle>; | |
| export const i18n = initServiceI18n({ | |
| bundles: { en: enMessages, de: deMessages, fr: frMessages }, | |
| // Vite requires the glob pattern to be a literal at the call site. | |
| regional: import.meta.glob<Bundle>('../../messages/*-*.json', { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/web/lib/i18n/i18n.ts` around lines 8 - 13, The alias Bundle uses
Record<string, Record<string, unknown>> which uses `unknown` and weakens the
glob contract; update the type used for the regional import to a concrete
recursive message-tree type (or import the exported message-tree type from the
shared i18n package) and replace Bundle with that type where referenced (e.g.,
the Bundle alias, the import.meta.glob<Bundle> call and the initServiceI18n
bundles object) so the glob contract is strongly typed without `unknown` (use a
recursive shape like a map of keys to string or nested maps or the shared
package's MessageTree type).
Comment on lines
+1
to
+7
| import path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| {{#if storybook}}import { storybookTest } from '@storybook/addon-vitest/vitest-plugin'; | ||
| {{/if}}import react from '@vitejs/plugin-react'; | ||
| {{#if storybook}}import { playwright } from '@vitest/browser-playwright'; | ||
| {{/if}}import { defineConfig } from 'vitest/config'; |
Contributor
There was a problem hiding this comment.
Conditionalize the node:path import so non-storybook scaffolds don't ship an unused import.
path is only referenced inside the {{#if storybook}} block on line 9 (path.dirname(...)). When storybook is false, the generated vitest.config.ts still imports node:path with no usage, which will trip noUnusedLocals / no-unused-vars lint in any scaffolded package that inherits the standard toolchain.
♻️ Proposed template fix
-import path from 'node:path';
-import { fileURLToPath } from 'node:url';
+{{`#if` storybook}}import path from 'node:path';
+{{/if}}import { fileURLToPath } from 'node:url';Note: the ember-template-lint warnings flagged by static analysis (lines 4, 6, 9, 19) are false positives — this is a Handlebars/Plop template, not an Ember .hbs template, so the no-implicit-this rule does not apply here.
🧰 Tools
🪛 ember-template-lint (7.9.3)
[error] 4-4: Ambiguous path 'storybook' is not allowed. Use '@storybook' if it is a named argument or 'this.storybook' if it is a property on 'this'. If it is a helper or component that has no arguments, you must either convert it to an angle bracket invocation or manually add it to the 'no-implicit-this' rule configuration, e.g. 'no-implicit-this': { allow: ['storybook'] }.
(no-implicit-this)
[error] 6-6: Ambiguous path 'storybook' is not allowed. Use '@storybook' if it is a named argument or 'this.storybook' if it is a property on 'this'. If it is a helper or component that has no arguments, you must either convert it to an angle bracket invocation or manually add it to the 'no-implicit-this' rule configuration, e.g. 'no-implicit-this': { allow: ['storybook'] }.
(no-implicit-this)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/plop/templates/react-package/vitest.config.ts.hbs` around lines 1 - 7,
The template currently always imports node:path even though it's only used when
the storybook flag is true (path.dirname(...) in the storybook block); update
the Handlebars template so the import "import path from 'node:path';" is wrapped
inside the {{`#if` storybook}}...{{/if}} block to avoid an unused import when
storybook is false, keeping the existing imports for react, defineConfig, and
the conditional storybookTest/playwright imports unchanged; ensure the path
usage (path.dirname(...)) remains inside the same {{`#if` storybook}} block so
there are no dangling references.
Contributor
Author
|
Closing as duplicate of #1700 (same branch, same commits, better-formatted body). |
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
1 participant
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.
Cross-service alignment + bundle of pre-existing WIP. Reduces boilerplate across services/platform, services/web, services/docs and pulls common patterns into @tale/ui and @tale/webui.
Service-shell alignment
Shared utilities pulled into packages
Other
Pre-PR checklist
Summary
Pre-merge checklist
Tick each box or mark N/A with a short reason. Empty boxes are a blocker.
bun run check(format, lint, typecheck, all tests).services/platform/messages/{en,de,fr}.jsonfor any new/renamed/removed keys — or N/A.docs/,docs/de/, anddocs/fr/for every user-visible change — or N/A.bun run --filter @tale/docs lint(broken links + oxlint) — or N/A.README.md,README.de.md,README.fr.mdif the change affects what they document — or N/A.Does this change need docs and translations? (decision tree)
Walk top-down. First yes wins.
services/platform/messages/? → Yes.If unsure, default to yes. Reviewer time is cheaper than stale docs.
Test plan
Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements