Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Killed RAG feature #727

Merged
merged 1 commit into from
Nov 21, 2024
Merged

fix: Killed RAG feature #727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions apps/webapp/Dockerfile.slim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
FROM node:20-alpine AS base
ENV PNPM_HOME="/pnpm"
ENV PATH="$PNPM_HOME:$PATH"
RUN apk add --no-cache libc6-compat && \
corepack enable
Comment on lines +4 to +5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Pin versions in apk add command for reproducible builds

To ensure reproducible builds and prevent unexpected issues from package updates, it's recommended to pin the versions when installing packages with apk add.

You can modify the command as follows:

- RUN apk add --no-cache libc6-compat && \
+ RUN apk add --no-cache libc6-compat=<version> && \

Replace <version> with the specific version number required.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Hadolint (2.12.0)

[warning] 4-4: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>

(DL3018)


WORKDIR /app

# Install Turbo
RUN pnpm add -g turbo@1.13.4

# Copy necessary files for turbo prune
COPY . .

# Prune the workspace
RUN turbo prune --scope=webapp --docker

# Installer stage
FROM base AS installer
WORKDIR /app

# Copy pruned files
COPY --from=base /app/out/json/ .
COPY --from=base /app/out/pnpm-lock.yaml ./pnpm-lock.yaml
COPY --from=base /app/out/full/ .

# Install dependencies
RUN pnpm install --shamefully-hoist

# Build shared package first
RUN cd packages/shared && pnpm run build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Use WORKDIR instead of cd for changing directories

Using WORKDIR to change directories is more efficient and makes the Dockerfile cleaner compared to using cd in a RUN command.

You can modify the Dockerfile like this:

- RUN cd packages/shared && pnpm run build
+ WORKDIR /app/packages/shared
+ RUN pnpm run build

If you need to return to the previous directory, you can add another WORKDIR command after the build step.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 Hadolint (2.12.0)

[warning] 31-31: Use WORKDIR to switch to a directory

(DL3003)


# Build the webapp
RUN pnpm run build --filter=webapp...

# Runner stage
FROM node:20-alpine AS runner
WORKDIR /app

# Don't run production as root
RUN addgroup --system --gid 1001 nodejs
RUN adduser --system --uid 1001 nextjs
Comment on lines +41 to +42
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consolidate consecutive RUN instructions to optimize image layers

Combining multiple RUN commands into one reduces the number of layers in the final image, improving build efficiency.

You can modify the commands as follows:

- RUN addgroup --system --gid 1001 nodejs
- RUN adduser --system --uid 1001 nextjs
+ RUN addgroup --system --gid 1001 nodejs && \
+     adduser --system --uid 1001 nextjs
📝 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
RUN addgroup --system --gid 1001 nodejs
RUN adduser --system --uid 1001 nextjs
RUN addgroup --system --gid 1001 nodejs && \
adduser --system --uid 1001 nextjs
🧰 Tools
🪛 Hadolint (2.12.0)

[info] 42-42: Multiple consecutive RUN instructions. Consider consolidation.

(DL3059)


# Copy necessary files
COPY --from=installer /app/apps/webapp/.next/standalone ./
COPY --from=installer /app/apps/webapp/.next/static ./apps/webapp/.next/static
COPY --from=installer /app/apps/webapp/public ./apps/webapp/public

# Copy package.json files
COPY --from=installer /app/apps/webapp/package.json ./package.json

# Install only production dependencies

Comment on lines +52 to +53
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Remove or update the comment about installing production dependencies

The comment # Install only production dependencies is not followed by any commands, which might cause confusion.

If no action is needed here, consider removing the comment:

- # Install only production dependencies

Alternatively, if you need to install production dependencies, add the appropriate command after the comment.

📝 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
# Install only production dependencies

USER nextjs

ENV NODE_ENV=production
ENV PORT=8090

EXPOSE 8090

CMD ["node", "server.js"]
8 changes: 7 additions & 1 deletion apps/webapp/next.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
/** @type {import('next').NextConfig} */
import path from 'path';
import { fileURLToPath } from 'url';
Comment on lines +2 to +3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use the node: protocol for Node.js built-in modules

For better clarity and explicit identification of Node.js built-in modules, use the node: protocol prefix.

-import path from 'path';
-import { fileURLToPath } from 'url';
+import path from 'node:path';
+import { fileURLToPath } from 'node:url';
📝 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
import path from 'path';
import { fileURLToPath } from 'url';
import path from 'node:path';
import { fileURLToPath } from 'node:url';
🧰 Tools
🪛 Biome (1.9.4)

[error] 2-2: A Node.js builtin module should be imported with the node: protocol.

Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.

(lint/style/useNodejsImportProtocol)

[error] 3-3: A Node.js builtin module should be imported with the node: protocol.

Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.

(lint/style/useNodejsImportProtocol)


const __dirname = path.dirname(fileURLToPath(import.meta.url));

const nextConfig = {
output: 'standalone',
outputFileTracingRoot: path.join(__dirname, '../../'),
async redirects() {
return [
{
Expand All @@ -12,4 +18,4 @@ const nextConfig = {
}
};

export default nextConfig;
export default nextConfig;
5 changes: 0 additions & 5 deletions apps/webapp/src/app/(Dashboard)/configuration/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import useUpdatePullFrequency from "@/hooks/create/useCreatePullFrequency";
import { toast } from "sonner";
import { useQueryClient } from "@tanstack/react-query";
import { Loader2 } from "lucide-react";
import RAGSettingsPage from "@/components/Configuration/RAGSettings/RAGSettingsPage";

const frequencyOptions = [
{ label: '5 min', value: 300 },
Expand Down Expand Up @@ -185,7 +184,6 @@ export default function Page() {
<TabsTrigger value="pull-frequency">
Sync Frequency
</TabsTrigger>
<TabsTrigger value="rag-settings">RAG Settings</TabsTrigger>
<TabsTrigger value="catalog">
Manage Catalog Widget
</TabsTrigger>
Expand Down Expand Up @@ -360,9 +358,6 @@ export default function Page() {
</div>
</TabsContent>

<TabsContent value="rag-settings" className="space-y-4">
<RAGSettingsPage />
</TabsContent>

<TabsContent value="custom" className="space-y-4">
<CustomConnectorPage />
Expand Down
Loading
Loading